Bug 38928

Created attachment 55738 [details] Reduction with description of the problem.

Sears.com scripts have bizarre Safari specific code paths that do this. The anchor element they create within their onbeforeunload handler and then dispatch a click to actually activates the policy delegate to consult about the navigation. When the policy delegate says "use", we call onbeforeunload again, which does the same thing with the anchor, consulting the policy delegate, etc etc etc. All the way till we blow out the stack and javascript aborts. Here's a cut of a single iteration of the stack blowing out: #0 0x1026c550e in WebCore::FrameLoader::continueLoadAfterNavigationPolicy at FrameLoader.cpp:3425 #1 0x1026c561c in WebCore::FrameLoader::callContinueLoadAfterNavigationPolicy at FrameLoader.cpp:3366 #2 0x102b6e063 in WebCore::PolicyCallback::call at PolicyCallback.cpp:101 #3 0x102b6ebb3 in WebCore::PolicyChecker::continueAfterNavigationPolicy at PolicyChecker.cpp:160 #4 0x101e3b999 in WebFrameLoaderClient::receivedPolicyDecison at WebFrameLoaderClient.mm:1271 #5 0x101e3ba2e in -[WebFramePolicyListener receivedPolicyDecision:] at WebFrameLoaderClient.mm:1864 #6 0x101e38066 in -[WebFramePolicyListener use] at WebFrameLoaderClient.mm:1879 #7 Browser policy delegate #8 Browser policy delegate #9 Browser policy delegate #10 0x7fff88026d8c in __invoking___ #11 0x7fff88026c5d in -[NSInvocation invoke] #12 0x7fff88042a71 in -[NSInvocation invokeWithTarget:] #13 0x101ed9f3e in -[_WebSafeForwarder forwardInvocation:] at WebView.mm:2467 #14 0x7fff88023dac in ___forwarding___ #15 0x7fff8801fe88 in __forwarding_prep_0___ #16 0x101e3cc30 in WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction at WebFrameLoaderClient.mm:750 #17 0x102b6f130 in WebCore::PolicyChecker::checkNavigationPolicy at PolicyChecker.cpp:88 #18 0x1026c5a34 in WebCore::FrameLoader::loadWithDocumentLoader at FrameLoader.cpp:1992 #19 0x1026c680a in WebCore::FrameLoader::loadWithNavigationAction at FrameLoader.cpp:1916 #20 0x1026c7c9e in WebCore::FrameLoader::loadURL at FrameLoader.cpp:1859 #21 0x1026c819d in WebCore::FrameLoader::loadFrameRequest at FrameLoader.cpp:1795 #22 0x1026c8545 in WebCore::FrameLoader::urlSelected at FrameLoader.cpp:361 #23 0x10273842d in WebCore::HTMLAnchorElement::defaultEventHandler at HTMLAnchorElement.cpp:199 #24 0x102b193a9 in WebCore::Node::dispatchGenericEvent at Node.cpp:2683 #25 0x102b19607 in WebCore::Node::dispatchEvent at Node.cpp:2567 #26 0x102678278 in WebCore::EventTarget::dispatchEvent at EventTarget.cpp:268 #27 0x102974c23 in WebCore::jsNodePrototypeFunctionDispatchEvent at JSNode.cpp:664 #28 0x33828e4001b4 in ?? #29 0x1017acca4 in JSC::JITCode::execute at JITCode.h:77 #30 0x1017974f0 in JSC::Interpreter::execute at Interpreter.cpp:758 #31 0x1017f2298 in JSC::JSFunction::call at JSFunction.cpp:139 #32 0x10173f45b in JSC::call at CallData.cpp:39 #33 0x1028e8392 in WebCore::JSEventListener::handleEvent at JSEventListener.cpp:116 #34 0x10267818a in WebCore::EventTarget::fireEventListeners at EventTarget.cpp:329 #35 0x102678877 in WebCore::EventTarget::fireEventListeners at EventTarget.cpp:290 #36 0x102630f44 in WebCore::DOMWindow::dispatchEvent at DOMWindow.cpp:1444 #37 0x1026ad413 in WebCore::Frame::shouldClose at Frame.cpp:1684 #38 0x1026c52ff in WebCore::FrameLoader::continueLoadAfterNavigationPolicy at FrameLoader.cpp:3383

When the stack blows out and javascript aborts, the FrameLoader is left in a bizarre state where it thinks it is the middle of FrameLoadStateProvisional, but doesn't have a provisional documentloader. This allows for the later crash, but also makes the frame un-navigable in the meantime.

<rdar://problem/7965182>

The most obvious and glaring problem here is that we re-enter the onbeforeunload event. Running the same test in Firefox, it's clear they don't re-enter. Modifying it for IE, they seem to enter it twice (once for the original navigation, once for the first dispatched click) but then they navigate to the first site as expected. I plan to protect the beforeunload event the same way we protect against unload re-entrancy, with a simple wrapper flag. Another problem is what happens to the FrameLoader when onbeforeunload aborts (as JSCore is actually aborting the javascript execution when the stack reaches its limit). I plan to at least add an ASSERT in ::activeDocumentLoader to catch the case that should never occur, and might explore the javascript-aborts aspect further, as well.

BTW, this isn't a (recent) regression. Happens in ToT but also in Safari 4.

Debugging a bit, it's not immediately clear why the aborted javascript due to the excessive stack results in the FrameLoadState and m_provisionalDocumentLoader getting out of sync. I'm going to focus on preventing the recursion in the onbeforeunload event handler and adding the ASSERT, then might revisit the out-of-syncness later.

Created attachment 55886 [details] Fix + layout test Guarding beforeunload with the same flag as we do pagehide and unload makes sense. But renaming the flag also seemed prudent.

Comment on attachment 55886 [details] Fix + layout test Instead of making setPageDismissalEventBeingDispatched public, could we instead refactor part of the Frame::shouldClose function so it can go inside FrameLoader?

Comment on attachment 55886 [details] Fix + layout test Lets land the renaming half of the patch first, separately. Sound OK?

(In reply to comment #10) > (From update of attachment 55886 [details]) > Lets land the renaming half of the patch first, separately. Sound OK? Sounds fine. r+ on doing that?

(In reply to comment #9) > (From update of attachment 55886 [details]) > Instead of making setPageDismissalEventBeingDispatched public, could we instead refactor part of the Frame::shouldClose function so it can go inside FrameLoader? Frame::shouldClose has a direct API/SPI equivalent in WebKit on WebFrame. I'm not saying it *couldn't* be done, but it might be much messier than one might think at a glance. Would you like me to pursue that?

Created attachment 55995 [details] Rename m_unloadEventBeingDispatched to m_pageDismissalEventBeingDispatched

Split off the rename, per Darin's request.

Rename landed in http://trac.webkit.org/changeset/59374

Not too different from the original patch, I landed http://trac.webkit.org/changeset/59384 with an in-person review from Darin.

http://trac.webkit.org/changeset/59384 might have broken Qt Linux Release

(In reply to comment #17) > http://trac.webkit.org/changeset/59384 might have broken Qt Linux Release I don't see a broken build on Qt Linux Release, nor did this automated message give me a link to something that shows the alleged breakage.

BTW, I know I caused a failure on the Tiger bot and am getting new results ready for it as I type.

http://trac.webkit.org/changeset/59389 should fix the Tiger layouttests

> I don't see a broken build on Qt Linux Release, nor did this automated message give me a link to something that shows the alleged breakage. http://build.webkit.org/waterfall?show=Qt%20Linux%20Release http://build.webkit.org/results/Qt%20Linux%20Release/r59384%20(11726)/results.html Tests that timed out: fast/loader/recursive-before-unload-crash.html ... which is a test added by your patch...

(In reply to comment #21) > > I don't see a broken build on Qt Linux Release, nor did this automated message give me a link to something that shows the alleged breakage. > > http://build.webkit.org/waterfall?show=Qt%20Linux%20Release > http://build.webkit.org/results/Qt%20Linux%20Release/r59384%20(11726)/results.html > > Tests that timed out: > > fast/loader/recursive-before-unload-crash.html > > ... which is a test added by your patch... Someone else pointed that out to me. Did the waterfall "View Results" links used to link *DIRECTLY* to results.html? I seem to recall not needing to click results.html in the past. Since I was giving a directory listing and not a page that actually described results, I clicked through to fast/loader and saw nothing, and didn't know what the complaint was. Could the error message posted in bugzilla be... more useful? What it told me: http://trac.webkit.org/changeset/59384 might have broken Qt Linux Release When I read that, I first thought "Oh, I broke the build," and was actually confused when I saw the build was fine. What it COULD have told me: http://trac.webkit.org/changeset/59384 might have broken Qt Linux Release layout tests. Refer to http://build.webkit.org/results/Qt%20Linux%20Release/r59384%20(11726)/results.html to see what I'm talking about.

Looking at the diffs for GTK, it's clear that their DRT isn't ready for these types of resource load callback dumping. I'll skip it there. And the Qt hang is something I'm not equipped to explore, and the tool isn't giving any sort of info like a sample that might let me explore blindly, so I'll skip that also.

Skipped for both Qt and GTK in r59392

http://trac.webkit.org/changeset/59389 might have broken Qt Windows 32-bit Release The following changes are on the blame list: http://trac.webkit.org/changeset/59387 http://trac.webkit.org/changeset/59388 http://trac.webkit.org/changeset/59389

fast/loader/recursive-before-unload.html failed again on Tiger: --- /Volumes/Data/WebKit-BuildSlave/tiger-intel-release/build/layout-test-results/fast/loader/recursive-before-unload-crash-expected.txt 2010-05-13 17:13:21.000000000 -0700 +++ /Volumes/Data/WebKit-BuildSlave/tiger-intel-release/build/layout-test-results/fast/loader/recursive-before-unload-crash-actual.txt 2010-05-13 17:13:21.000000000 -0700 @@ -6,8 +6,8 @@ main frame - didStartProvisionalLoadForFrame ALERT: Adding iframe frame "<!--framePath //<!--frame0-->-->" - didStartProvisionalLoadForFrame -frame "<!--framePath //<!--frame0-->-->" - didFailProvisionalLoadWithError main frame - didCancelClientRedirectForFrame +frame "<!--framePath //<!--frame0-->-->" - didFailProvisionalLoadWithError main frame - didFailProvisionalLoadWithError This test demonstrates a problem with our handling of the beforeunload event. If a script manages to try and navigate the frame from beforeunload - when a navigation is already pending - we end up blowing out the stack by recursively consulting the policy delegate then running onbeforeunload repeatedly. Looks like the test itself is racy (at least on Tiger), those two lines have flipped around a few times since the test was added.

I think I'm going to nuke the frame load callbacks from the test. I thought they were a nice bonus, but if they're unreliable then it's not worth the hassle - the alert logging and lack of crashing is the important thing.

(In reply to comment #23) > Looking at the diffs for GTK, it's clear that their DRT isn't ready for these types of resource load callback dumping. I'll skip it there. > > And the Qt hang is something I'm not equipped to explore, and the tool isn't giving any sort of info like a sample that might let me explore blindly, so I'll skip that also. I think the timeout is related to the issue tracked here: https://bugs.webkit.org/show_bug.cgi?id=39160

Have seen fast/loader/recursive-before-unload-crash.html fail a couple times on the leopard commit bot. Might be flaky.

This test is in fact very flaky. See bug 43840.