[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-commits
Subject:    Re: KDE/kdelibs/khtml
From:       David Faure <faure () kde ! org>
Date:       2008-01-31 21:00:41
Message-ID: 200801312200.44144.faure () kde ! org
[Download RAW message or body]

On Sunday 13 January 2008, Aurélien Gâteau wrote:
> Le Monday 07 January 2008 02:07:41, vous avez écrit :
> > On Monday 07 January 2008, David Faure wrote:
> > > On Sunday 06 January 2008, Aurélien Gâteau wrote:
> > > > SVN commit 757890 by gateau:
> > > >
> > > > Fix crashing when processObjectRequest() opens a dialog.
> > > > For example when visiting netvibes.com
> > > >
> > > >
> > > >  M  +8 -1      khtml_run.cpp
> > > >
> > > >
> > > > --- trunk/KDE/kdelibs/khtml/khtml_run.cpp #757889:757890
> > > > @@ -50,7 +50,14 @@
> > > >  {
> > > >      Q_ASSERT(!hasFinished());
> > > >      QString mimeType = _type; // this ref comes from the job, we lose
> > > > it when using KIO again -    if ( static_cast<KHTMLPart
> > > > *>(part())->processObjectRequest( m_child, KRun::url(), mimeType ) ) +
> > > > +    // Disable autoDelete for processObjectRequest, because it may
> > > > open a +    // dialog
> > > > +    bool autoDeleteWasEnabled = autoDelete();
> > > > +    setAutoDelete( false );
> > > > +    bool requestProcessed = static_cast<KHTMLPart
> > > > *>(part())->processObjectRequest( m_child, KRun::url(), mimeType ); +  
> > > >  setAutoDelete( autoDeleteWasEnabled );
> > > > +    if ( requestProcessed )
> > > >          setFinished( true );
> > > >      else {
> > > >          if ( hasFinished() ) // abort was called (this happens with
> > > > the activex fallback for instance)
> > >
> > > The problem is that now the KHTMLRun is leaking, since the timer (inside
> > > krun) isn't launched again by setFinished. You probably need a
> > > timer().start(0). Won't change anything if there was no message box, and
> > > will trigger the autodeletion if there was one.
> >
> > In fact this means that the above fix could be written in a simpler way...
> >
> > timer().stop();
> > bool requestProcessed = static_cast<KHTMLPart
> > *>(part())->processObjectRequest( m_child, KRun::url(), mimeType );
> > timer().start(0);
> > if ( requestProcessed )
> 
> This solution doesn't work: I still get the crash. I didn't investigate really 
> closely but I think it's because KRun autodelete timer is started from 
> several places. I suspect slotStatResult is the culprit.
> 
> For the same reason, the KRun instance is in fact deleted in the current code 
> because the timer is restarted somewhere. Still I agree restarting the timer 
> explicitly is safer. Attached patch does this. It also provide better 
> comments about this tricky code. Should I commit?

This change introduces regressions like #156447 (because the start(0) is done
too early, there's another dialog box in the code below it, handleNonEmbeddable).

Reverting your change fixes that page (which doesn't surprise me since the bug
doesn't happen in kde3 either - after removing kmidpart to trigger the Open With dialog there as well),
so now I wanted to debug the problem that made you create the above fix in 
the first place, but I don't know how to do that: "konqueror netvibes.com" 
doesn't crash even after reverting your fix.

In fact I wonder if this could be due to Maksim's other KIO fixes: jobs used to emit
error() too often (right?) so this would trigger KRun slots at the wrong time
(e.g. during the foundMimeType code) and create this bug. But I'm very tempted
to just revert the KHTMLRun hack unless I can be shown a case where it would
still crash without the hack.

-- 
David Faure, faure@kde.org, sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).

[prev in list] [next in list] [prev in thread] [next in thread]