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