Bug 98345 – [WK2] fast/parser/document-open-in-unload.html makes the following test crash

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Csaba Osztrogonác]

fast/parser/document-open-in-unload.html introduced in https://trac.webkit.org/changeset/130313
and the following test - fast/parser/document-write-ignores-later-network-bytes.html - started
to crash on the Qt WK2 bot:

worker/0 fast/parser/document-write-ignores-later-network-bytes.html crashed, (stderr lines):
0x7f5763d0d378 /home/oszi/WebKit/WebKitBuild/Release/lib/libWTF.so.1(+0x16378) [0x7f5763d0d378]
0x7f575ba93230 /lib/libc.so.6(+0x32230) [0x7f575ba93230]
0x7f576631b1c4 /home/oszi/WebKit/WebKitBuild/Release/lib/libWebCore.so.1(WebCore::FrameLoader::commitProvisionalLoad()+0x1f4) [0x7f576631b1c4]
0x7f57662fb2aa /home/oszi/WebKit/WebKitBuild/Release/lib/libWebCore.so.1(WebCore::DocumentLoader::commitLoad(char const*, int)+0x3a) [0x7f57662fb2aa]
0x7f5766343931 /home/oszi/WebKit/WebKitBuild/Release/lib/libWebCore.so.1(WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool)+0x41) [0x7f5766343931]
0x7f576632f105 /home/oszi/WebKit/WebKitBuild/Release/lib/libWebCore.so.1(WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool)+0x65) [0x7f576632f105]
0x7f57663432e5 /home/oszi/WebKit/WebKitBuild/Release/lib/libWebCore.so.1(WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int)+0xb5) [0x7f57663432e5]
0x7f57666f0311 /home/oszi/WebKit/WebKitBuild/Release/lib/libWebCore.so.1(WebCore::QNetworkReplyHandler::forwardData()+0x61) [0x7f57666f0311]
0x7f57666f0619 /home/oszi/WebKit/WebKitBuild/Release/lib/libWebCore.so.1(WebCore::QNetworkReplyHandlerCallQueue::flush()+0x59) [0x7f57666f0619]
0x7f57666f1033 /home/oszi/WebKit/WebKitBuild/Release/lib/libWebCore.so.1(WebCore::QNetworkReplyWrapper::emitMetaDataChanged()+0xd3) [0x7f57666f1033]
0x7f57666f131b /home/oszi/WebKit/WebKitBuild/Release/lib/libWebCore.so.1(WebCore::QNetworkReplyWrapper::receiveSniffedMIMEType()+0x6b) [0x7f57666f131b]
0x7f575ce63058 /usr/local/Trolltech/Qt5/Qt-5.0.0-beta1/lib/libQtCore.so.5(QMetaObject::activate(QObject*, int, int, void**)+0x328) [0x7f575ce63058]


You can easily reproduce the crash with the following command:
$run-webkit-tests -2 fast/parser/document-open-in-unload.html fast/parser/document-write-ignores-later-network-bytes.html

[Csaba Osztrogonác]

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

[Adam Barth]

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

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

and on Mac too ...

[Mikhail Pozdnyakov]

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.

[Mikhail Pozdnyakov]

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.

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

<rdar://problem/12474923>

[Csaba Osztrogonác]

the bug is still valid on Qt WK2.

[David Kilzer (:ddkilzer)]

Also affects WK2 on iOS Simulator.

[David Kilzer (:ddkilzer)]

Possible GTK dupe:

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

[David Kilzer (:ddkilzer)]

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.

[Martin Robinson]

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

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 343585 [details]
Patch

[Michael Catanzaro]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 343701 [details]
Patch

[Fujii Hironori]

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

[Ryosuke Niwa]

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.

[Fujii Hironori]

Created attachment 343899 [details]
Patch

[David Kilzer (:ddkilzer)]

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.

[Fujii Hironori]

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

[Darin Adler]

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

[Fujii Hironori]

Created attachment 344067 [details]
Patch

[Fujii Hironori]

Comment on attachment 344067 [details]
Patch

Clearing flags on attachment: 344067

Committed r233414: <https://trac.webkit.org/changeset/233414>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.