Bug 104525

Created attachment 178505 [details] Patch I only reproduced the problem randomly so I couldn't get a layout test to cover this.

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

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.

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.

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

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

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

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).

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.

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.

Created attachment 182354 [details] Patch Fixes the source of the problem and adds an ASSERT in Document::takeDOMWindowFrom.

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.

(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.

(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.

(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?

(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.

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.

(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.

Comment on attachment 182354 [details] Patch Clearing flags on attachment: 182354 Committed r139876: <http://trac.webkit.org/changeset/139876>

All reviewed patches have been landed. Closing bug.