Bug 47817 – FrameLoader::IsProcessingUserGesture is true for JavaScript initiated downloads after click navigation to webpage

• Component: Page Loading
• Status: RESOLVED FIXED

[Pierre-Antoine LaFayette]

Created attachment 71028 [details]
links.html, links to wait.html

If I click on a link that navigates to a page which uses setTimeout to initiate a download (e.g. setTimeout("window.location = 'file.exe'", 5000)), FrameLoader::IsProcessingUserGesture is still true when loading of file.exe begins.

It seems that the UserGestureState is set to PossiblyProcessingUserGesture and the !activeFrame conditional gets hit.

bool ScriptController::processingUserGesture()
{
    Frame* activeFrame = V8Proxy::retrieveFrameForEnteredContext();
    // No script is running, so it is user-initiated unless the gesture stack
    // explicitly says it is not.
    if (!activeFrame)
        return UserGestureIndicator::getUserGestureState() != DefinitelyNotProcessingUserGesture;

[Pierre-Antoine LaFayette]

Created attachment 71029 [details]
wait.html, when loaded it will download setup.exe after 5 secs.

[Pierre-Antoine LaFayette]

This was observed in Chromium. RenderView::didStartProvisionalLoad sets its |navigation_gesture_| based on the results of FrameLoader::IsProcessingUserGesture.

[Adam Barth]

RenderView::didStartProvisionalLoad doesn't exist in svn.webkit.org.  Is there an issue that's possible to observe using code from svn.webkit.org?

[Johnny(Jianning) Ding]

(In reply to comment #3)
> RenderView::didStartProvisionalLoad doesn't exist in svn.webkit.org.  Is there an issue that's possible to observe using code from svn.webkit.org?

Hi Adam,  Pierre-Antoine was right, the WebFrameImpl::isProcessingUserGesture() did return true (met PossiblyProcessingUserGesture and !activeFrame conditions) when the page was redirected (triggered by setTimeout) to file.exe.

It was because we didn't explicit set the GestureIndicator to DefinitelyProcessingUserGesture/DefinitelyNotProcessingUserGesture acording to the m_wasUserGesture in ScheduledURLNavigation::fire. (We only passed the m_wasUserGesture to FrameLoader::changeLocation).

I will file a patch to fix this bug.

[Pierre-Antoine LaFayette]

I believe this issue is only reproducible because we are checking the state of the user gesture in the FrameLoader during the load. But as Johnny pointed out, the issue seems to still be within the WebKit code.

[Adam Barth]

> I will file a patch to fix this bug.

Great.  We'll need to find a way to write a test for the fix...

[Johnny(Jianning) Ding]

(In reply to comment #6)
> > I will file a patch to fix this bug.
> 
> Great.  We'll need to find a way to write a test for the fix...
That's pain because we can not write script o test the gesture status (since using script breaks the !activeFrame condition). 
For chrome side, a ui test may be a way, so far no idea about webkit test.

[Adam Barth]

> That's pain because we can not write script o test the gesture status (since using script breaks the !activeFrame condition). 
> For chrome side, a ui test may be a way, so far no idea about webkit test.

Yeah, I think a UI test is ok in this case...  I'm not sure how we could test it in DumpRenderTree...

[Alexey Proskuryakov]

Is the problem that we cannot simulate a click without JS on the stack?

[Adam Barth]

(In reply to comment #9)
> Is the problem that we cannot simulate a click without JS on the stack?

My understanding is the problem is that we can't read the state in question without JavaScript on the stack.  It's visible in the browser not as a web compat issue but as wrong behavior by the download manager.

[Johnny(Jianning) Ding]

(In reply to comment #8) 
> Yeah, I think a UI test is ok in this case...  I'm not sure how we could test it in DumpRenderTree...

I figured out a way to test it in DumpRenderTree on all ports.

We call LayoutController::dumpFrameLoadCallbacks() to dump the frame info when calling didStartProvisionalLoad logic. Please refer to (only lists mac/chromium ports)
1. https://svn.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp#L1290
2. https://svn.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/mac/FrameLoadDelegate.mm#L143

In the dump implementation, we output the gesture status of main frame.  Please refer to (only lists mac/chromium ports)
1. https://svn.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp#L841
2. https://svn.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/mac/FrameLoadDelegate.mm#L76

Since the way needs to change a few DumpRenderTree files, I'd like file another bug to address the test issue and only provide the code patch in here.
Adam, does it make sense to you?

[Adam Barth]

That's a good idea.  We'll want to land the test infrastructure before the code change, of course.  :)

[Johnny(Jianning) Ding]

(In reply to comment #12)
> That's a good idea.  We'll want to land the test infrastructure before the code change, of course.  :)

Thanks. Bug 47849 is filed to track the test infrastructure.

[Johnny(Jianning) Ding]

Just found Adam's r69924 (https://svn.webkit.org/changeset/69924) already fixed this issue, now all fire implementations set the right UserGestureIndicator based on m_wasUserGesture.

Adam, please close this bug. I will update the test infrastructure progress in bug 47849.

[Pierre-Antoine LaFayette]

Nice.

[Pierre-Antoine LaFayette]

Created attachment 71290 [details]
Patch

[Pierre-Antoine LaFayette]

Hi I'm reopening this bug because Adam's patch does not fix the case where a user initiated navigation does a meta refresh and triggers a download; which, as discussed, should not be considered a user initiated navigation. 

To me it would seem that we should have IsProcessingUserGesture only return true if we know for certain that the navigation was user initiated. 

Adam, can you please review my patch to assure that I'm not overlooking anything by this change.

Thanks.

[Johnny(Jianning) Ding]

(In reply to comment #17)
> Hi I'm reopening this bug because Adam's patch does not fix the case where a user initiated navigation does a meta refresh and triggers a download; which, as discussed, should not be considered a user initiated navigation. 
> 
> To me it would seem that we should have IsProcessingUserGesture only return true if we know for certain that the navigation was user initiated. 
> 
> Adam, can you please review my patch to assure that I'm not overlooking anything by this change.
> 
> Thanks.

@Pierre-Antoine LaFayette, your patch is not the right way.

The default status of GestureIndicator is PossiblyProcessingUserGesture, some user-initiated actions don't explicitly pass the key/mouse events to WebKit (like typing URL in address bar and press enter), so in this case, we need to use the combination of GestureIndicator!=PossiblyProcessingUserGesture and no-js-on-stack to ensure this kind of user-gesture actions.


Yes, in current WebKit trunk, the meta refresh redirection caused  that FrameLoader::IsProcessingUserGesture returned true when calling it in WebViewClient::didStartProvisionalLoad, but the root cause is not what your patch pointed.

The bug was introduced by Adam's r47786(https://bugs.webkit.org/show_bug.cgi?id=47786).
In old WebKit code, the scheduleRefresh explicitly set false as user gesture status (meta refresh never can be considered as user initiated). However Adam might forget to keep this logic in NavigationScheduler::scheduleRedirect  (https://bugs.webkit.org/attachment.cgi?id=70988&action=diff#WebCore/loader/NavigationScheduler.cpp_sec6)

I will handle the patch once I finish https://bugs.webkit.org/show_bug.cgi?id=47849. Should be done today

[Johnny(Jianning) Ding]

> The bug was introduced by Adam's r47786(https://bugs.webkit.org/show_bug.cgi?id=47786).
> In old WebKit code, the scheduleRefresh explicitly set false as user gesture status (meta refresh never can be considered as user initiated). However Adam might forget to keep this logic in NavigationScheduler::scheduleRedirect  (https://bugs.webkit.org/attachment.cgi?id=70988&action=diff#WebCore/loader/NavigationScheduler.cpp_sec6)
> 
r47786 -> bug 47786, r69938(http://trac.webkit.org/changeset/69938)

[Adam Barth]

Comment on attachment 71290 [details]
Patch

What happened to the testing infrastructure?

[Johnny(Jianning) Ding]

(In reply to comment #20)
> (From update of attachment 71290 [details])
> What happened to the testing infrastructure?

Sent a patch, with using it we can track the gesture status in the frame load callbacks.
For example, the following test case can show that the user gesture is false in didStartProvisionalLoad callback. after fixing the meta-refresh bug.

<head>
<meta http-equiv="refresh" content="2">
</head>
<body>
<div id="loadCount"></div>
<div id="console"></div>
<script>      

    if (1 || window.layoutTestController) {
        if (!sessionStorage.loadCount)
            sessionStorage.loadCount = 1;
        else
            sessionStorage.loadCount = parseInt(sessionStorage.loadCount, 10) + 1;
        document.getElementById("loadCount").innerText = "load count : " + sessionStorage.loadCount;
        if (2 == sessionStorage.loadCount) {
            layoutTestController.notifyDone();
        } else {
            layoutTestController.dumpAsText();
            layoutTestController.dumpUserGestureInFrameLoadCallbacks();
            layoutTestController.waitUntilDone();
        }
    }
</script>

The result is
Frame with user gesture "true" - in didFinishDocumentLoadForFrame
Frame with user gesture "true" - in didHandleOnloadEventsForFrame
Frame with user gesture "true" - in willPerformClientRedirect
Frame with user gesture "true" - in didFinishLoadForFrame
Frame with user gesture "false" - in didStartProvisionalLoadForFrame
Frame with user gesture "true" - in didCancelClientRedirectForFrame
Frame with user gesture "true" - in didCommitLoadForFrame
Frame with user gesture "true" - in didFinishDocumentLoadForFrame
Frame with user gesture "true" - in didHandleOnloadEventsForFrame
Frame with user gesture "true" - in willPerformClientRedirect
Frame with user gesture "true" - in didFinishLoadForFrame
load count : 2

[Johnny(Jianning) Ding]

Created attachment 71374 [details]
patch v1

This patch depends on patch in https://bugs.webkit.org/show_bug.cgi?id=47849

[Johnny(Jianning) Ding]

(In reply to comment #22)
> This patch depends on patch in https://bugs.webkit.org/show_bug.cgi?id=47849

Miss test exceptions in skip list for GTK/win ports. Will add it later

[Johnny(Jianning) Ding]

(In reply to comment #23)
> Miss test exceptions in skip list for GTK/win ports. Will add it later.
I will move those two tests to another folder and send another patch for review.

[Johnny(Jianning) Ding]

Created attachment 71391 [details]
patch v1 (with layout tests)

[Adam Barth]

Comment on attachment 71391 [details]
patch v1 (with layout tests)

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

> WebCore/loader/NavigationScheduler.cpp:82
> +    // The derived class can call this method to override the m_wasUserGesture.
> +    void overrideWasUserGesture(bool wasUserGesture) { m_wasUserGesture = wasUserGesture; }

I'd remove this comment and call the function clearUserGesture().  Currently we don't have a use case for passing "true" here, and I'm not sure we will.

> WebCore/loader/NavigationScheduler.cpp:139
> +        // All ScheduledRedirects are not user-initiated, we need to override the m_wasUserGesture to false.
> +        overrideWasUserGesture(false);

I'd remove this comment too.  The test is a better explanation for why this line of code exists.

> LayoutTests/fast/frames/location-redirect-user-gesture.html:11
> +        setTimeout("location.href='data:text/html\,<script>layoutTestController.notifyDone()</" + "script>'", 1000);

The \ isn't needed.

> LayoutTests/fast/frames/meta-refresh-user-gesture.html:11
> +    if (1 || window.layoutTestController) {

1 || window.layoutTestController will always be true.

> LayoutTests/fast/frames/meta-refresh-user-gesture.html:17
> +        if (2 == sessionStorage.loadCount) {

Please clear session storage after this test so we can reload the test and have it work.

> LayoutTests/fast/frames/meta-refresh-user-gesture.html:18
> +            layoutTestController.notifyDone();

Doesn't this race with the refresh?

I feel like we're being cheap.  Why not just add another file and refresh to that file?  Then there isn't a worry about an infinite loop.

[Johnny(Jianning) Ding]

Created attachment 71396 [details]
patch v2

(In reply to comment #26)
All done.
> (From update of attachment 71391 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71391&action=review
> ...
> 1 || window.layoutTestController will always be true.
forgot to remove the test code:-(

[Adam Barth]

Comment on attachment 71396 [details]
patch v2

Thanks!

[Pierre-Antoine LaFayette]

Hi Johnny, I've tested the meta refresh case with today's build of Chromium and I'm still seeing IsProcessingUserGesture as true for the second provisional load when clicking on the link.

These are the test files I'm using:

9044.html:
<html>
<head>
<title>Issue 9044</title>
</head>
<body>
<ul>
    <li>
    <a href="9044_meta_redirect.html">Meta redirect to file</a>
    <li>
</ul>
</body>
</html>

9044_meta_redirect.html:
<html>
<head>
<meta HTTP-EQUIV="REFRESH" content="0; url=http://cygwin.com/setup.exe">
</head>
<body>
</body>
</html>

Can you reproduce this? I'm assuming that your changes are in the current Chromium build but I'm not sure.

[Johnny(Jianning) Ding]

(In reply to comment #29)
> Can you reproduce this? I'm assuming that your changes are in the current Chromium build but I'm not sure.

Unfortunately, the code still is not landed. Now I am not a committer, so I need someone who has committer privilege to land it. 
@Adam, would you please help me land it when you are free? Thanks!

@Pierre-Antoine, to test whether the issue can still be reproduced after landing this patch, I suggest that you download the latest patch and patch it under your own build/third_party/WebKit, then try again. Hope it helps:-)

[Pierre-Antoine LaFayette]

Oh right! False alarm :) I've tested the patch and it works as advertised. I didn't notice it hadn't even hit the commit queue.

Thanks!

[WebKit Commit Bot]

Comment on attachment 71396 [details]
patch v2

Rejecting patch 71396 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', 71396]" exit_code: 2
Last 500 characters of output:
tTests/fast/frames/meta-refresh-user-gesture.html
patching file LayoutTests/fast/frames/resources/meta-refresh-target.html
patching file LayoutTests/platform/gtk/Skipped
Hunk #1 FAILED at 5911.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/gtk/Skipped.rej
patching file LayoutTests/platform/win/Skipped
Hunk #1 succeeded at 1047 (offset 5 lines).

Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4847096

[Pierre-Antoine LaFayette]

(In reply to comment #32)
> (From update of attachment 71396 [details])
> Rejecting patch 71396 from commit-queue.
> 
> Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', 71396]" exit_code: 2
> Last 500 characters of output:
> tTests/fast/frames/meta-refresh-user-gesture.html
> patching file LayoutTests/fast/frames/resources/meta-refresh-target.html
> patching file LayoutTests/platform/gtk/Skipped
> Hunk #1 FAILED at 5911.
> 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/gtk/Skipped.rej
> patching file LayoutTests/platform/win/Skipped
> Hunk #1 succeeded at 1047 (offset 5 lines).
> 
> Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1
> 
> Full output: http://queues.webkit.org/results/4847096

Looks like the patch needs rebasing.

[Alexey Proskuryakov]

Comment on attachment 71396 [details]
patch v2

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

> LayoutTests/fast/frames/meta-refresh-user-gesture.html:13
> +In mete refresh redirection, the user gesture status in didStartProvisionalLoad callback should be false. For more details, please refer to https://bugs.webkit.org/show_bug.cgi?id=47817.

The bug doesn't really have more detail. It sort of kind of makes sense, but there is no concrete explanation or problem example in the bug.

 I don't know what's the use of checking for user gesture when downloading, so I can't tell what the benefit of making the indication more precise is.

> LayoutTests/platform/gtk/Skipped:5915
> +# https://bugs.webkit.org/show_bug.cgi?id=47849 and https://bugs.webkit.org/show_bug.cgi?id=47817

These are not correct bug numbers. Bug in Skipped list comment should be tracking a failure or missing DumpRenderTree feature. If there is no tracking bug, a comment should explain the failure.

> LayoutTests/platform/win/Skipped:1046
> +# https://bugs.webkit.org/show_bug.cgi?id=47849 and https://bugs.webkit.org/show_bug.cgi?id=47817

Ditto.

[Johnny(Jianning) Ding]

(In reply to comment #34)
Thanks, Alexey!
> The bug doesn't really have more detail. It sort of kind of makes sense, but there is no concrete explanation or problem example in the bug.
> 
>  I don't know what's the use of checking for user gesture when downloading, so I can't tell what the benefit of making the indication more precise is.

I thought Pierre-Antoine LaFayette's #1 comment might explain this issue. Anyway, I gonna change the explanation in test case to the following contents. @Pierre-Antoine LaFayette, can you also review it to see whether it explains this bug? Thanks!

Some WebKit ports send the user gesture to the embedders in didStartProvisionalLoad callback. The embedders use it to judge the navigation is user-initiated or not. In mete refresh redirection, the user gesture status in didStartProvisionalLoad callback should be false. 
For more details, please refer to https://bugs.webkit.org/show_bug.cgi?id=47817.

> These are not correct bug numbers. Bug in Skipped list comment should be tracking a failure or missing DumpRenderTree feature. If there is no tracking bug, a comment should explain the failure.

I change the comment to the following explanation.

# The following tests requires the DRT's dumpUserGestureInFrameLoadCallbacks
# method. But that method is not implemented in gtk port since gtk port can't
# get user gesture in frameload callbacks.
# The related bugs are https://bugs.webkit.org/show_bug.cgi?id=47849/47817.

Rebasing the patch, upload soon.

[Johnny(Jianning) Ding]

Created attachment 73373 [details]
re-baselined landing patch,

[Eric Seidel (no email)]

Comment on attachment 71396 [details]
patch v2

Cleared Adam Barth's review+ from obsolete attachment 71396 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Pierre-Antoine LaFayette]

There's a typo in LayoutTests/fast/frames/meta-refresh-user-gesture.html: 
In mete refresh redirection => In meta refresh redirection.

[Johnny(Jianning) Ding]

(In reply to comment #38)
> There's a typo in LayoutTests/fast/frames/meta-refresh-user-gesture.html: 
> In mete refresh redirection => In meta refresh redirection.

Thanks, after the patch (only re-baselined compared with last review+ patch) gets green light, I will ask for committer's help to fix the typo.

[Pierre-Antoine LaFayette]

(In reply to comment #39)
> (In reply to comment #38)
> > There's a typo in LayoutTests/fast/frames/meta-refresh-user-gesture.html: 
> > In mete refresh redirection => In meta refresh redirection.
> 
> Thanks, after the patch (only re-baselined compared with last review+ patch) gets green light, I will ask for committer's help to fix the typo.

The patch needs a new review since Eric cleared the review flag.

[Eric Seidel (no email)]

if abarth already reviewed, you can just fill him in as the reviewer in teh ChangeLog.  The clearning of flags on an obsolete patch is orthogonal to the review process.

[Johnny(Jianning) Ding]

(In reply to comment #41)
> if abarth already reviewed, you can just fill him in as the reviewer in teh ChangeLog.  The clearning of flags on an obsolete patch is orthogonal to the review process.

Thanks Eric! I re-generated the patch to resolve the latest conflict and filled Adam as reviewer in the ChangeLog. 
I guess we can land it patch now without a review again, am I right? If yes,  I want to ask for Abhishek's help to land it since Adam is kind of busy recently.

[Johnny(Jianning) Ding]

Created attachment 74215 [details]
re-baselined landing patch v2

[Johnny(Jianning) Ding]

Created attachment 75227 [details]
patch to resolve the latest conflict

Sorry for delay. After checking with Adam, I am gonna manually land it now

[Johnny(Jianning) Ding]

Created attachment 75230 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 75230 [details]:

inspector/styles-new-API.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 75230 [details]
Patch for landing

Rejecting patch 75230 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 75230]" exit_code: 2
Last 500 characters of output:
t-queue/WebCore/ChangeLog
Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Cannot dcommit with a dirty index.  Commit your changes first, or stash them with `git stash'.
 at /usr/local/git/libexec/git-core/git-svn line 485


Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Cannot dcommit with a dirty index.  Commit your changes first, or stash them with `git stash'.
 at /usr/local/git/libexec/git-core/git-svn line 485


Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1

Full output: http://queues.webkit.org/results/6837008

[Eric Seidel (no email)]

Comment on attachment 75230 [details]
Patch for landing

The second failure seems like a cq bug.  It may have hit an exception while trying to clean up things.

[WebKit Commit Bot]

Comment on attachment 75230 [details]
Patch for landing

Clearing flags on attachment: 75230

Committed r73181: <http://trac.webkit.org/changeset/73181>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 75230 [details]:

fast/media/mq-js-media-except-01.html
fast/preloader/script.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.