Keep the user gesture when postMessage() across frames.

Keep the user gesture when postMessage() across frames.

R=adamk@chromium.org
BUG=355658, 178319, 161068
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170243

[wjywbs, 2014-03-22 02:19:25]

This is my first time changing blink, and I'm not sure whether I need to make a
test case and how to make one. I tested this change locally. Thanks.

[not at google - send to devlin, 2014-03-24 10:59:20]

I think Adam is going to need a bit more context for this (and possibly CC
abarth though he's on some other review I saw?).

Basically, extension messaging preserves the user action bit so that we can
implement cross-process UIs (as in the bug).

It would be good if postMessage could do the same; however, in order to do that,
we're going to need some way of ensuring that it doesn't stay alive
indefinitely. Can the user gesture time out? Can there be an API for
transferring the user action bit over postMessage (e.g. using transferrables)?

[not at google - send to devlin, 2014-03-24 10:59:43]

This probably needs to go through the blink launch process or whatever anyway.

[adamk, 2014-03-24 17:17:42]

On 2014/03/24 10:59:43, kalman wrote:
> This probably needs to go through the blink launch process or whatever anyway.

I agree with Ben that this would be better-discussed not on this code review,
but perhaps on blink-dev (or on a new bug report about the general issue of
preserving user-gesture-ness across frames).

[abarth-chromium, 2014-03-24 17:39:47]

https://codereview.chromium.org/209363002/diff/1/Source/core/frame/DOMWindow.cpp
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/209363002/diff/1/Source/core/frame/DOMWindow....
Source/core/frame/DOMWindow.cpp:880: gesture.reset(new
UserGestureIndicator(DefinitelyProcessingNewUserGesture));
As written, this CL is incorrect because if you have a gesture and you queue up
100 postMessages, you'll get 100 new gesture tokens, which means you can perform
100 actions that consume user gestures instead of just one.  Instead, you need
to keep a reference to the gesture token instead of a flat bool.

[wjywbs, 2014-03-24 18:53:55]

On 2014/03/24 17:39:47, abarth wrote:
>
https://codereview.chromium.org/209363002/diff/1/Source/core/frame/DOMWindow.cpp
> File Source/core/frame/DOMWindow.cpp (right):
> 
>
https://codereview.chromium.org/209363002/diff/1/Source/core/frame/DOMWindow....
> Source/core/frame/DOMWindow.cpp:880: gesture.reset(new
> UserGestureIndicator(DefinitelyProcessingNewUserGesture));
> As written, this CL is incorrect because if you have a gesture and you queue
up
> 100 postMessages, you'll get 100 new gesture tokens, which means you can
perform
> 100 actions that consume user gestures instead of just one.  Instead, you need
> to keep a reference to the gesture token instead of a flat bool.

I've filed Issue 355658.

Sorry for introducing security issues in this CL, but this is exactly the method
to send user gesture in chrome.runtime.postMessage in extension side. I don't
know whether the user gesture object is thread safe for handling 100
postMessages, and whether the frames can be in different processes. I know that
chrome.runtime.postMessage in extension side needs cross-process user gesture
object passing. If there's a transferrable user gesture object/token like kalman
said, or a global user gesture tracker, etc, several user gesture handling
places can be safer.

I'll try to pass the user gesture object/token and see what happens. Thanks.

[not at google - send to devlin, 2014-03-25 14:35:01]

quick aside: there is a known issue with the extensions code for extending the
lifetime of the user action bit, so be mindful of that.

[wjywbs, 2014-03-26 03:37:03]

Now it pass the user gesture token so that the user gesture can only be consumed
once even if there's a stack of postMessages.

I haven't found a good method to consume the user gesture in script. window.open
will keep opening new windows when the gestures become zero. I'm currently
testing this script with --test-type=browser:

window.addEventListener("message", function(event) {
  window.postMessage({}, "*");
  chrome.permissions.request({
    origins: ['http://www.example.com/']
  }, function(granted) {});
  chrome.test.runWithoutUserGesture(function(){}); // consume gesture
});

Only one permission prompt is shown, which is correct.

Please take another look. Thanks.

[abarth-chromium, 2014-03-26 05:32:47]

This should be testable with a LayoutTest.  Maybe look at how other changes that
involve user gestures were tested.

https://codereview.chromium.org/209363002/diff/30001/Source/core/frame/DOMWin...
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/209363002/diff/30001/Source/core/frame/DOMWin...
Source/core/frame/DOMWindow.cpp:114: PostMessageTimer(DOMWindow& window,
PassRefPtr<SerializedScriptValue> message, const String& sourceOrigin,
PassRefPtr<DOMWindow> source, PassOwnPtr<MessagePortChannelArray> channels,
SecurityOrigin* targetOrigin, PassRefPtr<ScriptCallStack> stackTrace,
UserGestureToken* userGestureToken)
This list of parameters is getting pretty out of control.

https://codereview.chromium.org/209363002/diff/30001/Source/core/frame/DOMWin...
Source/core/frame/DOMWindow.cpp:877: scoped_ptr<UserGestureIndicator> gesture;
You can't use scoped_ptr in Blink.  I'm surprised it even was visible.  You need
to use OwnPtr instead.

https://codereview.chromium.org/209363002/diff/30001/Source/core/frame/DOMWin...
Source/core/frame/DOMWindow.cpp:879: gesture.reset(new
UserGestureIndicator(timer->userGestureToken()));
You can't just create this object on the stack and pass in the userGestureToken?
 I would have assumed that the UserGestureIndicator can handle a null token.

[wjywbs, 2014-03-27 05:23:37]

I updated the UserGestureIndicator handling same as
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

And I added tests. PTAL. Thanks.

[abarth-chromium, 2014-03-27 20:39:26]

lgtm

[commit-bot: I haz the power, 2014-03-27 20:39:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjywbs@gmail.com/209363002/110001

[commit-bot: I haz the power, 2014-03-27 22:32:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 22:32:56]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[wjywbs, 2014-03-27 23:22:37]

The CQ bit was checked by wjywbs@gmail.com

[commit-bot: I haz the power, 2014-03-27 23:22:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjywbs@gmail.com/209363002/110001

[commit-bot: I haz the power, 2014-03-28 00:31:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 00:31:34]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[wjywbs, 2014-03-28 00:49:45]

The CQ bit was checked by wjywbs@gmail.com

[commit-bot: I haz the power, 2014-03-28 00:49:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjywbs@gmail.com/209363002/110001

[commit-bot: I haz the power, 2014-03-28 01:53:14]

Change committed as 170243

[Charlie Reis, 2014-03-28 16:22:38]

https://codereview.chromium.org/209363002/diff/110001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/209363002/diff/110001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindow.cpp:874: if
(m_frame->loader().client()->willCheckAndDispatchMessageEvent(timer->targetOrigin(),
event.get()))
What about cross-process postMessage?  Seems like we might need to pass the
token to the new process.  This is where it would be handed off to the embedder.