[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-02-01 0:07:22
Message-ID: 200802010107.24898.faure () kde ! org
[Download RAW message or body]

On Friday 01 February 2008, Aurélien Gâteau wrote:
> David Faure a écrit :
> > 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).
> 
> Oh... sorry about that.
> 
> > 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.
> 
> Which change are you talking about? the iframe change or the "start the 
> delete timer just in case" change?

The one I quoted above, i.e. the "stop autodelete+timer; show dialog ; set \
autodelete+restart timer" change.

> The second one was supposed to be safer. If it's worse than without it, 
> it should go.

I think so too.

> As for reproducing the crash I got without any of the patches applied, 
> what I can say is that it didn't crash for me if I set Konqueror to 
> automatically open text/plain files without prompting. With the default 
> settings I had 6 dialog boxes asking me what to do with empty.txt. 
> Closing those dialog boxes caused Konqueror to crash.

OK I'll see if I get a different result after changing the filetypes settings, \
tomorrow.

-- 
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]