Bug 97483

Summary: It's possible to clone consumable user gestures
Product: WebKit Reporter: jochen
Component: WebCore Misc.Assignee: jochen
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cdn, cevans, dglazkov, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description jochen 2012-09-24 14:31:49 PDT
Since DOMTimer copies the user gesture state (but does not track whether or not the user gesture is consumed), you can effectively fork it like this:

<script>
function foo() {
 setTimeout(function() { var c = window.open("about:blank"); c.close(); }, 0);
 window.open("data:text/html,pop-under", "", "width=300,height=300");
}
</script>
<button onclick="foo()">Click Me</button>

A port supporting user gesture consumption should only create one window. Since the timeout is created before the first window.open call, however, two windows are created.
Comment 1 jochen 2012-10-02 04:29:38 PDT
Created attachment 166662 [details]
Patch
Comment 2 Build Bot 2012-10-02 05:56:32 PDT
Comment on attachment 166662 [details]
Patch

Attachment 166662 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14114676

New failing tests:
platform/mac/editing/deleting/backward-delete.html
fast/events/popup-blocking-timers.html
svg/custom/use-instanceRoot-event-bubbling.xhtml
Comment 3 WebKit Review Bot 2012-10-02 06:47:32 PDT
Comment on attachment 166662 [details]
Patch

Attachment 166662 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14138159

New failing tests:
fast/events/popup-blocking-timers.html
svg/custom/use-instanceRoot-event-bubbling.xhtml
Comment 4 jochen 2012-10-02 06:59:07 PDT
Created attachment 166685 [details]
Patch
Comment 5 jochen 2012-10-02 11:15:25 PDT
*** Bug 98060 has been marked as a duplicate of this bug. ***
Comment 6 Adam Barth 2012-10-02 11:19:31 PDT
Comment on attachment 166685 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        is suppoed to forward the user gesture, take a reference to this token

suppoed -> supposed

> Source/WebCore/dom/UserGestureIndicator.cpp:37
> +        return adoptRef(static_cast<UserGestureIndicator::Token*>(new GestureToken));

Is this static_cast needed?  I thought PassRefPtr was smart enough to know how to do the type conversion for you.

> Source/WebCore/dom/UserGestureIndicator.cpp:42
> +    void inc() { m_consumableGestures++; }

inc -> increment?  Please use complete words in variable names.  Maybe addGesture() is a better name?

> Source/WebCore/dom/UserGestureIndicator.cpp:43
> +    bool consume()

consume -> consumeGesture?

> Source/WebCore/dom/UserGestureIndicator.cpp:50
> +    bool isValid() const { return m_consumableGestures > 0; }

isValid -> hasGestures() ?
Comment 7 Adam Barth 2012-10-02 11:21:04 PDT
Comment on attachment 166685 [details]
Patch

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

> Source/WebCore/dom/UserGestureIndicator.h:64
> +    ProcessingUserGestureState m_state;
> +    Token* m_previousToken;
> +    RefPtr<Token> m_token;

It seems strange to need to add all this information to the stack.
Comment 8 Adam Barth 2012-10-02 11:23:34 PDT
Comment on attachment 166685 [details]
Patch

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

> Source/WebCore/page/DOMTimer.h:73
> -        bool m_shouldForwardUserGesture;
> +        RefPtr<UserGestureIndicator::Token> m_userGestureToken;

This part makes sense.  DOMTimer wants to forward the user gesture only if it wasn't consumed.
Comment 9 jochen 2012-10-02 11:25:12 PDT
(In reply to comment #7)
> (From update of attachment 166685 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166685&action=review
> 
> > Source/WebCore/dom/UserGestureIndicator.h:64
> > +    ProcessingUserGestureState m_state;
> > +    Token* m_previousToken;
> > +    RefPtr<Token> m_token;
> 
> It seems strange to need to add all this information to the stack.

hum, indeed m_state is not required.

The other two, however, are required.
Comment 10 Adam Barth 2012-10-02 11:33:44 PDT
This design seems more complicated than needed.

What question is DOMTimer really trying to answer?  It wants to know whether all the user gestures for its call stack where consumed before that stack unwound.  That means we only need one counter per call stack, not  stack of counters, right?
Comment 11 jochen 2012-10-02 13:09:07 PDT
Created attachment 166738 [details]
Patch
Comment 12 jochen 2012-10-02 13:10:20 PDT
Created attachment 166739 [details]
Patch
Comment 13 jochen 2012-10-02 13:11:47 PDT
I believe I addressed all your comments
Comment 14 jochen 2012-10-02 14:03:41 PDT
Created attachment 166747 [details]
Patch
Comment 15 Adam Barth 2012-10-03 02:21:58 PDT
Comment on attachment 166747 [details]
Patch

Looks great.  Thanks.
Comment 16 WebKit Review Bot 2012-10-03 02:45:26 PDT
Comment on attachment 166747 [details]
Patch

Clearing flags on attachment: 166747

Committed r130267: <http://trac.webkit.org/changeset/130267>
Comment 17 WebKit Review Bot 2012-10-03 02:45:30 PDT
All reviewed patches have been landed.  Closing bug.