Bug 44969 – Track the right gesture state during the asynchronous form submission.

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

[Johnny(Jianning) Ding]

The following html can bypass chromium's popup blocker (You can also run the attached test case)
<form id="frm1" action="http://www.google.com" target="_blank"> 
<input type="submit" value="Submit"/> 
</form> 
<script> 
var form1 = document.getElementById('frm1');
form1.submit();
</script>

It's because WebKit can not provide correct gesture state during the asynchronous form submission.

In WebKit, when submitting a form, the FrameLoader::submitForm can get right gesture state to allow or disallow the popup form submission according to setting's m_javaScriptCanOpenWindowsAutomatically (See DOMWindow::allowPopUp) before scheduling the submission.
However, in chromium, we keep setting's m_javaScriptCanOpenWindowsAutomatically always true to always schedule any form submissions to allow the embedder (aka browser process) to make the decision by leveraging the chromium's popup blocker feature.

In WebKit, the actual form submission is asynchronous and executed in ScheduledFormSubmission::fire, so during the real executing time, we lost the right context to get the right correct gesture state.
In chromium, the RenderView::createView tried to call WebFrame::isProcessingUserGesture to get the gesture state when doing the real form submission, since we can not get the active(entered) frame in that time, the ScriptController::processingUserGesture returns true, so chromium thinks the popup submission is gesture-initiated, that is why the above test code bypasses chromium's popup blocker.

To fix this issue, we need to express the following logic:
if a submission was scheduled in response to a user gesture, set UserGestureIndicator's state to DefinitelyProcessingUserGesture when the submission timer fires; otherwise, set the state to DefinitelyNotProcessingUserGesture during the submission timer fires. (The solution actually is from Andy's comments in bug 34541).

The patch will look like

ScheduledFormSubmission::ScheduledFormSubmission(PassRefPtr<FormSubmission> submission, bool lockBackForwardList, bool duringLoad, bool wasUserGesture)
        : ScheduledNavigation(0, submission->lockHistory(), lockBackForwardList, duringLoad, true)
        , m_submission(submission)
        , m_haveToldClient(false)
        , m_wasUserGesture(wasUserGesture)

virtual void ScheduledFormSubmission::fire(Frame* frame)
{
     UserGestureIndicator gestureIndicator(m_wasUserGesture ? DefinitelyProcessingUserGesture : DefinitelyNotProcessingUserGesture);
     ...

@Adam, what do you think?

[Johnny(Jianning) Ding]

Created attachment 66081 [details]
test case

[Adam Barth]

Yes.  That's the right solution.  We do something similar is a couple other cases where we need to track the user gesture state across async operations.  That's actually why we created UserGestureIndicator in the first place.

[Johnny(Jianning) Ding]

(In reply to comment #2)
> Yes.  That's the right solution.  We do something similar is a couple other cases where we need to track the user gesture state across async operations.  That's actually why we created UserGestureIndicator in the first place.

Thanks for super fast response:) Will give a patch after getting up.

[Chris Evans]

@jnd, @abarth: we in fact had exactly this logic for exactly this case :) Adam and I added it a while ago in this CL:
http://trac.webkit.org/changeset/57313/trunk/WebCore/loader/RedirectScheduler.cpp
(plus follow-up fix):
http://trac.webkit.org/changeset/59462/trunk/WebCore/loader/RedirectScheduler.cpp

However, this subsequent CL seems to have backed part of our changes out:
http://trac.webkit.org/changeset/65340/trunk/WebCore/loader/RedirectScheduler.cpp

Adding Mihai - do you perhaps remember why the UserGestureIndicator part changed in 65340?

[Mihai Parparita]

(In reply to comment #4)
> However, this subsequent CL seems to have backed part of our changes out:
> http://trac.webkit.org/changeset/65340/trunk/WebCore/loader/RedirectScheduler.cpp
> 
> Adding Mihai - do you perhaps remember why the UserGestureIndicator part changed in 65340?

It looked like an unused local variable. Now I see that simply creating a UserGestureIndicator instance toggles a static member. I can put it back (perhaps with a comment).

[Mihai Parparita]

(In reply to comment #5)
> It looked like an unused local variable. Now I see that simply creating a UserGestureIndicator instance toggles a static member. I can put it back (perhaps with a comment).

It's also weird that fast/events/popup-blocked-to-post-blank.html still passes (based on http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fevents%2Fpopup-blocked-to-post-blank.html) even with my change.

[Chris Evans]

@mihaip: unfortunately, the Chromium and Safari pop-up blockers work in totally different ways. Both were broken and I fixed both; but the test only tests the Safari blocker. I don't believe it's possible to test the Chromium blocker with a LayoutTest.
I take blame here for not adding a Chromium UI test.
Johnny, how bribe-able are you to add such a thing? Pop-up blocking keeps regressing; a Chromium UI test that checks all of the test cases we've been dealing with recently would be invaluable.

[Johnny(Jianning) Ding]

(In reply to comment #7)
> @mihaip: unfortunately, the Chromium and Safari pop-up blockers work in totally different ways. Both were broken and I fixed both; but the test only tests the Safari blocker. I don't believe it's possible to test the Chromium blocker with a LayoutTest.
> I take blame here for not adding a Chromium UI test.
> Johnny, how bribe-able are you to add such a thing? Pop-up blocking keeps regressing; a Chromium UI test that checks all of the test cases we've been dealing with recently would be invaluable.

Hi Chris,
Yes, I am sorry I didn't find your original patch.
and sure thing, I will add a chrome UI test to address this bug, also I will bring your fix back into WebKit with comments.
Thanks!

[Johnny(Jianning) Ding]

Created attachment 66211 [details]
fix patch

Hi Adam, Chris,

This patch is to bring the Chris's original fix (in r57313 and r594620) back.
Since WebKit and Chromium implement popup blocker in different ways, the test fast/events/popup-blocked-to-post-blank.html can not cover this bug in chromium.
I will add a browser_test to address this bug in chromium once this patch is landed and taken by chromium trunk.

Thanks!

[Adam Barth]

Comment on attachment 66211 [details]
fix patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66211&action=prettypatch

Mostly just questions.

> WebCore/ChangeLog:9
> +        fast/events/popup-blocked-to-post-blank.html can cover the test in WebKit.
> +        A UI test will be added in chromium to address chromium's bug.
Is fast/events/popup-blocked-to-post-blank.html test currently failing?  If so, do we need to remove it from the Skipped list?  If not, then it doesn't seem to cover this change.

> WebCore/loader/RedirectScheduler.cpp:188
> +        // Please do not delete the following local variable gestureIndicator definition.
> +        // The variable gestureIndicator toggles a static gesture state to let WebKit correctly
> +        // track the user gesture state across async operations.
This comment is not needed.  Instead, we should have a test that breaks if you remove this line.

> WebCore/loader/RedirectScheduler.cpp:324
>      bool lockBackForwardList = mustLockBackForwardList(m_frame, UserGestureIndicator::processingUserGesture()) || (submission->state()->formSubmissionTrigger() == SubmittedByJavaScript && m_frame->tree()->parent());
>  
> -    schedule(adoptPtr(new ScheduledFormSubmission(submission, lockBackForwardList, duringLoad)));
> +    schedule(adoptPtr(new ScheduledFormSubmission(submission, lockBackForwardList, duringLoad, m_frame->loader()->isProcessingUserGesture())));
Why do we use  m_frame->loader()->isProcessingUserGesture() here but UserGestureIndicator::processingUserGesture() two lines above?

[Johnny(Jianning) Ding]

(In reply to comment #10)
Thanks, Adam! Please see my inline comments.

> Is fast/events/popup-blocked-to-post-blank.html test currently failing?  If so, do we need to remove it from the Skipped list?  If not, then it doesn't seem to cover this change.
At now fast/events/popup-blocked-to-post-blank.html is not failed in WebKit layout test, and I think it does cover this issue, which is to check whether the form submission issued popup window can bypass popup blocker or not. 
AFAIK this test is not in any skip lists, we can still keep it. (Please correct me if I was wrong).
But since Chromium's popup feature is implemented in another way (judgement in  embedder), this test can not cover the same issue in Chromium. I will add a Chromium test once this patch is landed and taken by Chromium.
Does that make sense?


> This comment is not needed. Instead, we should have a test that breaks if you remove this line.
Yes, you are right!
The reason to add this comment is because the line looks like useless if someone is not familiar with the context. I want to remind him/her.
However removing it does not break WebKit, but does break Chromium. so a chromium test case to address this issue is definitely necessary. Like I said in above. I will add that test in chromium.
Of course, I will drop this comment in new patch.

> Why do we use  m_frame->loader()->isProcessingUserGesture() here but UserGestureIndicator::processingUserGesture() two lines above?

The reason to use m_frame->loader()->isProcessingUserGesture() is because that call can cover all scenarios of user gesture, but UserGestureIndicator::processingUserGesture() does not.
FrameLoader::isProcessingUserGesture() can handle right gesture state in the scenario of form submission when javascript is disabled, can handle right gesture state in the scenario of form submission via href="javascript:form.submit()", can handle right gesture state in the scenario of form submission inside gesture event or non-gesture event (See Event::fromUserGesture).
Why the code in two lines above used serGestureIndicator::processingUserGesture(). To be honest, I don't know, but I 'd like to change it to use FrameLoader::isProcessingUserGesture().
Any comments?

[Johnny(Jianning) Ding]

Created attachment 66365 [details]
new patch

[Adam Barth]

Comment on attachment 66365 [details]
new patch

We need a test for this patch.  Honestly, the test is more important than the fix in cases like this.  Having a test in Chromium-land is nice, but we should have a LayoutTest too, otherwise we're only going to discover regressions after a merge, which is bad news bears.  This is common code to all WebKit ports.  We should be able to test it with a LayoutTest.

[Chris Evans]

@abarth - let us know if you have a way to actually test this in WebKit.
The Chromium port actually runs with WebKit's notification of a pop-up blocker _disabled_. Chromium instead relies on WebKit's UserGestureIndicator API returning the correct results at various times, such as in the asynchronous form submission context.

(The original patch for this did add a LayoutTest for this with WebKit's pop-up blocker enabled).

[Adam Barth]

Comment on attachment 66365 [details]
new patch

I see.  Ok, we'll this is likely to cause us more pain in the future...

[Chris Evans]

Thanks, Adam.
Johnny - a good Chromium test in the area of pop-up blocking would go some way to lessening any future pain...

[Johnny(Jianning) Ding]

(In reply to comment #16)
> Thanks, Adam.
> Johnny - a good Chromium test in the area of pop-up blocking would go some way to lessening any future pain...
Thanks Adam, Chris!
The chromium test will be added after landing this patch. I will send the cl to you tomorrow.(ooo today)

[Abhishek Arya]

Added commit queue flag so that this gets committed.

[WebKit Commit Bot]

Comment on attachment 66365 [details]
new patch

Clearing flags on attachment: 66365

Committed r66742: <http://trac.webkit.org/changeset/66742>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66742 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/66742
http://trac.webkit.org/changeset/66743