Consume user gestures when navigating in a new tab/window. This ensures that a site can not create …

Consume user gestures when navigating in a new tab/window. This ensures that a site can not create large numbers of popups or new tabs piggybacking on a single user gesture.

BUG=144704
TEST=N/A

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153731

[Charlie Reis, 2012-08-27 19:18:55]

Hmm, I'm not familiar with the process/consume user gesture logic, so I don't
know whether this is a good fix or not.  Maybe abarth@ knows it?

(I also don't have access to the bug, if you could CC me.)

[Cris Neckar, 2012-08-27 19:23:52]

On 2012/08/27 19:18:55, creis wrote:
> Hmm, I'm not familiar with the process/consume user gesture logic, so I don't
> know whether this is a good fix or not.  Maybe abarth@ knows it?
> 
> (I also don't have access to the bug, if you could CC me.)

I just CCed you on the bug. Sorry about that.

I just added consumable user gestures to WebKit so understandable that you
aren't familiar with them :).

http://trac.webkit.org/changeset/126609

abarth reviewed my WebKit change so probably good to include him here as well.

[abarth-chromium, 2012-08-27 19:46:05]

http://codereview.chromium.org/10873090/diff/6001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10873090/diff/6001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1631: frame->consumeUserGesture();
Can we just call this unconditionally?

http://codereview.chromium.org/10873090/diff/6001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1726: creator->consumeUserGesture();
We need to do this both for OpenURL and createView ?  I would have hoped we
would only need to do this in one place (likely here).  Also, this comment is
kind of useless because it just restates what the code below it says.

[Cris Neckar, 2012-08-27 19:54:31]

>
http://codereview.chromium.org/10873090/diff/6001/content/renderer/render_vie...
> content/renderer/render_view_impl.cc:1631: frame->consumeUserGesture();
> Can we just call this unconditionally?

Yeah seems like that should be fine.

>
http://codereview.chromium.org/10873090/diff/6001/content/renderer/render_vie...
> content/renderer/render_view_impl.cc:1726: creator->consumeUserGesture();
> We need to do this both for OpenURL and createView ?  I would have hoped we
> would only need to do this in one place (likely here).  Also, this comment is
> kind of useless because it just restates what the code below it says.

Yes, unfortunately the behavior is different when we are creating a new process
(ie rel=noreferrer). The openURL call will be in the new process so we need to
check for a gesture (and consume it) before we allow it to be created.

I'll drop the comment and make the consumes unconditional.

[abarth-chromium, 2012-08-27 20:02:03]

That would be useful information to include in a comment.  :)

Maybe that means we should be consuming the gesture earlier?  It would make more
sense to me if we consumed the gesture in allowPopups (or whatever the function
is called that checks the user gesture state before allowing the popup).

[Cris Neckar, 2012-08-27 20:11:26]

On 2012/08/27 20:02:03, abarth wrote:
> That would be useful information to include in a comment.  :)
> 
> Maybe that means we should be consuming the gesture earlier?  It would make
more
> sense to me if we consumed the gesture in allowPopups (or whatever the
function
> is called that checks the user gesture state before allowing the popup).

Hrrm yeah not sure where a better place to do it would be. Charlie might have an
idea where the correct place to do this would be.

I could do it in AddWebContents() but I'm not sure if that will end up consuming
gestures when it isn't actually needed.

[abarth-chromium, 2012-08-27 20:13:05]

Where do we call isProcessingUserGesture?

[Cris Neckar, 2012-08-27 20:16:42]

On 2012/08/27 20:13:05, abarth wrote:
> Where do we call isProcessingUserGesture?

We set params.user_gesture for the message at line 1705. This is then checked
within the browser process.

[abarth-chromium, 2012-08-27 20:24:34]

> We set params.user_gesture for the message at line 1705. This is then checked
> within the browser process.

Ah, I see...  I presume we don't want to consume it on line 1705...

[Cris Neckar, 2012-08-27 20:34:22]

On 2012/08/27 20:24:34, abarth wrote:
> > We set params.user_gesture for the message at line 1705. This is then
checked
> > within the browser process.
> 
> Ah, I see...  I presume we don't want to consume it on line 1705...

Actually I think I might be able to only consume it in CreateView(). It seems to
get called in the relevant path for normal links as well so it was actually
consuming two in some cases. 

Do you think it is more clear if I move it up next to the
isProcessingUserGesture() check? It is a bit awkward there as its within the
initialization of the params for the host message.

[Charlie Reis, 2012-08-27 20:37:34]

On 2012/08/27 20:11:26, Cris Neckar wrote:
> On 2012/08/27 20:02:03, abarth wrote:
> > That would be useful information to include in a comment.  :)
> > 
> > Maybe that means we should be consuming the gesture earlier?  It would make
> more
> > sense to me if we consumed the gesture in allowPopups (or whatever the
> function
> > is called that checks the user gesture state before allowing the popup).
> 
> Hrrm yeah not sure where a better place to do it would be. Charlie might have
an
> idea where the correct place to do this would be.
> 
> I could do it in AddWebContents() but I'm not sure if that will end up
consuming
> gestures when it isn't actually needed.

Are you trying to consume the gesture in both the new and the old process?  If
you only care about the old process (the one that opened the window), then it
seems like createView should be sufficient.  If you need a consumeUserGesture
call in the new process as well, then it seems like you'll need two separate
calls, as you have now.

[abarth-chromium, 2012-08-27 20:38:20]

The old process is probably sufficient since the new process shouldn't be
processing a user gesture yet!

[Cris Neckar, 2012-08-27 20:46:33]

On 2012/08/27 20:38:20, abarth wrote:
> The old process is probably sufficient since the new process shouldn't be
> processing a user gesture yet!

Yeah that seems correct. I was confused by debugging with --single-process.
Uploading a new patch

[Cris Neckar, 2012-08-28 00:10:54]

Does this look reasonable now?

[abarth-chromium, 2012-08-28 00:18:45]

LGTM.  I would be nice to have a test.  Note: I'm not a reviewer.

[Cris Neckar, 2012-08-28 00:29:19]

On 2012/08/28 00:18:45, abarth wrote:
> LGTM.  I would be nice to have a test.  Note: I'm not a reviewer.

Thanks, Charlie is an owner I believe. I am planning on adding consumes to some
other things (fullscreen etc) so I can look at doing tests when I tackle that if
it's ok.

[Charlie Reis, 2012-08-28 01:21:42]

On 2012/08/28 00:29:19, Cris Neckar wrote:
> On 2012/08/28 00:18:45, abarth wrote:
> > LGTM.  I would be nice to have a test.  Note: I'm not a reviewer.
> 
> Thanks, Charlie is an owner I believe. I am planning on adding consumes to
some
> other things (fullscreen etc) so I can look at doing tests when I tackle that
if
> it's ok.

Yes, but please do add a test for this with that CL, since I'd hate to regress
this unintentionally.

LGTM.

[commit-bot: I haz the power, 2012-08-28 18:24:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/10873090/6003

[commit-bot: I haz the power, 2012-08-28 21:12:51]

Change committed as 153731