Bug 98345

I skipped it to paint the bot green - r130367 Please unskip it with the proper fix.

Sounds like this test uncovered an existing bug in WebKit2. I don't plan to work on fixing that bug.

I checked other platforms and it isn't Qt specific bug, but WK2. It crashes on GTK and EFL too.

and on Mac too ...

document.open results in nulling out m_documentLoader in fast/parser/document-open-in-unload.html and then fast/parser/document-write-ignores-later-network-bytes.html ends up with following code if (m_loadType == FrameLoadTypeStandard && m_documentLoader->isClientRedirect()) and crashes.

fast/parser/document-open-in-unload.html fails itself actually ./WebKitBuild/Debug/bin/WebKitTestRunner ./LayoutTests/fast/parser/document-open-in-unload.html Content-Type: text/plain This test passes if it doesn't crash. #EOF #EOF #EOF 1 0x7f728ac77e33 2 0x7f728cec74c0 3 0x7f728a04d072 WebCore::DocumentLoader::isClientRedirect() const 4 0x7f728a045459 WebCore::FrameLoader::commitProvisionalLoad() 5 0x7f728a02026f WebCore::DocumentLoader::commitIfReady() 6 0x7f728a0203d4 WebCore::DocumentLoader::commitLoad(char const*, int) 7 0x7f728a020919 WebCore::DocumentLoader::receivedData(char const*, int) 8 0x7f728a05bca5 WebCore::MainResourceLoader::addData(char const*, int, bool) 9 0x7f728a06ee3a WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) 10 0x7f728a05d15d WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool) 11 0x7f728a06f6ea WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) but WTR "thinks" that the next test is already running. Here is what actually happens: 1) Frame loader is loading child frame in FrameLoader::loadURLIntoChildFrame m_loadType becomes equal to FrameLoadTypeRedirectWithLockedBackForwardList. 2) Then document.open() is invoked from the 'onunload' handler in the test code. This leads to following call chain: 1 0x7fdec7cc50c3 WebCore::FrameLoader::checkLoadCompleteForThisFrame() 2 0x7fdec7cc5be0 WebCore::FrameLoader::checkLoadComplete() 3 0x7fdec7cc270f WebCore::FrameLoader::stopForUserCancel(bool) 4 0x7fdecb658d22 WebKit::WebPage::stopLoading() 5 0x7fdecb5d0535 WKBundlePageStopLoading 6 0x7fde7c0b3129 WTR::InjectedBundlePage::stopLoading() 7 0x7fde7c0adf76 WTR::InjectedBundle::done() 8 0x7fde7c0b5b5b WTR::InjectedBundlePage::dump() 9 0x7fde7c0b5ce7 WTR::InjectedBundlePage::didFinishLoadForFrame(OpaqueWKBundleFrame const*) 10 0x7fde7c0b3c53 WTR::InjectedBundlePage::didFinishLoadForFrame(OpaqueWKBundlePage const*, OpaqueWKBundleFrame const*, void const**, void const*) 11 0x7fdecb5c6d13 WebKit::InjectedBundlePageLoaderClient::didFinishLoadForFrame(WebKit::WebPage*, WebKit::WebFrame*, WTF::RefPtr<WebKit::APIObject>&) 12 0x7fdecb62b7b4 WebKit::WebFrameLoaderClient::dispatchDidFinishLoad() 13 0x7fdec7cc4fde WebCore::FrameLoader::checkLoadCompleteForThisFrame() 14 0x7fdec7cc5be0 WebCore::FrameLoader::checkLoadComplete() 15 0x7fdec7cc7081 WebCore::FrameLoader::receivedMainResourceError(WebCore::ResourceError const&) 16 0x7fdec7c9df27 WebCore::DocumentLoader::mainReceivedError(WebCore::ResourceError const&) 17 0x7fdec7cd9a07 WebCore::MainResourceLoader::didCancel(WebCore::ResourceError const&) 18 0x7fdec7ced456 WebCore::ResourceLoader::cancel(WebCore::ResourceError const&) 19 0x7fdec7ced21f WebCore::ResourceLoader::cancel() 20 0x7fdec7c9e100 WebCore::DocumentLoader::stopLoading() 21 0x7fdec7cc263a WebCore::FrameLoader::stopAllLoaders(WebCore::ClearProvisionalItemPolicy) 22 0x7fdec7cc5e2e WebCore::FrameLoader::detachFromParent() 23 0x7fdec7cc5db1 WebCore::FrameLoader::frameDetached() WebCore::FrameLoader::checkLoadCompleteForThisFrame here makes m_loadType becomes equal to FrameLoadTypeStandard, after WTR injected bundle reacted on WebFrameLoaderClient::dispatchDidFinishLoad and invoked WebPage::stopLoading(). 3) Then we have crash at if (m_loadType == FrameLoadTypeStandard && m_documentLoader->isClientRedirect()) 3 0x7fc0b82f0072 WebCore::DocumentLoader::isClientRedirect() const 4 0x7fc0b82e8459 WebCore::FrameLoader::commitProvisionalLoad() 5 0x7fc0b82c326f WebCore::DocumentLoader::commitIfReady() 6 0x7fc0b82c33d4 WebCore::DocumentLoader::commitLoad(char const*, int) 7 0x7fc0b82c3919 WebCore::DocumentLoader::receivedData(char const*, int) 8 0x7fc0b82feca5 WebCore::MainResourceLoader::addData(char const*, int, bool) 9 0x7fc0b8311e3a WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) 10 0x7fc0b830015d WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool) 11 0x7fc0b83126ea WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) The reason why it does not fail for WK 1 ports is that they do not invoke WebCore::FrameLoader::stopForUserCancel() as a reaction on WebFrameLoaderClient::dispatchDidFinishLoad(), so the first condition in the 'if' statement is false for them and fatal m_documentLoader->isClientRedirect() is not invoked. However, the main question is why WebCore::FrameLoader::commitProvisionalLoad() is still invoked even after FrameLoader was said to stop. That's confusing WTR and makes it think that that the following test is failing.

*** Bug 98960 has been marked as a duplicate of this bug. ***

<rdar://problem/12474923>

the bug is still valid on Qt WK2.

Also affects WK2 on iOS Simulator.

Possible GTK dupe: Bug 134996: [GTKl] Test fast/parser/document-open-in-unload.html is flaky (crashes sometimes)

Moved Skip expectation to LayoutTests/platforms/wk2/TestExpectations from mac-wk2: <http://trac.webkit.org/changeset/179806> The EFL and GTK expectations cover both WK1 and WK2, so I left them.

*** Bug 134996 has been marked as a duplicate of this bug. ***

Created attachment 343575 [details] crash log Several years has been passed. But, still crash happens. I tested with GTK port, debug build, trunk@233142. > ./Tools/Scripts/run-webkit-tests --gtk --debug --child-processes=1 -v fast/parser/document-open-in-unload.html css1/basic/comments.html --iterations=2 > [1/4] css1/basic/comments.html passed > [2/4] fast/parser/document-open-in-unload.html passed > [3/4] css1/basic/comments.html failed unexpectedly (WebKitWebProcess crashed [pid=20039]) > [4/4] fast/parser/document-open-in-unload.html passed I attached the crash log. crash happnes in the same code, WebCore::DocumentLoader::isClientRedirect. But, the call stack looks slightly different. I can obverse ResourceLoader::loadDataURL and a string "Hi" in the backtrace. This is the content of the child iframe's data URL.

Created attachment 343577 [details] gdb log I disabled JSC JIT to make gdb be able to retrieve callstack. > export JSC_useJIT=0 > export JSC_useDFGJIT=0 > export JSC_useRegExpJIT=0 > export JSC_useDOMJIT=0 Then, I put a breakpoint at WebCore::Document::open, and got two callstack, one is WebCore::Document::open, another is the crash. Both callstacks are exactly same under DocumentLoader::commitIfReady, because all things happens during the iframe's DocumentLoader::commitIfReady was called.

Created attachment 343579 [details] gdb log FrameLoader::m_documentLoader is cleared in FrameLoader::setDocumentLoader. So, I put breakpoints at FrameLoader::setDocumentLoader and Document::open. This callstack shows FrameLoader::m_documentLoader was cleared during FrameLoader::commitProvisionalLoad.

Created attachment 343584 [details] gdb log The reason this crash happens only in WebKitTestRunner not in MiniBrowser is m_loadType is changed to FrameLoadTypeStandard by invoking WKBundlePageStopLoading as mentioned in Mikhail's comment #6. I tried again by putting breakpoints at Document::open, FrameLoader::checkLoadCompleteForThisFrame and FrameLoader::setDocumentLoader. (In reply to Mikhail Pozdnyakov from comment #6) > However, the main question is why > WebCore::FrameLoader::commitProvisionalLoad() is still invoked > even after FrameLoader was said to stop. The answer to this Mikhail's question is FrameLoader::commitProvisionalLoad() is not invoked again, it isn't finished.

Created attachment 343585 [details] Patch

Comment on attachment 343585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343585&action=review > Source/WebCore/loader/FrameLoader.cpp:2008 > + && m_documentLoader This is not good, we can't have the DocumentLoader disappearing at random like this. :/ Certainly not without a warning comment. But the fact is that anything at all can happen after calling a function of m_client. This is similar to bug #182257. (CCs for that bug available on request.) I wonder what Chris thinks of this solution.

Comment on attachment 343585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343585&action=review Thank you for the review. >> Source/WebCore/loader/FrameLoader.cpp:2008 >> + && m_documentLoader > > This is not good, we can't have the DocumentLoader disappearing at random like this. :/ Certainly not without a warning comment. But the fact is that anything at all can happen after calling a function of m_client. This is similar to bug #182257. (CCs for that bug available on request.) > > I wonder what Chris thinks of this solution. There is already a null-check of m_documentLoader after calling transitionToCommitted. https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/loader/FrameLoader.cpp?rev=233176#L1956 I don't think so bad. I will add a comment.

Created attachment 343701 [details] Patch

(In reply to Fujii Hironori from comment #20) > There is already a null-check of m_documentLoader after calling > transitionToCommitted. > https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/loader/ > FrameLoader.cpp?rev=233176#L1956 This null-check and the test case fast/parser/document-open-in-unload.html have been added in Bug 98287.

Comment on attachment 343701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343701&action=review > Source/WebCore/loader/FrameLoader.cpp:2014 > + && m_documentLoader // m_documentLoader can become null by detaching the frame. I don't think this comment adds any value. Please remove. r=me with the comment removed.

Created attachment 343899 [details] Patch

Comment on attachment 343899 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343899&action=review > Source/WebCore/loader/FrameLoader.cpp:2015 > + if (m_loadType == FrameLoadType::Standard > + && m_documentLoader > + && m_documentLoader->isClientRedirect()) Nit: This can be one line again.

(In reply to David Kilzer (:ddkilzer) from comment #25) > Comment on attachment 343899 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=343899&action=review > > > Source/WebCore/loader/FrameLoader.cpp:2015 > > + if (m_loadType == FrameLoadType::Standard > > + && m_documentLoader > > + && m_documentLoader->isClientRedirect()) > > Nit: This can be one line again. Thank you for the feedback. I will fix it. The reason I split it into three lines even though I wanted to split into two lines is https://webkit.org/code-style-guidelines/#indentation-wrap-bool-op > Boolean expressions at the same nesting level that span multiple lines should have their operators on the left side of the line instead of the right side.

(In reply to Fujii Hironori from comment #26) > The reason I split it into three lines even though I wanted to split into > two lines is > https://webkit.org/code-style-guidelines/#indentation-wrap-bool-op > > > Boolean expressions at the same nesting level that span multiple lines should have their operators on the left side of the line instead of the right side. That guideline tells *where* to split when an expression like that is split into multiple lines. It does not tell us *when* or *whether* to split into multiple lines and when to just use one long line; often we choose not to split even if the line is long.

Created attachment 344067 [details] Patch

Comment on attachment 344067 [details] Patch Clearing flags on attachment: 344067 Committed r233414: <https://trac.webkit.org/changeset/233414>

All reviewed patches have been landed. Closing bug.