Bug 104525 – [Qt] Crash in WebCore::CachedFrame::destroy

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Jocelyn Turcotte]

Created attachment 178493 [details]
Stack trace

I have no specific repro steps but this could be related to http://trac.webkit.org/changeset/125592

From my look at the change I think it would be sane to put a null check there.

[Jocelyn Turcotte]

Created attachment 178505 [details]
Patch

I only reproduced the problem randomly so I couldn't get a layout test to cover this.

[Alexey Proskuryakov]

> From my look at the change I think it would be sane to put a null check there.

DOMWindow::willDestroyCachedFrame() looks like it performs important work. When does this work get performed otherwise?

> I only reproduced the problem randomly so I couldn't get a layout test to cover this.

Can you please share the way to reproduce this, even if only sometimes? That could help a reviewer suggest ways to test.

> If Document::takeDOMWindowFrom has been called on the Document

Do you have a stack trace for that happening early?

[Jocelyn Turcotte]

Comment on attachment 178505 [details]
Patch

(In reply to comment #2)
> DOMWindow::willDestroyCachedFrame() looks like it performs important work. When does this work get performed otherwise?

From a quick look it felt like most of this work would be already done through the DOMWindow's destructor, but you're right, this might not be the best fix.

I'll dig deeper in the core file if I can reproduce it again.

[Jocelyn Turcotte]

I couldn't see that crash again within one week of browsing.
I'm resolving is as can't reproduce until I see it again to reduce the noise on the blocker bug.

Attempting a fix wasn't a good idea without proper knowledge about the issue.

[Lindsay Mathieson]

I'm seeing this exact bug extremely often (every time a wndow is closed) in QtWebKit-2.3

[Brady Eidson]

If you can reproduce it easily, then you should be able to provide a stack trace to this bug.

[Lindsay Mathieson]

Sure, can do - its the same as the attached one. Will post tonight.

[Adam Barth]

Comment on attachment 178505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178505&action=review

> Source/WebCore/ChangeLog:9
> +        If Document::takeDOMWindowFrom has been called on the Document in our
> +        CachedFrame, Document::domWindow() might return nullptr at that point.

It doesn't make sense to have a CachedFrame that has a Document without a DOMWindow.  What if we tried to restore that CachedFrame?  We'd be in some crazy inconsistent state.  It's more likely the fix should be around the time we call takeDOMWindowFrom.  We shouldn't be trying to take the DOMWindow from a Document that's stored in a CachedFrame.  Only Documents that are currently displayed in a Frame (i.e., not in a CachedFrame) should have their DOMWindows taken (this happens in rare cases when we rip the document out of the frame and replace it with a new document).

[Jocelyn Turcotte]

The issue seems to be that we call HistoryController::setCurrentItem when deserializing a previous navigation history on a new tab at Source/WebKit/qt/Api/qwebhistory.cpp:546.

This puts the HistoryController in a bad shape before doing the navigation in QWebHistory::goToItem. The current history item is pointing to where we want to go, but the current document is still the initial empty document of the tab we just created.

The initial empty document usually gets its DOMWindow taken in DocumentWriter::begin and it seems like what makes sure that it isn't placed in a CachedFrame is only the FrameLoader not having a current HistoryItem.
Our call to HistoryController::setCurrentItem will then cause PageCache::canCachePageContainingThisFrame to return true, the initial document will be put in a CachedFrame, its DOMWindow will be taken away in in DocumentWriter::begin and CachedFrame::destroy will then crash.

A trivial fix would be to find another way to fix bug #37322 than calling HistoryController::setCurrentItem, but I first have to make sure that we won't regress by just removing that line. tst_QWebHistory::serialize_2() doesn't go through FrameLoader::navigateWithinDocument anymore.

Adding an ASSERT(!inPageCache()) in Document::takeDOMWindowFrom would probably be a good idea too.

[Lindsay Mathieson]

Created attachment 181494 [details]
Stack Trace from Rekonq crash

This happens with the latest QtWekKit 2.3 and rekonq, every time I close a tab or very often when navigating away from a page.

[Jocelyn Turcotte]

Created attachment 182354 [details]
Patch

Fixes the source of the problem and adds an ASSERT in Document::takeDOMWindowFrom.

[Adam Barth]

Comment on attachment 182354 [details]
Patch

LGTM, but I don't know if you want someone more knowledgeable in Qt to approve the Qt part of this change.

[Allan Sandfeld Jensen]

(In reply to comment #12)
> (From update of attachment 182354 [details])
> LGTM, but I don't know if you want someone more knowledgeable in Qt to approve the Qt part of this change.

Looks good to me too.

[Lindsay Mathieson]

(In reply to comment #11)
> Created an attachment (id=182354) [details]
> Patch
> 
> Fixes the source of the problem and adds an ASSERT in Document::takeDOMWindowFrom.


I applied it to 2.3, didn't fix it there.

[Jocelyn Turcotte]

(In reply to comment #14)
> I applied it to 2.3, didn't fix it there.

If you build in debug with that patch you should run into the assert I added instead of the crash in CachedFrame::destroy.
Could you verify and get a stack trace if you do?

[Lindsay Mathieson]

(In reply to comment #15)
> (In reply to comment #14)
> > I applied it to 2.3, didn't fix it there.
> 
> If you build in debug with that patch you should run into the assert I added instead of the crash in CachedFrame::destroy.
> Could you verify and get a stack trace if you do?

Will do - will take me a couple hours, I don't have the speediest of environments :)

Thanks for working on this.

[Lindsay Mathieson]

Ah, I forgot - have been unable to build the debug version:

ar: libWebCore.a: File truncated
make[3]: *** [debug/libWebCore.a] Error 1
make[3]: Leaving directory `/data/dev/qtwebkit-23/WebKitBuild/Debug/Source/WebCore'
make[2]: *** [sub-Target-pri-make_default-ordered] Error 2
make[2]: Leaving directory `/data/dev/qtwebkit-23/WebKitBuild/Debug/Source/WebCore'
make[1]: *** [sub-Source-WebCore-WebCore-pro-make_default-ordered] Error 2
make[1]: Leaving directory `/data/dev/qtwebkit-23/WebKitBuild/Debug'
make: *** [incremental] Error 2


Happens everytime.

[Allan Sandfeld Jensen]

(In reply to comment #17)
> Ah, I forgot - have been unable to build the debug version:
> 
> ar: libWebCore.a: File truncated
> make[3]: *** [debug/libWebCore.a] Error 1
> make[3]: Leaving directory `/data/dev/qtwebkit-23/WebKitBuild/Debug/Source/WebCore'
> make[2]: *** [sub-Target-pri-make_default-ordered] Error 2
> make[2]: Leaving directory `/data/dev/qtwebkit-23/WebKitBuild/Debug/Source/WebCore'
> make[1]: *** [sub-Source-WebCore-WebCore-pro-make_default-ordered] Error 2
> make[1]: Leaving directory `/data/dev/qtwebkit-23/WebKitBuild/Debug'
> make: *** [incremental] Error 2
> 
> 
> Happens everytime.

Because the archive is larger than 4Gbyte. Ar have that limitation. Try CONFIG += force_static_libs_as_shared.

[WebKit Review Bot]

Comment on attachment 182354 [details]
Patch

Clearing flags on attachment: 182354

Committed r139876: <http://trac.webkit.org/changeset/139876>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.