PlzNavigate: Fix the http/tests/security/cross-frame-access-parent-isolated-world.html and http/tes…

PlzNavigate: Fix the http/tests/security/cross-frame-access-parent-isolated-world.html and http/tests/security/cross-frame-access-parent-explicit-domain-isolated-world.html layout tests.

These tests instantiate an iframe with the following src "data:text/html," and then later
on switch the iframe src to a different URL to run the security tests.

With PlzNavigate when the iframe is instantiated, we send the navigation off to the browser
via RenderFrameImpl::BeginNavigation. This function gets called in the decidePolicyForNavigation code path. When this
returns blink eventually schedules a task for switching the navigation src url for the iframe via a call to
the NavigationScheduler::scheduleLocationChange which is called in the LocalFrame::navigate function. Before this scheduled
task runs, the CommitNavigation IPC comes back from the browser and the original load completes for the iframe which cancels
pending navigations on the iframe. As a result the task never runs and the test times out.
Without PlzNavigate this does not happen as the original load on the iframe is completed by the time the task runs.

It looks like fixing the layout test itself to switch the src on the iframe after it finishes loading is one approach. Alternatively we could avoid cancelling pending navigations for plznavigate. 
Exceptions being user initiated navigations.

BUG=673748
Review-Url: https://codereview.chromium.org/2630783003
Cr-Commit-Position: refs/heads/master@{#445174}
Committed: https://chromium.googlesource.com/chromium/src/+/f59dc823911d1e7fca195b638638e8458c631491

[ananta, 2017-01-14 02:09:48]

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

[ananta, 2017-01-14 02:10:01]

Description was changed from

==========
PlzNavigate: Fix the
http/tests/security/cross-frame-access-parent-isolated-world.html and
http/tests/security/cross-frame-access-parent-explicit-domain-isolated-world.html
layout tests.

These tests instantiate an iframe with the following src "data:text/html," and
then later
on switch the iframe src to a different URL to run the security tests.

With PlzNavigate when the iframe is instantiated, we send the navigation off to
the browser
via RenderFrameImpl::BeginNavigation. This function gets called in the
decidePolicyForNavigation code path. When this
returns blink eventually schedules a task for switching the navigation src url
for the iframe via a call to
the NavigationScheduler::scheduleLocationChange which is called in the
LocalFrame::navigate function. Before this scheduled
task runs, the CommitNavigation IPC comes back from the browser and the original
load completes for the iframe which cancles
pending navigations on the iframe. As a result the task never runs and the test
times out.
Without PlzNavigate this does not happen as the original load on the iframe is
completed by the time the task runs.

It looks like fixing the layout test itself to switch the src on the iframe
after it finishes loading is the correct way to go..
Alternatively we could remember the navigation changes in a list and run them
after the original request completes. From a brief perusal of
the code, there may be some corner cases which could be triggered here. Not sure
if such cases could happen in the field.

BUG=673748
==========

to

==========
PlzNavigate: Fix the
http/tests/security/cross-frame-access-parent-isolated-world.html and
http/tests/security/cross-frame-access-parent-explicit-domain-isolated-world.html
layout tests.

These tests instantiate an iframe with the following src "data:text/html," and
then later
on switch the iframe src to a different URL to run the security tests.

With PlzNavigate when the iframe is instantiated, we send the navigation off to
the browser
via RenderFrameImpl::BeginNavigation. This function gets called in the
decidePolicyForNavigation code path. When this
returns blink eventually schedules a task for switching the navigation src url
for the iframe via a call to
the NavigationScheduler::scheduleLocationChange which is called in the
LocalFrame::navigate function. Before this scheduled
task runs, the CommitNavigation IPC comes back from the browser and the original
load completes for the iframe which cancles
pending navigations on the iframe. As a result the task never runs and the test
times out.
Without PlzNavigate this does not happen as the original load on the iframe is
completed by the time the task runs.

It looks like fixing the layout test itself to switch the src on the iframe
after it finishes loading is the correct way to go..
Alternatively we could remember the navigation changes in a list and run them
after the original request completes. From a brief perusal of
the code, there may be some corner cases which could be triggered here. Not sure
if such cases could happen in the field.

BUG=673748
==========

[ananta, 2017-01-14 02:10:02]

ananta@chromium.org changed reviewers:
+ jam@chromium.org

[ananta, 2017-01-14 02:10:12]

+jam

[ananta, 2017-01-14 02:10:28]

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

[commit-bot: I haz the power, 2017-01-14 02:10:46]

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

[ananta, 2017-01-14 02:24:55]

https://codereview.chromium.org/2630783003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/cross-frame-access-parent-explicit-domain-isolated-world.html
(right):

https://codereview.chromium.org/2630783003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/cross-frame-access-parent-explicit-domain-isolated-world.html:12:
var srcChanged = false;
Should this function be moved to js-test.js or another common place?. It could
take in the src and possibly the
origin as parameters.

[ananta, 2017-01-14 02:59:45]

Description was changed from

==========
PlzNavigate: Fix the
http/tests/security/cross-frame-access-parent-isolated-world.html and
http/tests/security/cross-frame-access-parent-explicit-domain-isolated-world.html
layout tests.

These tests instantiate an iframe with the following src "data:text/html," and
then later
on switch the iframe src to a different URL to run the security tests.

With PlzNavigate when the iframe is instantiated, we send the navigation off to
the browser
via RenderFrameImpl::BeginNavigation. This function gets called in the
decidePolicyForNavigation code path. When this
returns blink eventually schedules a task for switching the navigation src url
for the iframe via a call to
the NavigationScheduler::scheduleLocationChange which is called in the
LocalFrame::navigate function. Before this scheduled
task runs, the CommitNavigation IPC comes back from the browser and the original
load completes for the iframe which cancles
pending navigations on the iframe. As a result the task never runs and the test
times out.
Without PlzNavigate this does not happen as the original load on the iframe is
completed by the time the task runs.

It looks like fixing the layout test itself to switch the src on the iframe
after it finishes loading is the correct way to go..
Alternatively we could remember the navigation changes in a list and run them
after the original request completes. From a brief perusal of
the code, there may be some corner cases which could be triggered here. Not sure
if such cases could happen in the field.

BUG=673748
==========

to

==========
PlzNavigate: Fix the
http/tests/security/cross-frame-access-parent-isolated-world.html and
http/tests/security/cross-frame-access-parent-explicit-domain-isolated-world.html
layout tests.

These tests instantiate an iframe with the following src "data:text/html," and
then later
on switch the iframe src to a different URL to run the security tests.

With PlzNavigate when the iframe is instantiated, we send the navigation off to
the browser
via RenderFrameImpl::BeginNavigation. This function gets called in the
decidePolicyForNavigation code path. When this
returns blink eventually schedules a task for switching the navigation src url
for the iframe via a call to
the NavigationScheduler::scheduleLocationChange which is called in the
LocalFrame::navigate function. Before this scheduled
task runs, the CommitNavigation IPC comes back from the browser and the original
load completes for the iframe which cancels
pending navigations on the iframe. As a result the task never runs and the test
times out.
Without PlzNavigate this does not happen as the original load on the iframe is
completed by the time the task runs.

It looks like fixing the layout test itself to switch the src on the iframe
after it finishes loading is the correct way to go..
Alternatively we could remember the navigation changes in a list and run them
after the original request completes. From a brief perusal of
the code, there may be some corner cases which could be triggered here. Not sure
if such cases could happen in the field.

BUG=673748
==========

[commit-bot: I haz the power, 2017-01-14 03:20:34]

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

[commit-bot: I haz the power, 2017-01-14 03:20:35]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-01-17 19:22:31]

[+clamy,nasko to CC]

I'll defer to Nate on most of the CL, but a few thoughts...

The basic problem seems to be that PlzNavigate is introducing a race for data
URLs, where they now go up to the browser process and may clobber subsequent
navigations when they come back down.  That seems possibly concerning in
practice, not just something we should avoid doing in layout tests.  There could
be real pages (e.g., ads) that do this same thing, and PlzNavigate might break
them.

(If we weren't worried about that, it seems like we could just change the test
URL from data:text/html, to about:blank to avoid the problem?)

Camille or Nasko, is there a potential bug here that needs investigation?

[clamy, 2017-01-18 14:48:09]

On 2017/01/17 19:22:31, Charlie Reis wrote:
> [+clamy,nasko to CC]
> 
> I'll defer to Nate on most of the CL, but a few thoughts...
> 
> The basic problem seems to be that PlzNavigate is introducing a race for data
> URLs, where they now go up to the browser process and may clobber subsequent
> navigations when they come back down.  That seems possibly concerning in
> practice, not just something we should avoid doing in layout tests.  There
could
> be real pages (e.g., ads) that do this same thing, and PlzNavigate might break
> them.
> 
> (If we weren't worried about that, it seems like we could just change the test
> URL from data:text/html, to about:blank to avoid the problem?)
> 
> Camille or Nasko, is there a potential bug here that needs investigation?

Switching to about:blank should indeed fix the issue.

I'd be interested to know how this test work on Android. AFAIR all data urls in
Android are sent to the browser and do not commit synchronously either. This
caused issue with NavigationHandles and interstitials in the past. This means
that there is no requirement that data urls commit in a synchronous manner. I
imagine that in the Android case, the asynchronous data url navigation gets
cancelled by the following location.replace. I imagine this is not the case on
PlzNavigate, possibly because the data url navigation could be marked as having
a user gesture while the latter location.replace doesn't, so we would favor the
first one.

That said if a website is relying on fixed behavior between two concurrent
non-synchronous navigations there's bound to be issues IMO.

[ananta, 2017-01-18 19:59:36]

On 2017/01/18 14:48:09, clamy wrote:
> On 2017/01/17 19:22:31, Charlie Reis wrote:
> > [+clamy,nasko to CC]
> > 
> > I'll defer to Nate on most of the CL, but a few thoughts...
> > 
> > The basic problem seems to be that PlzNavigate is introducing a race for
data
> > URLs, where they now go up to the browser process and may clobber subsequent
> > navigations when they come back down.  That seems possibly concerning in
> > practice, not just something we should avoid doing in layout tests.  There
> could
> > be real pages (e.g., ads) that do this same thing, and PlzNavigate might
break
> > them.
> > 
> > (If we weren't worried about that, it seems like we could just change the
test
> > URL from data:text/html, to about:blank to avoid the problem?)
> > 
> > Camille or Nasko, is there a potential bug here that needs investigation?
> 
> Switching to about:blank should indeed fix the issue.
> 
> I'd be interested to know how this test work on Android. AFAIR all data urls
in
> Android are sent to the browser and do not commit synchronously either. This
> caused issue with NavigationHandles and interstitials in the past. This means
> that there is no requirement that data urls commit in a synchronous manner. I
> imagine that in the Android case, the asynchronous data url navigation gets
> cancelled by the following location.replace. I imagine this is not the case on
> PlzNavigate, possibly because the data url navigation could be marked as
having
> a user gesture while the latter location.replace doesn't, so we would favor
the
> first one.
> 
> That said if a website is relying on fixed behavior between two concurrent
> non-synchronous navigations there's bound to be issues IMO.

Looking at the code for Android the data urls appear to be handled here. 
https://cs.chromium.org/chromium/src/content/child/web_url_loader_impl.cc?dr=...
The request is then sent off to the browser. When the src url is switched to
something else, the original request would be cancelled in blink which would
explain why it works on Android. With PlzNavigate the top level frame
navigations are routed to the browser which does not know about subsequent src
url changes in blink. Perhaps we need to add a mechanism to cancel ongoing
navigations?

[Nate Chapin, 2017-01-18 22:14:11]

lgtm

https://codereview.chromium.org/2630783003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/cross-frame-access-parent-explicit-domain-isolated-world.html
(right):

https://codereview.chromium.org/2630783003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/cross-frame-access-parent-explicit-domain-isolated-world.html:12:
var srcChanged = false;
On 2017/01/14 02:24:55, ananta wrote:
> Should this function be moved to js-test.js or another common place?. It could
> take in the src and possibly the
> origin as parameters.

I don't feel strongly, do you see other tests that are likely to need this
trick?

[Charlie Reis, 2017-01-18 23:07:21]

On 2017/01/18 19:59:36, ananta wrote:
> On 2017/01/18 14:48:09, clamy wrote:
> > On 2017/01/17 19:22:31, Charlie Reis wrote:
> > > [+clamy,nasko to CC]
> > > 
> > > I'll defer to Nate on most of the CL, but a few thoughts...
> > > 
> > > The basic problem seems to be that PlzNavigate is introducing a race for
> data
> > > URLs, where they now go up to the browser process and may clobber
subsequent
> > > navigations when they come back down.  That seems possibly concerning in
> > > practice, not just something we should avoid doing in layout tests.  There
> > could
> > > be real pages (e.g., ads) that do this same thing, and PlzNavigate might
> break
> > > them.
> > > 
> > > (If we weren't worried about that, it seems like we could just change the
> test
> > > URL from data:text/html, to about:blank to avoid the problem?)
> > > 
> > > Camille or Nasko, is there a potential bug here that needs investigation?
> > 
> > Switching to about:blank should indeed fix the issue.
> > 
> > I'd be interested to know how this test work on Android. AFAIR all data urls
> in
> > Android are sent to the browser and do not commit synchronously either. This
> > caused issue with NavigationHandles and interstitials in the past. This
means
> > that there is no requirement that data urls commit in a synchronous manner.
I
> > imagine that in the Android case, the asynchronous data url navigation gets
> > cancelled by the following location.replace. I imagine this is not the case
on
> > PlzNavigate, possibly because the data url navigation could be marked as
> having
> > a user gesture while the latter location.replace doesn't, so we would favor
> the
> > first one.
> > 
> > That said if a website is relying on fixed behavior between two concurrent
> > non-synchronous navigations there's bound to be issues IMO.
> 
> Looking at the code for Android the data urls appear to be handled here. 
>
https://cs.chromium.org/chromium/src/content/child/web_url_loader_impl.cc?dr=...
> The request is then sent off to the browser. When the src url is switched to
> something else, the original request would be cancelled in blink which would
> explain why it works on Android. With PlzNavigate the top level frame
> navigations are routed to the browser which does not know about subsequent src
> url changes in blink. Perhaps we need to add a mechanism to cancel ongoing
> navigations?

Camille was describing the mechanism we already have in PlzNavigate, which takes
into account whether there was a user gesture or not.

I'd feel better about this change if the existing layout tests were flaky in
non-PlzNavigate, given a race between the data URL and the subsequent URL.  It
sounds like that's not the case, likely because the subsequent navigation
cancels the data URL navigation today if they overlap.  I think there's a
fundamental change that PlzNavigate is making here by not having the second
navigation cancel the first (which would probably be hard to pull off anyway due
to the race between processes).

Anyway, I won't stand in the way if you want to proceed with this, since I don't
see a way to avoid the race in PlzNavigate.  Still, I think you'll want to be
very careful about introducing bugs like https://crbug.com/657896 (Pri-0) when
PlzNavigate gets turned on-- any page that starts its iframe at a data URL and
then navigates it to a real URL is at risk of breaking.  Maybe it's worth
collecting metrics to see if you need to address it?

[nasko, 2017-01-19 00:04:24]

On 2017/01/18 23:07:21, Charlie Reis wrote:
> On 2017/01/18 19:59:36, ananta wrote:
> > On 2017/01/18 14:48:09, clamy wrote:
> > > On 2017/01/17 19:22:31, Charlie Reis wrote:
> > > > [+clamy,nasko to CC]
> > > > 
> > > > I'll defer to Nate on most of the CL, but a few thoughts...
> > > > 
> > > > The basic problem seems to be that PlzNavigate is introducing a race for
> > data
> > > > URLs, where they now go up to the browser process and may clobber
> subsequent
> > > > navigations when they come back down.  That seems possibly concerning in
> > > > practice, not just something we should avoid doing in layout tests. 
There
> > > could
> > > > be real pages (e.g., ads) that do this same thing, and PlzNavigate might
> > break
> > > > them.
> > > > 
> > > > (If we weren't worried about that, it seems like we could just change
the
> > test
> > > > URL from data:text/html, to about:blank to avoid the problem?)
> > > > 
> > > > Camille or Nasko, is there a potential bug here that needs
investigation?
> > > 
> > > Switching to about:blank should indeed fix the issue.
> > > 
> > > I'd be interested to know how this test work on Android. AFAIR all data
urls
> > in
> > > Android are sent to the browser and do not commit synchronously either.
This
> > > caused issue with NavigationHandles and interstitials in the past. This
> means
> > > that there is no requirement that data urls commit in a synchronous
manner.
> I
> > > imagine that in the Android case, the asynchronous data url navigation
gets
> > > cancelled by the following location.replace. I imagine this is not the
case
> on
> > > PlzNavigate, possibly because the data url navigation could be marked as
> > having
> > > a user gesture while the latter location.replace doesn't, so we would
favor
> > the
> > > first one.
> > > 
> > > That said if a website is relying on fixed behavior between two concurrent
> > > non-synchronous navigations there's bound to be issues IMO.
> > 
> > Looking at the code for Android the data urls appear to be handled here. 
> >
>
https://cs.chromium.org/chromium/src/content/child/web_url_loader_impl.cc?dr=...
> > The request is then sent off to the browser. When the src url is switched to
> > something else, the original request would be cancelled in blink which would
> > explain why it works on Android. With PlzNavigate the top level frame
> > navigations are routed to the browser which does not know about subsequent
src
> > url changes in blink. Perhaps we need to add a mechanism to cancel ongoing
> > navigations?
> 
> Camille was describing the mechanism we already have in PlzNavigate, which
takes
> into account whether there was a user gesture or not.
> 
> I'd feel better about this change if the existing layout tests were flaky in
> non-PlzNavigate, given a race between the data URL and the subsequent URL.  It
> sounds like that's not the case, likely because the subsequent navigation
> cancels the data URL navigation today if they overlap.  I think there's a
> fundamental change that PlzNavigate is making here by not having the second
> navigation cancel the first (which would probably be hard to pull off anyway
due
> to the race between processes).

I'm also very curious why the data URL navigation isn't cancelled by the
subsequent navigation. Aren't both of them lacking user gestures, at which point
we should be in the case where the last one wins. Or am I missing some detail?

> Anyway, I won't stand in the way if you want to proceed with this, since I
don't
> see a way to avoid the race in PlzNavigate.  Still, I think you'll want to be
> very careful about introducing bugs like https://crbug.com/657896 (Pri-0) when
> PlzNavigate gets turned on-- any page that starts its iframe at a data URL and
> then navigates it to a real URL is at risk of breaking.  Maybe it's worth
> collecting metrics to see if you need to address it?

I'd like to understand this case a bit better too before calling it "fixed".

[ananta, 2017-01-19 00:47:19]

On 2017/01/19 00:04:24, nasko wrote:
> On 2017/01/18 23:07:21, Charlie Reis wrote:
> > On 2017/01/18 19:59:36, ananta wrote:
> > > On 2017/01/18 14:48:09, clamy wrote:
> > > > On 2017/01/17 19:22:31, Charlie Reis wrote:
> > > > > [+clamy,nasko to CC]
> > > > > 
> > > > > I'll defer to Nate on most of the CL, but a few thoughts...
> > > > > 
> > > > > The basic problem seems to be that PlzNavigate is introducing a race
for
> > > data
> > > > > URLs, where they now go up to the browser process and may clobber
> > subsequent
> > > > > navigations when they come back down.  That seems possibly concerning
in
> > > > > practice, not just something we should avoid doing in layout tests. 
> There
> > > > could
> > > > > be real pages (e.g., ads) that do this same thing, and PlzNavigate
might
> > > break
> > > > > them.
> > > > > 
> > > > > (If we weren't worried about that, it seems like we could just change
> the
> > > test
> > > > > URL from data:text/html, to about:blank to avoid the problem?)
> > > > > 
> > > > > Camille or Nasko, is there a potential bug here that needs
> investigation?
> > > > 
> > > > Switching to about:blank should indeed fix the issue.
> > > > 
> > > > I'd be interested to know how this test work on Android. AFAIR all data
> urls
> > > in
> > > > Android are sent to the browser and do not commit synchronously either.
> This
> > > > caused issue with NavigationHandles and interstitials in the past. This
> > means
> > > > that there is no requirement that data urls commit in a synchronous
> manner.
> > I
> > > > imagine that in the Android case, the asynchronous data url navigation
> gets
> > > > cancelled by the following location.replace. I imagine this is not the
> case
> > on
> > > > PlzNavigate, possibly because the data url navigation could be marked as
> > > having
> > > > a user gesture while the latter location.replace doesn't, so we would
> favor
> > > the
> > > > first one.
> > > > 
> > > > That said if a website is relying on fixed behavior between two
concurrent
> > > > non-synchronous navigations there's bound to be issues IMO.
> > > 
> > > Looking at the code for Android the data urls appear to be handled here. 
> > >
> >
>
https://cs.chromium.org/chromium/src/content/child/web_url_loader_impl.cc?dr=...
> > > The request is then sent off to the browser. When the src url is switched
to
> > > something else, the original request would be cancelled in blink which
would
> > > explain why it works on Android. With PlzNavigate the top level frame
> > > navigations are routed to the browser which does not know about subsequent
> src
> > > url changes in blink. Perhaps we need to add a mechanism to cancel ongoing
> > > navigations?
> > 
> > Camille was describing the mechanism we already have in PlzNavigate, which
> takes
> > into account whether there was a user gesture or not.
> > 
> > I'd feel better about this change if the existing layout tests were flaky in
> > non-PlzNavigate, given a race between the data URL and the subsequent URL. 
It
> > sounds like that's not the case, likely because the subsequent navigation
> > cancels the data URL navigation today if they overlap.  I think there's a
> > fundamental change that PlzNavigate is making here by not having the second
> > navigation cancel the first (which would probably be hard to pull off anyway
> due
> > to the race between processes).
> 
> I'm also very curious why the data URL navigation isn't cancelled by the
> subsequent navigation. Aren't both of them lacking user gestures, at which
point
> we should be in the case where the last one wins. Or am I missing some detail?
> 
> > Anyway, I won't stand in the way if you want to proceed with this, since I
> don't
> > see a way to avoid the race in PlzNavigate.  Still, I think you'll want to
be
> > very careful about introducing bugs like https://crbug.com/657896 (Pri-0)
when
> > PlzNavigate gets turned on-- any page that starts its iframe at a data URL
and
> > then navigates it to a real URL is at risk of breaking.  Maybe it's worth
> > collecting metrics to see if you need to address it?
> 
> I'd like to understand this case a bit better too before calling it "fixed".

The timing issue introduced by PlzNavigate is as below:
1. A navigation is initiated in blink for a frame.
2. In RenderFrameImpl::decidePolicyForNavigation we route this to the browser
via BeginNavigation.
3. A script then sets the frame src to another url. This schedules a task.
NavigationScheduler::scheduleLocationChange().
4. Before this task runs, the commit navigation IPC is processed. This
eventually calls FrameLoader::startLoad() which
   cancels scheduled navigations.
5. This causes scheduled navigations to not take effect.

Without PlzNavigate, that won't happen because the timing window is much smaller
and the navigation scheduler task gets to
run predictably.

4.

[ananta, 2017-01-19 00:53:29]

Description was changed from

==========
PlzNavigate: Fix the
http/tests/security/cross-frame-access-parent-isolated-world.html and
http/tests/security/cross-frame-access-parent-explicit-domain-isolated-world.html
layout tests.

These tests instantiate an iframe with the following src "data:text/html," and
then later
on switch the iframe src to a different URL to run the security tests.

With PlzNavigate when the iframe is instantiated, we send the navigation off to
the browser
via RenderFrameImpl::BeginNavigation. This function gets called in the
decidePolicyForNavigation code path. When this
returns blink eventually schedules a task for switching the navigation src url
for the iframe via a call to
the NavigationScheduler::scheduleLocationChange which is called in the
LocalFrame::navigate function. Before this scheduled
task runs, the CommitNavigation IPC comes back from the browser and the original
load completes for the iframe which cancels
pending navigations on the iframe. As a result the task never runs and the test
times out.
Without PlzNavigate this does not happen as the original load on the iframe is
completed by the time the task runs.

It looks like fixing the layout test itself to switch the src on the iframe
after it finishes loading is the correct way to go..
Alternatively we could remember the navigation changes in a list and run them
after the original request completes. From a brief perusal of
the code, there may be some corner cases which could be triggered here. Not sure
if such cases could happen in the field.

BUG=673748
==========

to

==========
PlzNavigate: Fix the
http/tests/security/cross-frame-access-parent-isolated-world.html and
http/tests/security/cross-frame-access-parent-explicit-domain-isolated-world.html
layout tests.

These tests instantiate an iframe with the following src "data:text/html," and
then later
on switch the iframe src to a different URL to run the security tests.

With PlzNavigate when the iframe is instantiated, we send the navigation off to
the browser
via RenderFrameImpl::BeginNavigation. This function gets called in the
decidePolicyForNavigation code path. When this
returns blink eventually schedules a task for switching the navigation src url
for the iframe via a call to
the NavigationScheduler::scheduleLocationChange which is called in the
LocalFrame::navigate function. Before this scheduled
task runs, the CommitNavigation IPC comes back from the browser and the original
load completes for the iframe which cancels
pending navigations on the iframe. As a result the task never runs and the test
times out.
Without PlzNavigate this does not happen as the original load on the iframe is
completed by the time the task runs.

It looks like fixing the layout test itself to switch the src on the iframe
after it finishes loading is one approach. Alternatively we could avoid
cancelling pending navigations for plznavigate. 
Exceptions being user initiated navigations.

BUG=673748
==========

[ananta, 2017-01-19 00:54:19]

On 2017/01/19 00:47:19, ananta wrote:
> On 2017/01/19 00:04:24, nasko wrote:
> > On 2017/01/18 23:07:21, Charlie Reis wrote:
> > > On 2017/01/18 19:59:36, ananta wrote:
> > > > On 2017/01/18 14:48:09, clamy wrote:
> > > > > On 2017/01/17 19:22:31, Charlie Reis wrote:
> > > > > > [+clamy,nasko to CC]
> > > > > > 
> > > > > > I'll defer to Nate on most of the CL, but a few thoughts...
> > > > > > 
> > > > > > The basic problem seems to be that PlzNavigate is introducing a race
> for
> > > > data
> > > > > > URLs, where they now go up to the browser process and may clobber
> > > subsequent
> > > > > > navigations when they come back down.  That seems possibly
concerning
> in
> > > > > > practice, not just something we should avoid doing in layout tests. 
> > There
> > > > > could
> > > > > > be real pages (e.g., ads) that do this same thing, and PlzNavigate
> might
> > > > break
> > > > > > them.
> > > > > > 
> > > > > > (If we weren't worried about that, it seems like we could just
change
> > the
> > > > test
> > > > > > URL from data:text/html, to about:blank to avoid the problem?)
> > > > > > 
> > > > > > Camille or Nasko, is there a potential bug here that needs
> > investigation?
> > > > > 
> > > > > Switching to about:blank should indeed fix the issue.
> > > > > 
> > > > > I'd be interested to know how this test work on Android. AFAIR all
data
> > urls
> > > > in
> > > > > Android are sent to the browser and do not commit synchronously
either.
> > This
> > > > > caused issue with NavigationHandles and interstitials in the past.
This
> > > means
> > > > > that there is no requirement that data urls commit in a synchronous
> > manner.
> > > I
> > > > > imagine that in the Android case, the asynchronous data url navigation
> > gets
> > > > > cancelled by the following location.replace. I imagine this is not the
> > case
> > > on
> > > > > PlzNavigate, possibly because the data url navigation could be marked
as
> > > > having
> > > > > a user gesture while the latter location.replace doesn't, so we would
> > favor
> > > > the
> > > > > first one.
> > > > > 
> > > > > That said if a website is relying on fixed behavior between two
> concurrent
> > > > > non-synchronous navigations there's bound to be issues IMO.
> > > > 
> > > > Looking at the code for Android the data urls appear to be handled here.

> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/content/child/web_url_loader_impl.cc?dr=...
> > > > The request is then sent off to the browser. When the src url is
switched
> to
> > > > something else, the original request would be cancelled in blink which
> would
> > > > explain why it works on Android. With PlzNavigate the top level frame
> > > > navigations are routed to the browser which does not know about
subsequent
> > src
> > > > url changes in blink. Perhaps we need to add a mechanism to cancel
ongoing
> > > > navigations?
> > > 
> > > Camille was describing the mechanism we already have in PlzNavigate, which
> > takes
> > > into account whether there was a user gesture or not.
> > > 
> > > I'd feel better about this change if the existing layout tests were flaky
in
> > > non-PlzNavigate, given a race between the data URL and the subsequent URL.

> It
> > > sounds like that's not the case, likely because the subsequent navigation
> > > cancels the data URL navigation today if they overlap.  I think there's a
> > > fundamental change that PlzNavigate is making here by not having the
second
> > > navigation cancel the first (which would probably be hard to pull off
anyway
> > due
> > > to the race between processes).
> > 
> > I'm also very curious why the data URL navigation isn't cancelled by the
> > subsequent navigation. Aren't both of them lacking user gestures, at which
> point
> > we should be in the case where the last one wins. Or am I missing some
detail?
> > 
> > > Anyway, I won't stand in the way if you want to proceed with this, since I
> > don't
> > > see a way to avoid the race in PlzNavigate.  Still, I think you'll want to
> be
> > > very careful about introducing bugs like https://crbug.com/657896 (Pri-0)
> when
> > > PlzNavigate gets turned on-- any page that starts its iframe at a data URL
> and
> > > then navigates it to a real URL is at risk of breaking.  Maybe it's worth
> > > collecting metrics to see if you need to address it?
> > 
> > I'd like to understand this case a bit better too before calling it "fixed".
> 
> The timing issue introduced by PlzNavigate is as below:
> 1. A navigation is initiated in blink for a frame.
> 2. In RenderFrameImpl::decidePolicyForNavigation we route this to the browser
> via BeginNavigation.
> 3. A script then sets the frame src to another url. This schedules a task.
> NavigationScheduler::scheduleLocationChange().
> 4. Before this task runs, the commit navigation IPC is processed. This
> eventually calls FrameLoader::startLoad() which
>    cancels scheduled navigations.
> 5. This causes scheduled navigations to not take effect.
> 
> Without PlzNavigate, that won't happen because the timing window is much
smaller
> and the navigation scheduler task gets to
> run predictably.
> 
> 4.

I updated the patchset to avoid navigation cancellation for plznavigate with
exceptions being user initiated
navigations. The tests work correctly with this approach as well.

[ananta, 2017-01-19 00:54:42]

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

[commit-bot: I haz the power, 2017-01-19 00:55:20]

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-19 02:47:10]

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

[commit-bot: I haz the power, 2017-01-19 02:47:11]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-01-19 21:02:59]

https://codereview.chromium.org/2630783003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2630783003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1362:
UserGestureIndicator::processingUserGesture()) {
Is this user gesture check necessary to get the tests to pass? It looks
different than what blink does for internal navigations, which seems less than
ideal.

https://codereview.chromium.org/2630783003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1694: bool
navigationHandledByClient = m_isNavigationHandledByClient;
This should be probably have a "was" in the name, and note that
shouldContinueForNavigationPolicy may change its value?

[ananta, 2017-01-19 21:38:08]

https://codereview.chromium.org/2630783003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2630783003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1362:
UserGestureIndicator::processingUserGesture()) {
On 2017/01/19 21:02:59, Nate Chapin wrote:
> Is this user gesture check necessary to get the tests to pass? It looks
> different than what blink does for internal navigations, which seems less than
> ideal.

The user gesture flag is passed via the FrameHostMsg_BeginNavigation IPC to the
browser and is sent back via the FrameMsg_CommitNavigation IPC back to the
renderer. This is then set in the form of a scoped user gesture for the duration
of the NavigateInternal() function.

The UserGestureIndicator::processingUserGesture() checks the same flag. Added a
comment explaining this.

https://codereview.chromium.org/2630783003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1694: bool
navigationHandledByClient = m_isNavigationHandledByClient;
On 2017/01/19 21:02:59, Nate Chapin wrote:
> This should be probably have a "was" in the name, and note that
> shouldContinueForNavigationPolicy may change its value?

Done.

[ananta, 2017-01-19 22:10:03]

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

[commit-bot: I haz the power, 2017-01-19 22:10:34]

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

[Nate Chapin, 2017-01-19 23:33:51]

https://codereview.chromium.org/2630783003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2630783003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1362:
UserGestureIndicator::processingUserGesture()) {
On 2017/01/19 21:38:08, ananta wrote:
> On 2017/01/19 21:02:59, Nate Chapin wrote:
> > Is this user gesture check necessary to get the tests to pass? It looks
> > different than what blink does for internal navigations, which seems less
than
> > ideal.
> 
> The user gesture flag is passed via the FrameHostMsg_BeginNavigation IPC to
the
> browser and is sent back via the FrameMsg_CommitNavigation IPC back to the
> renderer. This is then set in the form of a scoped user gesture for the
duration
> of the NavigateInternal() function.
> 
> The UserGestureIndicator::processingUserGesture() checks the same flag. Added
a
> comment explaining this.
> 

Right, I got that. I just don't immediately see why you need to check
processingUserGesture() here. There's no similar special case for
processingUserGesture() when starting navigations without PlzNaviagate, is
there?

[commit-bot: I haz the power, 2017-01-19 23:54:31]

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

[commit-bot: I haz the power, 2017-01-19 23:54:32]

Dry run: This issue passed the CQ dry run.

[ananta, 2017-01-20 00:00:33]

https://codereview.chromium.org/2630783003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2630783003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1362:
UserGestureIndicator::processingUserGesture()) {
On 2017/01/19 23:33:50, Nate Chapin wrote:
> On 2017/01/19 21:38:08, ananta wrote:
> > On 2017/01/19 21:02:59, Nate Chapin wrote:
> > > Is this user gesture check necessary to get the tests to pass? It looks
> > > different than what blink does for internal navigations, which seems less
> than
> > > ideal.
> > 
> > The user gesture flag is passed via the FrameHostMsg_BeginNavigation IPC to
> the
> > browser and is sent back via the FrameMsg_CommitNavigation IPC back to the
> > renderer. This is then set in the form of a scoped user gesture for the
> duration
> > of the NavigateInternal() function.
> > 
> > The UserGestureIndicator::processingUserGesture() checks the same flag.
Added
> a
> > comment explaining this.
> > 
> 
> Right, I got that. I just don't immediately see why you need to check
> processingUserGesture() here. There's no similar special case for
> processingUserGesture() when starting navigations without PlzNaviagate, is
> there?

Based on our discussion, removing this check

[ananta, 2017-01-20 00:03:08]

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

[commit-bot: I haz the power, 2017-01-20 00:04:34]

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

[Nate Chapin, 2017-01-20 00:08:08]

LGTM. Coming up with something better for m_navigationHandledByClient is on my
list of things to do someday, but maybe it'll get better when all of blink's
provisional logic goes away :)

[commit-bot: I haz the power, 2017-01-20 03:20:56]

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

[commit-bot: I haz the power, 2017-01-20 03:20:57]

Dry run: This issue passed the CQ dry run.

[ananta, 2017-01-20 19:40:47]

The CQ bit was checked by ananta@chromium.org

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

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 19:44:59]

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

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

Failed to apply patch for
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:150
  error:
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:
patch does not apply

Patch:      
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
Index:
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
diff --git
a/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
b/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
index
eaa5322b2333c1a3e1d9fdee09e50615b592a1ef..b8d170fe1dd49e9b5363fd8045be76b710251434
100644
---
a/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
+++
b/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
@@ -150,12 +150,6 @@ Bug(none) http/tests/navigation/response204.html [ Timeout
]
 Bug(none) virtual/stable/http/tests/navigation/response204.html [ Timeout ]
 Bug(none) virtual/mojo-loading/http/tests/navigation/response204.html [ Timeout
]
 
-# https://crbug.com/673748 Cross frame access should work
-crbug.com/673748
http/tests/security/cross-frame-access-parent-isolated-world.html [ Timeout ]
-crbug.com/673748
http/tests/security/cross-frame-access-parent-explicit-domain-isolated-world.html
[ Timeout ]
-crbug.com/673748
virtual/mojo-loading/http/tests/security/cross-frame-access-parent-isolated-world.html
[ Timeout ]
-crbug.com/673748
virtual/mojo-loading/http/tests/security/cross-frame-access-parent-explicit-domain-isolated-world.html
[ Timeout ]
-
 # This test seems to be partially failing without PlzNavigate as well.
 Bug(none)
imported/wpt/html/browsers/origin/cross-origin-objects/cross-origin-objects-exceptions.html
[ Failure ]

[ananta, 2017-01-20 20:10:22]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-01-20 20:10:23]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2630783003/#ps100001
(title: "Rebased to tip")

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

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 21:35:50]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484943022998710,
"parent_rev": "860f9bdcd6642816bd2a4879bc878f8473927d9f", "commit_rev":
"f59dc823911d1e7fca195b638638e8458c631491"}

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

Description was changed from

==========
PlzNavigate: Fix the
http/tests/security/cross-frame-access-parent-isolated-world.html and
http/tests/security/cross-frame-access-parent-explicit-domain-isolated-world.html
layout tests.

These tests instantiate an iframe with the following src "data:text/html," and
then later
on switch the iframe src to a different URL to run the security tests.

With PlzNavigate when the iframe is instantiated, we send the navigation off to
the browser
via RenderFrameImpl::BeginNavigation. This function gets called in the
decidePolicyForNavigation code path. When this
returns blink eventually schedules a task for switching the navigation src url
for the iframe via a call to
the NavigationScheduler::scheduleLocationChange which is called in the
LocalFrame::navigate function. Before this scheduled
task runs, the CommitNavigation IPC comes back from the browser and the original
load completes for the iframe which cancels
pending navigations on the iframe. As a result the task never runs and the test
times out.
Without PlzNavigate this does not happen as the original load on the iframe is
completed by the time the task runs.

It looks like fixing the layout test itself to switch the src on the iframe
after it finishes loading is one approach. Alternatively we could avoid
cancelling pending navigations for plznavigate. 
Exceptions being user initiated navigations.

BUG=673748
==========

to

==========
PlzNavigate: Fix the
http/tests/security/cross-frame-access-parent-isolated-world.html and
http/tests/security/cross-frame-access-parent-explicit-domain-isolated-world.html
layout tests.

These tests instantiate an iframe with the following src "data:text/html," and
then later
on switch the iframe src to a different URL to run the security tests.

With PlzNavigate when the iframe is instantiated, we send the navigation off to
the browser
via RenderFrameImpl::BeginNavigation. This function gets called in the
decidePolicyForNavigation code path. When this
returns blink eventually schedules a task for switching the navigation src url
for the iframe via a call to
the NavigationScheduler::scheduleLocationChange which is called in the
LocalFrame::navigate function. Before this scheduled
task runs, the CommitNavigation IPC comes back from the browser and the original
load completes for the iframe which cancels
pending navigations on the iframe. As a result the task never runs and the test
times out.
Without PlzNavigate this does not happen as the original load on the iframe is
completed by the time the task runs.

It looks like fixing the layout test itself to switch the src on the iframe
after it finishes loading is one approach. Alternatively we could avoid
cancelling pending navigations for plznavigate. 
Exceptions being user initiated navigations.

BUG=673748

Review-Url: https://codereview.chromium.org/2630783003
Cr-Commit-Position: refs/heads/master@{#445174}
Committed:
https://chromium.googlesource.com/chromium/src/+/f59dc823911d1e7fca195b638638...
==========

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

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f59dc823911d1e7fca195b638638...