Add the feature to retain the user gesture in the extension callback

Add the feature to retain the user gesture in the extension callback.

R=kalman@chromium.org,mek@chromium.org
BUG=351958, 361116
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266085

[wjywbs, 2014-04-07 21:46:36]

Hello kalman,

Please take a look this sample. Maybe I need to open a separate bug for
discussion.

Thanks.

https://codereview.chromium.org/227413008/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/permissions/permissions_api.cc (right):

https://codereview.chromium.org/227413008/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/permissions/permissions_api.cc:192:
//response_user_gesture_ = true;
This can cause security issues same as chrome.test.runWithUserGesture.

[not at google - send to devlin, 2014-04-07 22:09:09]

https://codereview.chromium.org/227413008/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/permissions/permissions_api.cc (right):

https://codereview.chromium.org/227413008/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/permissions/permissions_api.cc:192:
//response_user_gesture_ = true;
On 2014/04/07 21:46:37, wjywbs wrote:
> This can cause security issues same as chrome.test.runWithUserGesture.

how?

https://codereview.chromium.org/227413008/diff/1/chrome/browser/renderer_host...
File
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.h
(right):

https://codereview.chromium.org/227413008/diff/1/chrome/browser/renderer_host...
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.h:91:
const bool response_user_gesture);
just |user_gesture| is fine, everywhere else too.

[wjywbs, 2014-04-08 13:24:41]

I filed Issue 361116 with some discussions. PTAL. Thanks.

https://codereview.chromium.org/227413008/diff/1/chrome/browser/renderer_host...
File
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.h
(right):

https://codereview.chromium.org/227413008/diff/1/chrome/browser/renderer_host...
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.h:91:
const bool response_user_gesture);
On 2014/04/07 22:09:09, kalman wrote:
> just |user_gesture| is fine, everywhere else too.

I thought |user_gesture| is defined in extension_function.h, so
|response_user_gesture| can distinct with it. I can change
|response_user_gesture| to |user_gesture| outside extension_function.h

[not at google - send to devlin, 2014-04-08 16:01:09]

https://codereview.chromium.org/227413008/diff/1/chrome/browser/renderer_host...
File
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.h
(right):

https://codereview.chromium.org/227413008/diff/1/chrome/browser/renderer_host...
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.h:91:
const bool response_user_gesture);
On 2014/04/08 13:24:42, wjywbs wrote:
> On 2014/04/07 22:09:09, kalman wrote:
> > just |user_gesture| is fine, everywhere else too.
> 
> I thought |user_gesture| is defined in extension_function.h, so
> |response_user_gesture| can distinct with it. I can change
> |response_user_gesture| to |user_gesture| outside extension_function.h

Yeah the only place where you'd need to have a different name is in
extension_function. It's a bit confusing with respect to the "user_gesture"
property. Perhaps we should call response_user_gesture more
"retain_user_gesture"; it's much the same, but clearer what is going on (and
idiomatically more restrictive).

[wjywbs, 2014-04-08 23:01:45]

I renamed the variable. PTAL. Thanks.

[not at google - send to devlin, 2014-04-09 16:44:32]

https://codereview.chromium.org/227413008/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/permissions/permissions_api.cc (right):

https://codereview.chromium.org/227413008/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/permissions/permissions_api.cc:134: 
you may need to split this method up.

https://codereview.chromium.org/227413008/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/permissions/permissions_api.cc:192:
retain_user_gesture_ = true;
this creates the problem you mention in the bug. we only want to actually retain
the user action bit if the dialog was shown (and accepted), which didn't happen
here.

https://codereview.chromium.org/227413008/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/permissions/permissions_api.cc:211:
InstallUIProceed();
likewise, the check/acceptance for "has_no_warnings" retains the user action bit
when it shouldn't.

https://codereview.chromium.org/227413008/diff/20001/chrome/browser/renderer_...
File
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.h
(right):

https://codereview.chromium.org/227413008/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.h:91:
const bool user_gesture);
no need for const, it's meaningless if it's not a ref or pointer.

https://codereview.chromium.org/227413008/diff/20001/extensions/browser/exten...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/227413008/diff/20001/extensions/browser/exten...
extensions/browser/extension_function.cc:158: response_callback_.Run(type,
*results_, GetError(), retain_user_gesture_);
it should be "user_gesture_ && retain_user_gesture_".

https://codereview.chromium.org/227413008/diff/20001/extensions/browser/exten...
File extensions/browser/extension_function.h (right):

https://codereview.chromium.org/227413008/diff/20001/extensions/browser/exten...
extensions/browser/extension_function.h:255: // True if the response callback
should include a user gesture.
True if the response callback should retain the |user_gesture_| bit when sending
the function response.

[wjywbs, 2014-04-09 18:13:29]

PTAL, thanks!

https://codereview.chromium.org/227413008/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/permissions/permissions_api.cc (right):

https://codereview.chromium.org/227413008/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/permissions/permissions_api.cc:192:
retain_user_gesture_ = true;
I will post a reply on the bug to discuss this.

[not at google - send to devlin, 2014-04-09 20:24:10]

this lgtm, but mek@ could you sanity check and make sure that what you have
planned doesn't conflict or moot it?

[Marijn Kruisselbrink, 2014-04-09 20:27:02]

On 2014/04/09 20:24:10, kalman wrote:
> this lgtm, but mek@ could you sanity check and make sure that what you have
> planned doesn't conflict or moot it?
it doesn't really conflict, but I don't think this is the right way of doing it.
If you want to retain a user gesture and later recreate it in the same renderer
process you should store the old user gesture token, and use that to recreate a
new user gesture. Not create an entirely new user gesture from scratch. This is
exactly the usecase user gesture tokens were designed for.

[not at google - send to devlin, 2014-04-09 20:28:51]

not lgtm I just realised the change to permissions_api.cc wasn't made. i guess
that's what the discussion was about.

[not at google - send to devlin, 2014-04-09 20:30:03]

On 2014/04/09 20:27:02, Marijn Kruisselbrink wrote:
> On 2014/04/09 20:24:10, kalman wrote:
> > this lgtm, but mek@ could you sanity check and make sure that what you have
> > planned doesn't conflict or moot it?
> it doesn't really conflict, but I don't think this is the right way of doing
it.
> If you want to retain a user gesture and later recreate it in the same
renderer
> process you should store the old user gesture token, and use that to recreate
a
> new user gesture. Not create an entirely new user gesture from scratch. This
is
> exactly the usecase user gesture tokens were designed for.

Re-using the old user gesture token makes sense. The problem in this case is
that if a dialog shows then it might be a long time before the user acknowledges
it, and the user gesture token would expire. You could re-create the user
gesture token but that has other problems (like re-requesting permissions
repeatedly).

[Marijn Kruisselbrink, 2014-04-09 20:37:00]

On 2014/04/09 20:30:03, kalman wrote:
> On 2014/04/09 20:27:02, Marijn Kruisselbrink wrote:
> > On 2014/04/09 20:24:10, kalman wrote:
> > > this lgtm, but mek@ could you sanity check and make sure that what you
have
> > > planned doesn't conflict or moot it?
> > it doesn't really conflict, but I don't think this is the right way of doing
> it.
> > If you want to retain a user gesture and later recreate it in the same
> renderer
> > process you should store the old user gesture token, and use that to
recreate
> a
> > new user gesture. Not create an entirely new user gesture from scratch. This
> is
> > exactly the usecase user gesture tokens were designed for.
> 
> Re-using the old user gesture token makes sense. The problem in this case is
> that if a dialog shows then it might be a long time before the user
acknowledges
> it, and the user gesture token would expire. You could re-create the user
> gesture token but that has other problems (like re-requesting permissions
> repeatedly).
Actually, since this is exactly what user gesture token retention was meant for,
creating a new user with an old token currently always resets the timestamp
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
So no, it won't expire.

[not at google - send to devlin, 2014-04-10 04:02:19]

On 2014/04/09 20:37:00, Marijn Kruisselbrink wrote:
> On 2014/04/09 20:30:03, kalman wrote:
> > On 2014/04/09 20:27:02, Marijn Kruisselbrink wrote:
> > > On 2014/04/09 20:24:10, kalman wrote:
> > > > this lgtm, but mek@ could you sanity check and make sure that what you
> have
> > > > planned doesn't conflict or moot it?
> > > it doesn't really conflict, but I don't think this is the right way of
doing
> > it.
> > > If you want to retain a user gesture and later recreate it in the same
> > renderer
> > > process you should store the old user gesture token, and use that to
> recreate
> > a
> > > new user gesture. Not create an entirely new user gesture from scratch.
This
> > is
> > > exactly the usecase user gesture tokens were designed for.
> > 
> > Re-using the old user gesture token makes sense. The problem in this case is
> > that if a dialog shows then it might be a long time before the user
> acknowledges
> > it, and the user gesture token would expire. You could re-create the user
> > gesture token but that has other problems (like re-requesting permissions
> > repeatedly).
> Actually, since this is exactly what user gesture token retention was meant
for,
> creating a new user with an old token currently always resets the timestamp
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> So no, it won't expire.

But we also do want expiry in some cases? Like in the case of constantly calling
chrome.permissions.request() to maintain the user action bit?

[wjywbs, 2014-04-10 04:44:21]

The code path for retaining the token in UserGestureIndicator.cpp can be
confusing. It only resets the time stamp of the argument |token|. If
|s_topmostIndicator| exists and the |token| passed in is not the same one as
s_topmostIndicator->currentToken(), it does not reset the time in line 154-158.
Only line 152 resets the token and the time stamp. I'm not sure whether the
|token| and s_topmostIndicator->currentToken() are the same yet in this case
(RequestSender::StartRequest).

[wjywbs, 2014-04-10 14:15:48]

I changed RequestSender to retain the original user gesture token. PTAL. Thanks.

[not at google - send to devlin, 2014-04-10 20:21:07]

https://codereview.chromium.org/227413008/diff/60001/chrome/renderer/extensio...
File chrome/renderer/extensions/request_sender.cc (right):

https://codereview.chromium.org/227413008/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/request_sender.cc:24: blink::WebUserGestureToken
token)
run "git cl format" pls

https://codereview.chromium.org/227413008/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/request_sender.cc:141: gesture.reset(new
blink::WebScopedUserGesture(request->token));
ok... but just to check, if the dialog is shown and stays open for 5 minutes,
will the user gesture token still work here?

[wjywbs, 2014-04-10 21:14:26]

PTAL. Thanks.

https://codereview.chromium.org/227413008/diff/60001/chrome/renderer/extensio...
File chrome/renderer/extensions/request_sender.cc (right):

https://codereview.chromium.org/227413008/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/request_sender.cc:24: blink::WebUserGestureToken
token)
On 2014/04/10 20:21:07, kalman wrote:
> run "git cl format" pls

Fixed.

https://codereview.chromium.org/227413008/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/request_sender.cc:141: gesture.reset(new
blink::WebScopedUserGesture(request->token));
On 2014/04/10 20:21:07, kalman wrote:
> ok... but just to check, if the dialog is shown and stays open for 5 minutes,
> will the user gesture token still work here?

I waited for about 10 minutes and it worked.

[not at google - send to devlin, 2014-04-10 21:19:33]

https://codereview.chromium.org/227413008/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/permissions/permissions_api.cc (right):

https://codereview.chromium.org/227413008/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/permissions/permissions_api.cc:192:
retain_user_gesture_ = true;
if we're re-using the user gesture token on the renderer do we even need this
"retain user gesture" stuff? can we just pass the user gesture bit back down
again?

[not at google - send to devlin, 2014-04-10 21:19:43]

also this needs tests

[wjywbs, 2014-04-10 21:42:29]

On 2014/04/10 21:19:33, kalman wrote:
>
https://codereview.chromium.org/227413008/diff/80001/chrome/browser/extension...
> File chrome/browser/extensions/api/permissions/permissions_api.cc (right):
> 
>
https://codereview.chromium.org/227413008/diff/80001/chrome/browser/extension...
> chrome/browser/extensions/api/permissions/permissions_api.cc:192:
> retain_user_gesture_ = true;
> if we're re-using the user gesture token on the renderer do we even need this
> "retain user gesture" stuff? can we just pass the user gesture bit back down
> again?

Then all callback functions will retain the user gesture. Do we want this
behavior?

chrome.management.uninstall("id", function() {
  // The user gesture is still valid inside.
});

// Suppose there's a user gesture outside.
chrome.sessions.getRecentlyClosed(function(data) {
  // Do we need the user gesture here?
});

Thanks.

[wjywbs, 2014-04-10 23:12:53]

I've added tests. PTAL. Thanks.

[not at google - send to devlin, 2014-04-11 00:21:15]

On 2014/04/10 21:42:29, wjywbs wrote:
> On 2014/04/10 21:19:33, kalman wrote:
> >
>
https://codereview.chromium.org/227413008/diff/80001/chrome/browser/extension...
> > File chrome/browser/extensions/api/permissions/permissions_api.cc (right):
> > 
> >
>
https://codereview.chromium.org/227413008/diff/80001/chrome/browser/extension...
> > chrome/browser/extensions/api/permissions/permissions_api.cc:192:
> > retain_user_gesture_ = true;
> > if we're re-using the user gesture token on the renderer do we even need
this
> > "retain user gesture" stuff? can we just pass the user gesture bit back down
> > again?
> 
> Then all callback functions will retain the user gesture. Do we want this
> behavior?
> 
> chrome.management.uninstall("id", function() {
>   // The user gesture is still valid inside.
> });
> 
> // Suppose there's a user gesture outside.
> chrome.sessions.getRecentlyClosed(function(data) {
>   // Do we need the user gesture here?
> });
> 
> Thanks.

Yeah that's what I was wondering. It seems surprising to be making exceptions
for specific APIs - so long as the user gesture bit *does* eventually get
consumed, or time out.

[wjywbs, 2014-04-11 21:17:26]

I'm assuming you've confirmed to retain the user gesture all  function
callbacks? Please take a look, thanks.

[not at google - send to devlin, 2014-04-11 23:22:19]

Ok thanks for sticking with this. Sorry about the crazy back and forth with the
browser-side changes.

https://codereview.chromium.org/227413008/diff/120001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/permissions/optional_retain_gesture/background.js
(right):

https://codereview.chromium.org/227413008/diff/120001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/permissions/optional_retain_gesture/background.js:49:
// Wait 2s to make sure the user gesture is expired.
I wish there were a better way to test this than waiting 2 seconds. It's bound
to be flaky.

How about: you keep trying every... 100ms... to request. Eventually it should
fail, if not, the test times out. Similar to what you did for the onChanged
event in the sessions API. Worth thinking about how that functionality could be
moved into the chrome.test module.

[not at google - send to devlin, 2014-04-11 23:22:37]

apart from the test lg, mek do you have anything to add?

[wjywbs, 2014-04-13 15:38:02]

On 2014/04/11 23:22:19, kalman wrote:
> How about: you keep trying every... 100ms... to request. Eventually it should
> fail, if not, the test times out. Similar to what you did for the onChanged
> event in the sessions API. Worth thinking about how that functionality could
be
> moved into the chrome.test module.

I synced the files because request_sender.cc was moved to e/r. I updated the
test, but I found out in this case we can't try every 100ms. If the user gesture
timestamp is reset every 100ms, it will never expire. Now it first waits for 2s.
If the test doesn't succeed, try again in 3s.

[not at google - send to devlin, 2014-04-14 17:56:04]

On 2014/04/13 15:38:02, wjywbs wrote:
> On 2014/04/11 23:22:19, kalman wrote:
> > How about: you keep trying every... 100ms... to request. Eventually it
should
> > fail, if not, the test times out. Similar to what you did for the onChanged
> > event in the sessions API. Worth thinking about how that functionality could
> be
> > moved into the chrome.test module.
> 
> I synced the files because request_sender.cc was moved to e/r. I updated the
> test, but I found out in this case we can't try every 100ms. If the user
gesture
> timestamp is reset every 100ms, it will never expire. Now it first waits for
2s.
> If the test doesn't succeed, try again in 3s.

The user gesture token resetting every 100ms seems wrong, I thought the point of
holding onto the token was so that you couldn't reset it that way?

[wjywbs, 2014-04-14 18:11:51]

On 2014/04/14 17:56:04, kalman wrote:
> The user gesture token resetting every 100ms seems wrong, I thought the point
of
> holding onto the token was so that you couldn't reset it that way?

On 2014/04/09 20:37:00, Marijn Kruisselbrink wrote:
> Actually, since this is exactly what user gesture token retention was meant
for,
> creating a new user with an old token currently always resets the timestamp
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> So no, it won't expire.

Yes, it's wrong and I think mek is working on fixing this. The token's timestamp
is reset when we use it to re-create a WebScopedUserGesture.

[not at google - send to devlin, 2014-04-14 19:59:23]

Ok, I think that mek@ should be doing this review not me.

[wjywbs, 2014-04-22 22:13:00]

Hello mek, please take a look when you have time. Thanks.

[Marijn Kruisselbrink, 2014-04-23 23:00:42]

On 2014/04/22 22:13:00, wjywbs wrote:
> Hello mek, please take a look when you have time. Thanks.
Sorry for the delay. Implementation lgtm, the tests don't quite test what you
seem to be trying to test, but that might or might not matter.

https://codereview.chromium.org/227413008/diff/130001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/permissions/optional_retain_gesture/background.js
(right):

https://codereview.chromium.org/227413008/diff/130001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/permissions/optional_retain_gesture/background.js:23:
// window.open. Can't wait longer than 1s since the user
very minor nit: this comment is a bit misleading, since window.setTimeout is a
bit special in its user gesture treatment. window.setTimeout doesn't actually
cause the user gesture to expire, it just is not retained for timeouts > 1000ms,
which only coincidentally is the same as the time until user gestures normally
expire.

https://codereview.chromium.org/227413008/diff/130001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/permissions/optional_retain_gesture/background.js:43:
function testPermissionsRetainGestureExpire() {
Similarly all the setTimeout calls in this test don't actually test what you
expect them to test. They test the fact that setTimeout doesn't retain the user
gesture when the timeout > 1000ms, but don't test that the user gesture does or
doesn't expire. (I'm not sure how you'd reliably test that anyway; the only way
to get a user gesture to expire seems to be to busy-loop for > 1 second).

[wjywbs, 2014-04-24 04:33:25]

I updated the test to use busy loops. PTAL, thanks.

[Marijn Kruisselbrink, 2014-04-24 17:10:02]

https://codereview.chromium.org/227413008/diff/150001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/permissions/optional_retain_gesture/background.js
(right):

https://codereview.chromium.org/227413008/diff/150001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/permissions/optional_retain_gesture/background.js:29:
// Wait 500ms for consuming the user gesture in
You don't actually need to wait here. Javascript is single-threaded, and our
callbacks are not reentrant, so the window.open will always be called before the
callback is called.

[wjywbs, 2014-04-24 20:51:40]

https://codereview.chromium.org/227413008/diff/150001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/permissions/optional_retain_gesture/background.js
(right):

https://codereview.chromium.org/227413008/diff/150001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/permissions/optional_retain_gesture/background.js:29:
// Wait 500ms for consuming the user gesture in
On 2014/04/24 17:10:02, Marijn Kruisselbrink wrote:
> You don't actually need to wait here. Javascript is single-threaded, and our
> callbacks are not reentrant, so the window.open will always be called before
the
> callback is called.

I removed the loop here. PTAL. Thanks.

[Marijn Kruisselbrink, 2014-04-24 20:55:14]

On 2014/04/24 20:51:40, wjywbs wrote:
>
https://codereview.chromium.org/227413008/diff/150001/chrome/test/data/extens...
> File
>
chrome/test/data/extensions/api_test/permissions/optional_retain_gesture/background.js
> (right):
> 
>
https://codereview.chromium.org/227413008/diff/150001/chrome/test/data/extens...
>
chrome/test/data/extensions/api_test/permissions/optional_retain_gesture/background.js:29:
> // Wait 500ms for consuming the user gesture in
> On 2014/04/24 17:10:02, Marijn Kruisselbrink wrote:
> > You don't actually need to wait here. Javascript is single-threaded, and our
> > callbacks are not reentrant, so the window.open will always be called before
> the
> > callback is called.
> 
> I removed the loop here. PTAL. Thanks.
lgtm

[wjywbs, 2014-04-24 20:58:16]

The CQ bit was checked by wjywbs@gmail.com

[wjywbs, 2014-04-24 21:12:19]

kalman@, can you please take a look at this CL? I think CQ does not pick up this
CL because of your not lgtm. Thanks.

[commit-bot: I haz the power, 2014-04-24 21:14:44]

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

[not at google - send to devlin, 2014-04-24 21:20:04]

rubberstamp lgtm

[commit-bot: I haz the power, 2014-04-25 02:24:19]

Change committed as 266085