| 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, mihaip, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | PC | ||||||
| OS: | OS X 10.5 | ||||||
| Attachments: |
|
||||||
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.
Created attachment 89734 [details]
Patch
Comment on attachment 89734 [details]
Patch
Layout tests seem to be happy with this change, including the one that had been disabled.
Comment on attachment 89734 [details]
Patch
OK
Comment on attachment 89734 [details] Patch Clearing flags on attachment: 89734 Committed r84055: <http://trac.webkit.org/changeset/84055> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/84055 might have broken WinCairo Debug (Build) |
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.