Reenable framebusting

Reenable framebusting

Change user gesture tracking from per-document to per-frame. This allows
user gesture state to survive navigation.

BUG=624061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2625773002
Cr-Commit-Position: refs/heads/master@{#444920}
Committed: https://chromium.googlesource.com/chromium/src/+/61835ae17fb417e72d42d7e4829c755c5abdb578

[Nate Chapin, 2017-01-10 20:34:55]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 20:35:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 22:58:39]

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

[commit-bot: I haz the power, 2017-01-10 22:58:40]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-01-11 18:59:36]

Description was changed from

==========
Reenable framebusting

BUG=
==========

to

==========
Reenable framebusting

Change user gesture tracking from per-document to per-frame. This allows
user gesture state to survive navigation. Note, however, that this
implementation does not propagate the state during a cross-process
navigation.

BUG=624061
==========

[Nate Chapin, 2017-01-11 19:01:39]

japhet@chromium.org changed reviewers:
+ dcheng@chromium.org, ojan@chromium.org

[Nate Chapin, 2017-01-11 19:01:40]

ojan: WDYT?
dcheng: PTAL, especially for the cross-process question.

https://codereview.chromium.org/2625773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2625773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.h:274: bool
m_hasReceivedUserGesture;
This won't propagate if this frame goes cross-process. I don't know how much of
a problem that is in practice, but it does mean their would be a behavior
difference between site isolation enabled and disabled.

[ojan, 2017-01-11 19:15:48]

https://codereview.chromium.org/2625773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2625773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.h:274: bool
m_hasReceivedUserGesture;
On 2017/01/11 at 19:01:40, Nate Chapin wrote:
> This won't propagate if this frame goes cross-process. I don't know how much
of a problem that is in practice, but it does mean their would be a behavior
difference between site isolation enabled and disabled.

I think we need to come up with a solution that is unaffected by site isolation.
Unfortunately, I can't think of anything other than just making this work
properly across processes. 

Also, is there a test for showing that you can navigate the top-level page after
the frame has been navigated?

[dcheng, 2017-01-12 10:41:00]

+alexmos who's done a lot of replication stuff

To make sure I'm understanding correctly, this is a one-way transformation -- it
can't ever be reset to false.

I would suggest something relatively simple like this:
- Initially, this is triggered by a Document.
- DocumentUserGestureToken can call up into FrameLoaderClient.
- FrameLoaderClientImpl will call Frame::setHasReceivedUserGesture().
- Frame::setHasReceivedUserGesture() will set the bit and propagate to its
parent, if any.
- FrameLoaderClientImpl will also forward it to content, which messages all the
RemoteFrames, which will similarly update the corresponding RemoteFrame and all
its ancestors.

I suggest going up to FrameLoaderClient, rather than having
LocalFrame::setHasReceivedUserGesture() do it directly: otherwise,
LocalFrame::setHasReceivedUserGesture() will need to know when to suppress calls
back into the embedder to avoid infinite notification loops.

Of course, if we need this to work on Frame not associated with a WebFrame, then
we can't do that and we'll need to figure out some way to suppress notification
loops (we have to do this for focus). I'm hoping we won't need this, but we do
have a few weird dummy/shadow pages being held throughout Blink.

Finally, we'll have to make sure this state is preserved on WebFrame::swap.

https://codereview.chromium.org/2625773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h (right):

https://codereview.chromium.org/2625773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:44:
document->frame()->setDocumentHasReceivedUserGesture();
Nit: perhaps remove 'document' from the name, since it's stick to frame now.

[Nate Chapin, 2017-01-12 21:37:54]

Description was changed from

==========
Reenable framebusting

Change user gesture tracking from per-document to per-frame. This allows
user gesture state to survive navigation. Note, however, that this
implementation does not propagate the state during a cross-process
navigation.

BUG=624061
==========

to

==========
Reenable framebusting

Change user gesture tracking from per-document to per-frame. This allows
user gesture state to survive navigation. Note, however, that this
implementation does not propagate the state during a cross-process
navigation.

BUG=624061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Nate Chapin, 2017-01-12 21:38:05]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 21:38:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 23:48:27]

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

[commit-bot: I haz the power, 2017-01-12 23:48:28]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Nate Chapin, 2017-01-13 19:41:50]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 19:42:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 22:31:30]

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

[commit-bot: I haz the power, 2017-01-13 22:31:31]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-01-18 20:20:46]

Ok, I think I've got the replumb done more-or-less as requested.

[Nate Chapin, 2017-01-18 20:21:37]

https://codereview.chromium.org/2625773002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h (right):

https://codereview.chromium.org/2625773002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:42:
document->frame()->setDocumentHasReceivedUserGesture();
Doing this here rather than in FrameLoaderClientImpl so that
DocumentUserGestureTokenTest still works, since it doesn't have a real
FrameLoaderClient.

[dcheng, 2017-01-19 06:15:11]

Overall, looks good.

I think there's one potential issue though: if we create a local frame or remote
frame in a new process, I think we won't propagate the bit over to the new
process. So I think we'd also want to record this bit in FrameReplicationState,
and then pass this bit down when constructing a new WebFrame via
WebLocalFrame::create, WebLocalFrame::createProvisional, WebRemoteFrame::create,
WebRemoteFrame::createLocalChild, and WebRemoteFrame::createRemoteChild...

FWIW, this seems a bit unscalable to me, and I do plan on revisiting the
replication at some point... but for now, it's what we have =/

[alexmos, 2017-01-19 17:53:17]

On 2017/01/19 06:15:11, dcheng wrote:
> Overall, looks good.
> 
> I think there's one potential issue though: if we create a local frame or
remote
> frame in a new process, I think we won't propagate the bit over to the new
> process. So I think we'd also want to record this bit in
FrameReplicationState,
> and then pass this bit down when constructing a new WebFrame via
> WebLocalFrame::create, WebLocalFrame::createProvisional,
WebRemoteFrame::create,
> WebRemoteFrame::createLocalChild, and WebRemoteFrame::createRemoteChild...
> 
> FWIW, this seems a bit unscalable to me, and I do plan on revisiting the
> replication at some point... but for now, it's what we have =/

+1 that this is a problem, but I don't think you'd need to pass it down in all
those creation places; you could just call setHasReceivedUserGesture() once in
RenderFrameProxy::SetReplicatedState and that should take care of all the cases.
 It actually shouldn't be that difficult: just record it in
FrameReplicationState in OnSetHasReceivedUserGesture on the browser side and set
the bit in SetReplicatedState().

What I'm not very clear about is whether this ever needs to become false.  Is
the intent of this that once the bit becomes true, it will stay that way for the
lifetime of the frame?

[alexmos, 2017-01-19 17:55:50]

On 2017/01/19 17:53:17, alexmos wrote:
> On 2017/01/19 06:15:11, dcheng wrote:
> > Overall, looks good.
> > 
> > I think there's one potential issue though: if we create a local frame or
> remote
> > frame in a new process, I think we won't propagate the bit over to the new
> > process. So I think we'd also want to record this bit in
> FrameReplicationState,
> > and then pass this bit down when constructing a new WebFrame via
> > WebLocalFrame::create, WebLocalFrame::createProvisional,
> WebRemoteFrame::create,
> > WebRemoteFrame::createLocalChild, and WebRemoteFrame::createRemoteChild...
> > 
> > FWIW, this seems a bit unscalable to me, and I do plan on revisiting the
> > replication at some point... but for now, it's what we have =/
> 
> +1 that this is a problem, but I don't think you'd need to pass it down in all
> those creation places; you could just call setHasReceivedUserGesture() once in
> RenderFrameProxy::SetReplicatedState and that should take care of all the
cases.
>  It actually shouldn't be that difficult: just record it in
> FrameReplicationState in OnSetHasReceivedUserGesture on the browser side and
set
> the bit in SetReplicatedState().

Argh, typed that too fast -- Daniel's right that you'd need a separate call to
also init
this for local frames.  So you'd probably also need to set it in
RenderFrameImpl::CreateFrame.  Yeah, this could definitely be simpler :/

[ojan, 2017-01-19 19:19:40]

As per the current design of the feature, we won't ever need to set it to false
again. That might change in the future, but we can reevaluate the design at that
point if we need to change it.

[Nate Chapin, 2017-01-19 20:40:32]

On 2017/01/19 06:15:11, dcheng wrote:
> Overall, looks good.
> 
> I think there's one potential issue though: if we create a local frame or
remote
> frame in a new process, I think we won't propagate the bit over to the new
> process. So I think we'd also want to record this bit in
FrameReplicationState,
> and then pass this bit down when constructing a new WebFrame via
> WebLocalFrame::create, WebLocalFrame::createProvisional,
WebRemoteFrame::create,
> WebRemoteFrame::createLocalChild, and WebRemoteFrame::createRemoteChild...
> 
> FWIW, this seems a bit unscalable to me, and I do plan on revisiting the
> replication at some point... but for now, it's what we have =/

Done. PTAL at 4->5 diffs.

[Nate Chapin, 2017-01-19 20:41:06]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 20:41:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-01-19 21:53:42]

I think PS5 fixes all the remote frame proxy cases. It doesn't address
createProvisional() directly, but in theory, we should get that for free, since
going remote -> local always involves going through WebFrame::swap(). When
swapped in, the provisional local frame will pick up the has user gesture bit
from the previous remote frame.

However, I think there's one code path that might not be covered.
1. Say that the frame that receives a user gesture in renderer A. It sends an
IPC to the browser to update the other renderers.
2. Simultaneously, a load commits in that frame, but in renderer B. The browser
notifies renderer B, which swaps the remote frame to a local frame. This local
frame won't have the user gesture bit set.
3. Now the browser process receives and handles the IPC from renderer A. But
because renderer B no longer has a RenderFrameProxyHost, just a RenderFrameHost,
we never update it.

[dcheng, 2017-01-19 21:54:18]

dcheng@chromium.org changed reviewers:
+ alexmos@chromium.org

[dcheng, 2017-01-19 21:54:19]

+alexmos again as well since rietveld randomly dropped some reviewers

[commit-bot: I haz the power, 2017-01-19 22:23:50]

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

[commit-bot: I haz the power, 2017-01-19 22:23:52]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-01-19 23:10:15]

We chatted and we believe that we actually want to ignore frame replication
state for things that aren't the active RFH anymore. Thus, since the long-term
plan is to drop these IPCs anyway, we're OK with 'eventual consistency' here for
now: the racing navigation might not have the correct state, but a subsequent
cross-process navigation will fix that.

LGTM

[alexmos, 2017-01-19 23:37:40]

content/ LGTM

[Nate Chapin, 2017-01-19 23:39:47]

Description was changed from

==========
Reenable framebusting

Change user gesture tracking from per-document to per-frame. This allows
user gesture state to survive navigation. Note, however, that this
implementation does not propagate the state during a cross-process
navigation.

BUG=624061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reenable framebusting

Change user gesture tracking from per-document to per-frame. This allows
user gesture state to survive navigation.

BUG=624061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Nate Chapin, 2017-01-19 23:39:55]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2017-01-19 23:40:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 01:25:43]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484869195707290,
"parent_rev": "a6dd3781121d6a697d9d525e9d965f38c72b8df0", "commit_rev":
"61835ae17fb417e72d42d7e4829c755c5abdb578"}

[commit-bot: I haz the power, 2017-01-20 01:26:41]

Description was changed from

==========
Reenable framebusting

Change user gesture tracking from per-document to per-frame. This allows
user gesture state to survive navigation.

BUG=624061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reenable framebusting

Change user gesture tracking from per-document to per-frame. This allows
user gesture state to survive navigation.

BUG=624061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2625773002
Cr-Commit-Position: refs/heads/master@{#444920}
Committed:
https://chromium.googlesource.com/chromium/src/+/61835ae17fb417e72d42d7e4829c...
==========

[commit-bot: I haz the power, 2017-01-20 01:26:43]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/61835ae17fb417e72d42d7e4829c...