Only use pending navigation params for browser-initiated navigations.

Only use pending navigation params for browser-initiated navigations.

BUG=597239
TEST=as in bug
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278
Cr-Commit-Position: refs/heads/master@{#393760}

[Avi (use Gerrit), 2016-05-12 13:12:33]

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

[commit-bot: I haz the power, 2016-05-12 13:12:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1976573002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1976573002/1

[commit-bot: I haz the power, 2016-05-12 13:35:26]

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

[commit-bot: I haz the power, 2016-05-12 13:35:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2016-05-12 14:15:56]

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

[commit-bot: I haz the power, 2016-05-12 14:16:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1976573002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1976573002/20001

[commit-bot: I haz the power, 2016-05-12 15:13:11]

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

[commit-bot: I haz the power, 2016-05-12 15:13:12]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-05-12 15:15:28]

avi@chromium.org changed reviewers:
+ creis@chromium.org, japhet@chromium.org

[Avi (use Gerrit), 2016-05-12 15:15:29]

Charlie: navigation in Chromium
Nate: navigation in Blink

[Avi (use Gerrit), 2016-05-12 18:33:07]

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

[commit-bot: I haz the power, 2016-05-12 18:33:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1976573002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1976573002/40001

[Nate Chapin, 2016-05-12 18:52:17]

blink LGTM

[Charlie Reis, 2016-05-12 20:51:47]

Could you write a test for this?  It'd be good to prevent regression, and it
would be nice documentation for why the old approach was wrong.

I'm curious about the "content-initiated" concept (which I hope is the same
thing as "renderer-initiated," since both terms get used a lot).  Is it always
that case that a lack of request.originDocument() implies the navigation was
browser-initiated?  If so, this looks great.

https://codereview.chromium.org/1976573002/diff/40001/chrome/renderer/safe_br...
File chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
(right):

https://codereview.chromium.org/1976573002/diff/40001/chrome/renderer/safe_br...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:254:
DidNavigateWithinPage(GetMainFrame(), true, false);
Just curious, was there a reason this needed to be false, or is it arbitrary for
this test?

https://codereview.chromium.org/1976573002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1976573002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:3583: if (content_initiated) {
Would it be better to put this fix inside UpdateNavigationState?  That method is
incorrectly assuming that the presence of pending_navigation_params_ means the
current navigation is browser-initiated, and it's called from more than just
this location (i.e., also didCreateDataSource).

Or do we not know if it's browser-initiated in didCreateDataSource?  (Could
there be a similar bug on that path?)

[commit-bot: I haz the power, 2016-05-12 21:11:16]

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

[commit-bot: I haz the power, 2016-05-12 21:11:16]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-05-12 23:41:17]

On 2016/05/12 20:51:47, Charlie Reis (slow to reply) wrote:
> Could you write a test for this?  It'd be good to prevent regression, and it
> would be nice documentation for why the old approach was wrong.

When I first looked at this, I tried and failed to get a test working for it.
The thing is that this scenario is almost DoReplaceStateWhilePending, but that
didn't die. Maybe if we put the replaceState in the unload handler?

Didn't you try to get a working test? I can look at Twitter to see if I can work
out what they do.

> I'm curious about the "content-initiated" concept (which I hope is the same
> thing as "renderer-initiated," since both terms get used a lot).  Is it always
> that case that a lack of request.originDocument() implies the navigation was
> browser-initiated?  If so, this looks great.

"content-initiated" == "renderer-initiated". I was struggling to find the right
naming, since referring to the "renderer" inside of Blink doesn't seem to be
done.

https://codereview.chromium.org/1976573002/diff/40001/chrome/renderer/safe_br...
File chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
(right):

https://codereview.chromium.org/1976573002/diff/40001/chrome/renderer/safe_br...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:254:
DidNavigateWithinPage(GetMainFrame(), true, false);
On 2016/05/12 20:51:47, Charlie Reis (slow to reply) wrote:
> Just curious, was there a reason this needed to be false, or is it arbitrary
for
> this test?

I kinda picked it arbitrarily.

https://codereview.chromium.org/1976573002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1976573002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:3583: if (content_initiated) {
On 2016/05/12 20:51:48, Charlie Reis (slow to reply) wrote:
> Or do we not know if it's browser-initiated in didCreateDataSource?  (Could
> there be a similar bug on that path?)

Right now we don't know if it's browser-initiated in didCreateDataSource. I
suppose there might be a similar bug there, but I'm straining to figure that
out.

It might be worth it to plumb that too, just to cover ourselves.

[Avi (use Gerrit), 2016-05-12 23:59:25]

On 2016/05/12 23:41:17, Avi wrote:
> Didn't you try to get a working test? I can look at Twitter to see if I can
work
> out what they do.

I see in the source for Twitter:

        this.destroyCurrentPageState = function() {
            history.replaceState(null , document.title, b)
        }
        // ...
        this.after("initialize", function() {
            // ...
            this.on(window, "beforeunload uiBeforeUnload",
this.destroyCurrentPageState)) : this.on("uiSwiftLoaded",
this.resetStateVariables)
        })

so maybe in the beforeunload? I'll work on that tomorrow.

[Charlie Reis, 2016-05-13 00:46:17]

On 2016/05/12 23:41:17, Avi wrote:
> On 2016/05/12 20:51:47, Charlie Reis (slow to reply) wrote:
> > Could you write a test for this?  It'd be good to prevent regression, and it
> > would be nice documentation for why the old approach was wrong.
> 
> When I first looked at this, I tried and failed to get a test working for it.
> The thing is that this scenario is almost DoReplaceStateWhilePending, but that
> didn't die. Maybe if we put the replaceState in the unload handler?
> 
> Didn't you try to get a working test? I can look at Twitter to see if I can
work
> out what they do.

> I see in the source for Twitter:
> 
>         this.destroyCurrentPageState = function() {
>             history.replaceState(null , document.title, b)
>         }
>         // ...
>         this.after("initialize", function() {
>             // ...
>             this.on(window, "beforeunload uiBeforeUnload",
> this.destroyCurrentPageState)) : this.on("uiSwiftLoaded",
> this.resetStateVariables)
>         })
> 
> so maybe in the beforeunload? I'll work on that tomorrow.

Ah, nice.  Yes, let's see if that works.

> > I'm curious about the "content-initiated" concept (which I hope is the same
> > thing as "renderer-initiated," since both terms get used a lot).  Is it
always
> > that case that a lack of request.originDocument() implies the navigation was
> > browser-initiated?  If so, this looks great.
> 
> "content-initiated" == "renderer-initiated". I was struggling to find the
right
> naming, since referring to the "renderer" inside of Blink doesn't seem to be
> done.

Thanks.

Nate, can you confirm that request.originDocument() is always present for
content-initiated navigations and never for browser-initiated navigations? 
That's the one assumption in this CL I'm not sure I know enough to answer.

https://codereview.chromium.org/1976573002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1976573002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:3583: if (content_initiated) {
On 2016/05/12 23:41:17, Avi wrote:
> On 2016/05/12 20:51:48, Charlie Reis (slow to reply) wrote:
> > Or do we not know if it's browser-initiated in didCreateDataSource?  (Could
> > there be a similar bug on that path?)
> 
> Right now we don't know if it's browser-initiated in didCreateDataSource. I
> suppose there might be a similar bug there, but I'm straining to figure that
> out.
> 
> It might be worth it to plumb that too, just to cover ourselves.

I could be ok leaving it as a TODO for this CL and fixing it next.  That said,
we probably still want to change UpdateNavigationState to take in this bool. 
Then didCreateDataSource can have a "bool content_initiated =
!!pending_nav_params_;" with a giant TODO over it saying that it's not a safe
assumption.

That way we know exactly where the remaining bug is, and any future callers of
UpdateNavigationState are forced to get it right.

[Avi (use Gerrit), 2016-05-13 01:35:44]

On 2016/05/13 00:46:17, Charlie Reis (slow to reply) wrote:
> Nate, can you confirm that request.originDocument() is always present for
> content-initiated navigations and never for browser-initiated navigations? 

Don't ask Nate; that was his idea and he spelled it out for me. If you're not
sure if this is the right thing, ask a _second_ Blink expert :)

https://codereview.chromium.org/1976573002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1976573002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:3583: if (content_initiated) {
On 2016/05/13 00:46:17, Charlie Reis (slow to reply) wrote:
> I could be ok leaving it as a TODO for this CL and fixing it next.

It's probably not difficult to plumb it too. I'll look at it after writing a
test. Perhaps you want it in its own CL anyway.

[Avi (use Gerrit), 2016-05-13 16:14:12]

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

[Avi (use Gerrit), 2016-05-13 16:14:15]

Description was changed from

==========
Only use pending navigation params for browser-initiated navigations.

BUG=597239
TEST=as in bug
==========

to

==========
Only use pending navigation params for browser-initiated navigations.

BUG=597239
TEST=as in bug
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-05-13 16:14:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1976573002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1976573002/60001

[Avi (use Gerrit), 2016-05-13 16:45:19]

https://codereview.chromium.org/1976573002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1976573002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:3583: if (content_initiated) {
On 2016/05/13 00:46:17, Charlie Reis (slow to reply) wrote:
> On 2016/05/12 23:41:17, Avi wrote:
> > On 2016/05/12 20:51:48, Charlie Reis (slow to reply) wrote:
> > > Or do we not know if it's browser-initiated in didCreateDataSource? 
(Could
> > > there be a similar bug on that path?)
> > 
> > Right now we don't know if it's browser-initiated in didCreateDataSource. I
> > suppose there might be a similar bug there, but I'm straining to figure that
> > out.
> > 
> > It might be worth it to plumb that too, just to cover ourselves.
> 
> I could be ok leaving it as a TODO for this CL and fixing it next.  That said,
> we probably still want to change UpdateNavigationState to take in this bool. 
> Then didCreateDataSource can have a "bool content_initiated =
> !!pending_nav_params_;" with a giant TODO over it saying that it's not a safe
> assumption.
> 
> That way we know exactly where the remaining bug is, and any future callers of
> UpdateNavigationState are forced to get it right.

FYI, at the beginning of RenderFrameImpl::didCreateDataSource it already makes
that EXACT ASSUMPTION:

void RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
                                          blink::WebDataSource* datasource) {
  DCHECK(!frame_ || frame_ == frame);

  bool content_initiated = !pending_navigation_params_.get();

[Charlie Reis, 2016-05-13 18:26:55]

LGTM!  Looks like you'll need a owner for the chrome/ browsertests.

On 2016/05/13 01:35:44, Avi wrote:
> On 2016/05/13 00:46:17, Charlie Reis (slow to reply) wrote:
> > Nate, can you confirm that request.originDocument() is always present for
> > content-initiated navigations and never for browser-initiated navigations? 
> 
> Don't ask Nate; that was his idea and he spelled it out for me. If you're not
> sure if this is the right thing, ask a _second_ Blink expert :)

If he's confident, that's good enough for me.  :)

https://codereview.chromium.org/1976573002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1976573002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:3583: if (content_initiated) {
On 2016/05/13 16:45:19, Avi wrote:
> On 2016/05/13 00:46:17, Charlie Reis (slow to reply) wrote:
> > On 2016/05/12 23:41:17, Avi wrote:
> > > On 2016/05/12 20:51:48, Charlie Reis (slow to reply) wrote:
> > > > Or do we not know if it's browser-initiated in didCreateDataSource? 
> (Could
> > > > there be a similar bug on that path?)
> > > 
> > > Right now we don't know if it's browser-initiated in didCreateDataSource.
I
> > > suppose there might be a similar bug there, but I'm straining to figure
that
> > > out.
> > > 
> > > It might be worth it to plumb that too, just to cover ourselves.
> > 
> > I could be ok leaving it as a TODO for this CL and fixing it next.  That
said,
> > we probably still want to change UpdateNavigationState to take in this bool.

> > Then didCreateDataSource can have a "bool content_initiated =
> > !!pending_nav_params_;" with a giant TODO over it saying that it's not a
safe
> > assumption.
> > 
> > That way we know exactly where the remaining bug is, and any future callers
of
> > UpdateNavigationState are forced to get it right.
> 
> FYI, at the beginning of RenderFrameImpl::didCreateDataSource it already makes
> that EXACT ASSUMPTION:
> 
> void RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
>                                           blink::WebDataSource* datasource) {
>   DCHECK(!frame_ || frame_ == frame);
> 
>   bool content_initiated = !pending_navigation_params_.get();

Ha!  Ok, yes, let's revisit that case (and any others) after this lands.  This
CL is strictly better than what we have today.

[Nate Chapin, 2016-05-13 18:28:29]

On 2016/05/13 18:26:55, Charlie Reis (slow to reply) wrote:
> LGTM!  Looks like you'll need a owner for the chrome/ browsertests.
> 
> On 2016/05/13 01:35:44, Avi wrote:
> > On 2016/05/13 00:46:17, Charlie Reis (slow to reply) wrote:
> > > Nate, can you confirm that request.originDocument() is always present for
> > > content-initiated navigations and never for browser-initiated navigations?

> > 
> > Don't ask Nate; that was his idea and he spelled it out for me. If you're
not
> > sure if this is the right thing, ask a _second_ Blink expert :)
> 
> If he's confident, that's good enough for me.  :)

It depends a little bit on your definition of content-initiated and
renderer-initiated. I believe originDocument() may be null in some cases where
there aren't associated pending_navigation_params_, but in
those cases there isn't a RenderFrameImpl either (e.g., SVGImage).

> 
>
https://codereview.chromium.org/1976573002/diff/40001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/1976573002/diff/40001/content/renderer/render...
> content/renderer/render_frame_impl.cc:3583: if (content_initiated) {
> On 2016/05/13 16:45:19, Avi wrote:
> > On 2016/05/13 00:46:17, Charlie Reis (slow to reply) wrote:
> > > On 2016/05/12 23:41:17, Avi wrote:
> > > > On 2016/05/12 20:51:48, Charlie Reis (slow to reply) wrote:
> > > > > Or do we not know if it's browser-initiated in didCreateDataSource? 
> > (Could
> > > > > there be a similar bug on that path?)
> > > > 
> > > > Right now we don't know if it's browser-initiated in
didCreateDataSource.
> I
> > > > suppose there might be a similar bug there, but I'm straining to figure
> that
> > > > out.
> > > > 
> > > > It might be worth it to plumb that too, just to cover ourselves.
> > > 
> > > I could be ok leaving it as a TODO for this CL and fixing it next.  That
> said,
> > > we probably still want to change UpdateNavigationState to take in this
bool.
> 
> > > Then didCreateDataSource can have a "bool content_initiated =
> > > !!pending_nav_params_;" with a giant TODO over it saying that it's not a
> safe
> > > assumption.
> > > 
> > > That way we know exactly where the remaining bug is, and any future
callers
> of
> > > UpdateNavigationState are forced to get it right.
> > 
> > FYI, at the beginning of RenderFrameImpl::didCreateDataSource it already
makes
> > that EXACT ASSUMPTION:
> > 
> > void RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
> >                                           blink::WebDataSource* datasource)
{
> >   DCHECK(!frame_ || frame_ == frame);
> > 
> >   bool content_initiated = !pending_navigation_params_.get();
> 
> Ha!  Ok, yes, let's revisit that case (and any others) after this lands.  This
> CL is strictly better than what we have today.

[Avi (use Gerrit), 2016-05-13 18:31:23]

On 2016/05/13 18:26:55, Charlie Reis (slow to reply) wrote:
> LGTM!  Looks like you'll need a owner for the chrome/ browsertests.

Right, but I wanted to get the approval for the real code first.

[Avi (use Gerrit), 2016-05-13 18:34:59]

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

[commit-bot: I haz the power, 2016-05-13 18:35:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1976573002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1976573002/80001

[Avi (use Gerrit), 2016-05-13 18:36:06]

avi@chromium.org changed reviewers:
+ mattm@chromium.org, thestig@chromium.org

[Avi (use Gerrit), 2016-05-13 18:36:07]

mattm: phishing_classifier_delegate_browsertest.cc
thestig: content_settings_observer_browsertest.cc

[mattm, 2016-05-13 19:43:03]

lgtm

[Lei Zhang, 2016-05-13 20:14:09]

lgtm

[Avi (use Gerrit), 2016-05-13 21:57:16]

The CQ bit was unchecked by avi@chromium.org

[Avi (use Gerrit), 2016-05-14 06:20:40]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-05-14 06:20:41]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1976573002/#ps80001
(title: "consistency")

[commit-bot: I haz the power, 2016-05-14 06:20:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1976573002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1976573002/80001

[commit-bot: I haz the power, 2016-05-15 01:46:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-15 01:47:55]

Description was changed from

==========
Only use pending navigation params for browser-initiated navigations.

BUG=597239
TEST=as in bug
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Only use pending navigation params for browser-initiated navigations.

BUG=597239
TEST=as in bug
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278
Cr-Commit-Position: refs/heads/master@{#393760}
==========

[commit-bot: I haz the power, 2016-05-15 01:47:56]

Patchset 5 (id:??) landed as
https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278
Cr-Commit-Position: refs/heads/master@{#393760}