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

List:       kde-commits
Subject:    Re: KDE/kdelibs/khtml
From:       Aurélien_Gâteau <aurelien.gateau () free ! fr>
Date:       2008-01-31 23:49:43
Message-ID: 47A25E97.3050808 () free ! fr
[Download RAW message or body]

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 second one was supposed to be safer. If it's worse than without it, 
it should go.

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.

Aurélien
[prev in list] [next in list] [prev in thread] [next in thread]