Bug 52211 – FrameLoader::isProcessingUserGesture is wrong in dispatchWillPerformClientRedirect

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

[Joe Mason]

If frame->loader()->isProcessingUserGesture() is called while in the dispatchWillPerformClientRedirect callback, it returns "true" if the redirect is caused by a meta refresh tag.  It should return "false" in this case.

[Joe Mason]

Created attachment 78555 [details]
fix with layout tests

[Adam Barth]

jnd is our user gesture expert.  Thoughts?

[Johnny(Jianning) Ding]

(In reply to comment #2)
> jnd is our user gesture expert.  Thoughts?

I think Joe's patch is on the right way to fix this issue.
My only question is:
Is that necessary to restore the gesture state when calling clientRedirectCancelledOrFinished?
what scenarios need to know the gesture state of canceled redirection in the didCancelClientRedirect callback?

[Joe Mason]

(In reply to comment #3)
> (In reply to comment #2)
> > jnd is our user gesture expert.  Thoughts?
> 
> I think Joe's patch is on the right way to fix this issue.
> My only question is:
> Is that necessary to restore the gesture state when calling clientRedirectCancelledOrFinished?
> what scenarios need to know the gesture state of canceled redirection in the didCancelClientRedirect callback?

Actually I meant to take that out.  In fact, didCancelClientRedirect gets called from several completely different code paths, and in at least one other - namely when the redirect actually succeeds and the new page finishes loading - the gesture state isn't set, and it's unclear how it could be set.

So if we set it here, the gesture state in didCancelClientRedirect will be inconsistent - sometimes it will be the same as when the redirect started, and sometimes not.  This isn't useful to anybody.

The correct thing for the client to do if they want to know the gesture state in didCancelClientRedirect is to save it in willPerformClientRedirect and check (and clear) the saved state in didCancelClientRedirect.

[Joe Mason]

Created attachment 78692 [details]
fixed review comments

[Johnny(Jianning) Ding]

(In reply to comment #4)

> Actually I meant to take that out.  In fact, didCancelClientRedirect gets called from several completely different code paths, and in at least one other - namely when the redirect actually succeeds and the new page finishes loading - the gesture state isn't set, and it's unclear how it could be set.
> 
> So if we set it here, the gesture state in didCancelClientRedirect will be inconsistent - sometimes it will be the same as when the redirect started, and sometimes not.  This isn't useful to anybody.
> 
> The correct thing for the client to do if they want to know the gesture state in didCancelClientRedirect is to save it in willPerformClientRedirect and check (and clear) the saved state in didCancelClientRedirect.

That was my concern. Thanks to clearly comment and fix it.

[Daniel Bates]

Comment on attachment 78692 [details]
fixed review comments

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

This patch looks sane to me and Johnny Ding.

> Source/WebCore/loader/NavigationScheduler.cpp:126
> +        // do not set a UserGestureIndicator because there are other paths to dispatchDidCancelClientRedirect where the gesture state is not set, and we should be consistent with them

dispatchDidCancelClientRedirect => dispatchDidCancelClientRedirect()
(notice the added parentheses to indicate to the reader that this is a function call)

Nit: We prefer full and complete sentences for comments. Also this comment is very long, so please break it into two or more lines.

Also, can you include in this comment an example of a path that ends up calling dispatchDidCancelClientRedirect() where the gesture state isn't set.

> Source/WebCore/loader/NavigationScheduler.cpp:241
> +        // do not set a UserGestureIndicator because there are other paths to dispatchDidCancelClientRedirect where the gesture state is not set, and we should be consistent with them

Please update this comment according to the feedback I left for line 126 (above).

> Tools/ChangeLog:9
> +        (For the mac and chromium ports - other ports don't support dumping user gestures in DRT.)

Nit: Both Mac and Chromium are proper names and should be capitalized.

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:832
> -    if (!m_shell->shouldDumpFrameLoadCallbacks())
> -        return;
> -    printFrameDescription(frame);
> -    printf(" - willPerformClientRedirectToURL: %s \n", to.spec().data());
> +    if (m_shell->shouldDumpFrameLoadCallbacks()) {
> +        printFrameDescription(frame);
> +        printf(" - willPerformClientRedirectToURL: %s \n", to.spec().data());
> +    }
> +
> +    if (m_shell->shouldDumpUserGestureInFrameLoadCallbacks())
> +        printFrameUserGestureStatus(frame, " - in willPerformClientRedirect\n");

The name shouldDumpUserGestureInFrameLoadCallbacks(), in particular the suffix "InFrameLoadCallbacks", implies to me that we should only print the user gesture status when both shouldDumpFrameLoadCallbacks() and shouldDumpUserGestureInFrameLoadCallbacks() evaluates to true. That is, a code structure similar to:

if (!m_shell->shouldDumpFrameLoadCallbacks())
     return;
printFrameDescription(frame);
...
if (m_shell->shouldDumpUserGestureInFrameLoadCallbacks())
    printFrameUserGestureStatus(frame, " - in willPerformClientRedirect\n");

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:859
> +    if (m_shell->shouldDumpUserGestureInFrameLoadCallbacks())
> +        printFrameUserGestureStatus(frame, " - in didStartProvisionalLoadForFrame\n");
> +

Based on the comments above about the name shouldDumpUserGestureInFrameLoadCallbacks(), I would move this code inside the body of the if-statement "if (m_shell->shouldDumpFrameLoadCallbacks())" so that we only print the user gesture status when both shouldDumpFrameLoadCallbacks() and shouldDumpUserGestureInFrameLoadCallbacks() to true.

[Joe Mason]

(In reply to comment #7)
> The name shouldDumpUserGestureInFrameLoadCallbacks(), in particular the suffix "InFrameLoadCallbacks", implies to me that we should only print the user gesture status when both shouldDumpFrameLoadCallbacks() and shouldDumpUserGestureInFrameLoadCallbacks() evaluates to true. That is, a code structure similar to:
> 
> if (!m_shell->shouldDumpFrameLoadCallbacks())
>      return;
> printFrameDescription(frame);
> ...
> if (m_shell->shouldDumpUserGestureInFrameLoadCallbacks())
>     printFrameUserGestureStatus(frame, " - in willPerformClientRedirect\n");

I disagree.  This isn't how the existing uses of shouldDumpUserGestureInFrameLoadCallbacks are structured.  See FrameLoaderClientQt::dispatchDidStartProvisionalLoad() for instance:

     if (dumpFrameLoaderCallbacks)
         printf("%s - didStartProvisionalLoadForFrame\n", qPrintable(drtDescriptionSuitableForTestResult(m_frame)));

     if (dumpUserGestureInFrameLoaderCallbacks)
         printf("%s - in didStartProvisionalLoadForFrame\n", qPrintable(drtPrintFrameUserGestureStatus(m_frame)));

Treating it differently in willPerformClientRedirect() would be inconsistent, and changing it for all cases is out of scope of this bug.

I will update the comments and put up a new patch this weekend.

[Joe Mason]

Created attachment 92546 [details]
reworded comments as Dan suggested

[WebKit Review Bot]

Attachment 92546 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/loader/NavigationScheduler.cpp:130:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/loader/NavigationScheduler.cpp:249:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joe Mason]

Created attachment 92549 [details]
fixed style error

[Daniel Bates]

(In reply to comment #8)
> [...] 
> I disagree.  This isn't how the existing uses of shouldDumpUserGestureInFrameLoadCallbacks are structured.  See FrameLoaderClientQt::dispatchDidStartProvisionalLoad() for instance:

OK

[WebKit Commit Bot]

Comment on attachment 92549 [details]
fixed style error

Clearing flags on attachment: 92549

Committed r86013: <http://trac.webkit.org/changeset/86013>

[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 92549 [details]:

http/tests/xmlhttprequest/open-async-overload.html bug 51594 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

http://trac.webkit.org/changeset/86013 might have broken GTK Linux 32-bit Debug
The following tests are not passing:
svg/W3C-SVG-1.1/animate-elem-46-t.svg