Bug 44079

Summary: [Chromium] fast/events/popup-allowed-from-gesture-initiated-form-submit.html causing debug ASSERT
Product: WebKit Reporter: Nate Chapin <japhet>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, creis, eric, fishd, mihai, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch none

Description Nate Chapin 2010-08-16 15:44:27 PDT
ASSERTION FAILED: m_expectedClientRedirectDest.protocolIs("javascript") || m_expectedClientRedirectDest == url
(third_party/WebKit/WebKit/chromium/src/FrameLoaderClientImpl.cpp:701 virtual void WebKit::FrameLoaderClientImpl::dispatchDidStartProvisionalLoad())

This test is causing the test that runs immediately after it to hit the above ASSERT.  I suspect r65381, but I'm not yet certain.
Comment 1 Charles Reis 2011-04-14 21:55:51 PDT
I ran into this ASSERT in another context, which I think sheds some light on it.

FrameLoaderClientImpl keeps track of whether a redirect is in progress using m_expectedClientRedirect{Src,Dest}.  If so, this ASSERT is making sure the url of the next navigation matches the expectation.

However, there's at least two cases where this isn't true.  In my case, a navigation can be aborted if the embedder returns PolicyIgnore.  That doesn't clean up this m_expectedClientRedirectSrc field, so we fail the ASSERT.

In the case of this layout test, the form submission targets "_blank", causing it to open in a new window.  However, at the time of FrameLoader::submitForm, the new frame doesn't exist yet.  FrameLoader instead calls scheduleFormSubmission on the current frame, even though the navigation won't occur there.  As a result, we end up expecting the redirect in the wrong frame.  Any future navigations in the form's frame will fail the ASSERT.

There may be more cases where a redirect doesn't get canceled through the expected approach, so I'm inclined to think the ASSERT is broken.  I've put together a patch that changes it into an "if" instead of an assertion, which should make sure we do the right thing in these corner cases.
Comment 2 Charles Reis 2011-04-14 21:58:30 PDT
Created attachment 89734 [details]
Patch
Comment 3 Charles Reis 2011-04-15 11:25:51 PDT
Comment on attachment 89734 [details]
Patch

Layout tests seem to be happy with this change, including the one that had been disabled.
Comment 4 Darin Fisher (:fishd, Google) 2011-04-15 14:32:04 PDT
Comment on attachment 89734 [details]
Patch

OK
Comment 5 WebKit Commit Bot 2011-04-15 17:01:35 PDT
Comment on attachment 89734 [details]
Patch

Clearing flags on attachment: 89734

Committed r84055: <http://trac.webkit.org/changeset/84055>
Comment 6 WebKit Commit Bot 2011-04-15 17:01:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 WebKit Review Bot 2011-04-15 18:22:19 PDT
http://trac.webkit.org/changeset/84055 might have broken WinCairo Debug (Build)