| Summary: | FrameLoader::IsProcessingUserGesture is true for JavaScript initiated downloads after click navigation to webpage | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Pierre-Antoine LaFayette <pierre.lafayette> | ||||||||||||||||||||||
| Component: | Page Loading | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, ap, commit-queue, eric, inferno, jnd, pierre.lafayette | ||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
Created attachment 71029 [details]
wait.html, when loaded it will download setup.exe after 5 secs.
This was observed in Chromium. RenderView::didStartProvisionalLoad sets its |navigation_gesture_| based on the results of FrameLoader::IsProcessingUserGesture. RenderView::didStartProvisionalLoad doesn't exist in svn.webkit.org. Is there an issue that's possible to observe using code from svn.webkit.org? (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. 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. > I will file a patch to fix this bug.
Great. We'll need to find a way to write a test for the fix...
(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. > 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...
Is the problem that we cannot simulate a click without JS on the stack? (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. (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? That's a good idea. We'll want to land the test infrastructure before the code change, of course. :) (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. 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. Nice. Created attachment 71290 [details]
Patch
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. (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 > 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) Comment on attachment 71290 [details]
Patch
What happened to the testing infrastructure?
(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 Created attachment 71374 [details] patch v1 This patch depends on patch in https://bugs.webkit.org/show_bug.cgi?id=47849 (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 (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. Created attachment 71391 [details]
patch v1 (with layout tests)
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. 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:-( Comment on attachment 71396 [details]
patch v2
Thanks!
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.
(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:-) 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! 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 (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. 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. (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. Created attachment 73373 [details]
re-baselined landing patch,
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. There's a typo in LayoutTests/fast/frames/meta-refresh-user-gesture.html: In mete refresh redirection => In meta refresh redirection. (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. (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. 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. (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. Created attachment 74215 [details]
re-baselined landing patch v2
Created attachment 75227 [details]
patch to resolve the latest conflict
Sorry for delay. After checking with Adam, I am gonna manually land it now
Created attachment 75230 [details]
Patch for landing
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 apavlov@chromium.org and pfeldman@chromium.org. The commit-queue is continuing to process your patch. 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 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.
Comment on attachment 75230 [details] Patch for landing Clearing flags on attachment: 75230 Committed r73181: <http://trac.webkit.org/changeset/73181> All reviewed patches have been landed. Closing bug. 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 abarth@webkit.org and kimmo.t.kinnunen@nokia.com. The commit-queue is continuing to process your patch. |
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;