Allow navigations to frames that aren't being unloaded in the unload handler.

Allow navigations to frames that aren't being unloaded in the unload handler.

This change lets the page navigate frames that aren't being unloaded in
the unload handler. For example, a popup may want to navigate its
opener while it's being closed.

FrameLoader now also correctly checks if the |targetFrame| can be
navigated, instead of checking if |m_frame| can be navigated.

BUG=660496
Review-Url: https://codereview.chromium.org/2487403002
Cr-Commit-Position: refs/heads/master@{#442353}
Committed: https://chromium.googlesource.com/chromium/src/+/2267cfcce06c92602376754343e5f45effeee1f1

[lfg, 2016-11-10 00:05:02]

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

[commit-bot: I haz the power, 2016-11-10 00:05:33]

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

[lfg, 2016-11-10 00:15:03]

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

[lfg, 2016-11-10 00:15:04]

Daniel, can you take a look?

This change shouldn't break any existing use cases, only allow extra ones.

I also looked at the spec:
https://html.spec.whatwg.org/multipage/browsers.html#navigating-across-documents

Looking at the spec, it only specifies that the navigation should be aborted
when the if the existing browsingContext is being navigated, so I believe this
is more correct according to the spec (as opposed to using a global disabler).

https://codereview.chromium.org/2487403002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/events/before-unload-forbidden-navigation.html
(left):

https://codereview.chromium.org/2487403002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/events/before-unload-forbidden-navigation.html:29:
location.href = 'resources/before-unload-in-subframe-fail.html';
This test was navigating the top-level frame in the unload handler of a
subframe. This is allowed after my change.

The test is still relevant though, since it tests that the subframe won't
navigate on its own unload handler.

https://codereview.chromium.org/2487403002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/History.cpp (right):

https://codereview.chromium.org/2487403002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/History.cpp:143:
!activeDocument->frame()->isNavigationAllowed())
We should probably merge these two functions, but let's do it in a separate
change.

https://codereview.chromium.org/2487403002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2487403002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1088: if
(!m_frame->isNavigationAllowed())
This change has a side effect that javascript: navigations will be allowed if
the frame disallows navigations. I think this should be ok, since the frame is
already executing javascript anyway.

[lfg, 2016-11-29 17:28:14]

Ping!

[dcheng, 2016-11-30 06:15:31]

1) I'd rather not roll back the restrictions on beforeunload at all.

2) Is it possible for an unloading page to navigate other pages in a way that
will cause us to try to display multiple modal dialogs? What happens in that
case? For example, can this happen if a page schedules navigations in the unload
handler, and then spins the message loop with a sync XHR?

[lfg, 2016-11-30 18:30:39]

On 2016/11/30 06:15:31, dcheng wrote:
> 1) I'd rather not roll back the restrictions on beforeunload at all.

Why not? I think unifying the code paths for the navigation disabler is a
significant improvement to the NavigationScheduler and I would prefer fixing
any issues that could come out of there than keep maintaining the old disabler
with a global variable.

I also think it follows the spec better. Edge/IE also allows navigations in
the beforeunload handler.

> 2) Is it possible for an unloading page to navigate other pages in a way that
> will cause us to try to display multiple modal dialogs? What happens in that
> case? For example, can this happen if a page schedules navigations in the
unload
> handler, and then spins the message loop with a sync XHR?

Yes, it's possible. What kind of modal dialog are you thinking? For beforeunload
dialogs, multiple dialogs are supressed here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...

Note that this is no different than it was before my change, and other browser
behave similarly.

[dcheng, 2016-11-30 19:36:20]

On 2016/11/30 18:30:39, lfg wrote:
> On 2016/11/30 06:15:31, dcheng wrote:
> > 1) I'd rather not roll back the restrictions on beforeunload at all.
> 
> Why not? I think unifying the code paths for the navigation disabler is a
> significant improvement to the NavigationScheduler and I would prefer fixing
> any issues that could come out of there than keep maintaining the old disabler
> with a global variable.
> 
> I also think it follows the spec better. Edge/IE also allows navigations in
> the beforeunload handler.

Because Blink hasn't allowed them for a very long time, and that hasn't been
problematic. I don't see a need to roll this behavior back when we should be
applying more limitations on beforeunload, rather than less.

> 
> > 2) Is it possible for an unloading page to navigate other pages in a way
that
> > will cause us to try to display multiple modal dialogs? What happens in that
> > case? For example, can this happen if a page schedules navigations in the
> unload
> > handler, and then spins the message loop with a sync XHR?
> 
> Yes, it's possible. What kind of modal dialog are you thinking? For
beforeunload
> dialogs, multiple dialogs are supressed here:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...
> 
> Note that this is no different than it was before my change, and other browser
> behave similarly.

It's not clear to me how that actually blocks multiple dialogs, since the
boolean it checks is scoped to one instantation of shouldClose().

[lfg, 2016-12-01 00:34:26]

On 2016/11/30 19:36:20, dcheng wrote:
> On 2016/11/30 18:30:39, lfg wrote:
> > On 2016/11/30 06:15:31, dcheng wrote:
> > > 1) I'd rather not roll back the restrictions on beforeunload at all.
> > 
> > Why not? I think unifying the code paths for the navigation disabler is a
> > significant improvement to the NavigationScheduler and I would prefer fixing
> > any issues that could come out of there than keep maintaining the old
disabler
> > with a global variable.
> > 
> > I also think it follows the spec better. Edge/IE also allows navigations in
> > the beforeunload handler.
> 
> Because Blink hasn't allowed them for a very long time, and that hasn't been
> problematic. I don't see a need to roll this behavior back when we should be
> applying more limitations on beforeunload, rather than less.

Sorry, but I'll have to say that I don't think that the fact that this was done
for a long time is a good argument for maintaining a broken behavior. Both
Firefox and Edge allows navigations in beforeunload, I don't see why we
shouldn't
do it as well.

> > > 2) Is it possible for an unloading page to navigate other pages in a way
> that
> > > will cause us to try to display multiple modal dialogs? What happens in
that
> > > case? For example, can this happen if a page schedules navigations in the
> > unload
> > > handler, and then spins the message loop with a sync XHR?
> > 
> > Yes, it's possible. What kind of modal dialog are you thinking? For
> beforeunload
> > dialogs, multiple dialogs are supressed here:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...
> > 
> > Note that this is no different than it was before my change, and other
browser
> > behave similarly.
> 
> It's not clear to me how that actually blocks multiple dialogs, since the
> boolean it checks is scoped to one instantation of shouldClose().

It blocks all dialogs from the same page. However, if navigating another tab,
then one
dialog is shown after the first one is answered.

[dcheng, 2016-12-01 00:42:45]

On 2016/12/01 00:34:26, lfg wrote:
> On 2016/11/30 19:36:20, dcheng wrote:
> > On 2016/11/30 18:30:39, lfg wrote:
> > > On 2016/11/30 06:15:31, dcheng wrote:
> > > > 1) I'd rather not roll back the restrictions on beforeunload at all.
> > > 
> > > Why not? I think unifying the code paths for the navigation disabler is a
> > > significant improvement to the NavigationScheduler and I would prefer
fixing
> > > any issues that could come out of there than keep maintaining the old
> disabler
> > > with a global variable.
> > > 
> > > I also think it follows the spec better. Edge/IE also allows navigations
in
> > > the beforeunload handler.
> > 
> > Because Blink hasn't allowed them for a very long time, and that hasn't been
> > problematic. I don't see a need to roll this behavior back when we should be
> > applying more limitations on beforeunload, rather than less.
> 
> Sorry, but I'll have to say that I don't think that the fact that this was
done
> for a long time is a good argument for maintaining a broken behavior. Both
> Firefox and Edge allows navigations in beforeunload, I don't see why we
> shouldn't
> do it as well.

Addressing this is out of scope of the actual bug we're trying to address here.

In general, there's strong consensus in Blink that beforeunload should do less,
not more. While it's unfortunate that Edge and FF don't agree with Blink/Safari
on this behavior, it doesn't actually seem to be causing trouble in practice. I
don't see any reason we should relax those restrictions, since it just makes it
that much harder to try to improve this in the future.

> 
> > > > 2) Is it possible for an unloading page to navigate other pages in a way
> > that
> > > > will cause us to try to display multiple modal dialogs? What happens in
> that
> > > > case? For example, can this happen if a page schedules navigations in
the
> > > unload
> > > > handler, and then spins the message loop with a sync XHR?
> > > 
> > > Yes, it's possible. What kind of modal dialog are you thinking? For
> > beforeunload
> > > dialogs, multiple dialogs are supressed here:
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...
> > > 
> > > Note that this is no different than it was before my change, and other
> browser
> > > behave similarly.
> > 
> > It's not clear to me how that actually blocks multiple dialogs, since the
> > boolean it checks is scoped to one instantation of shouldClose().
> 
> It blocks all dialogs from the same page. However, if navigating another tab,
> then one
> dialog is shown after the first one is answered.

[lfg, 2016-12-01 21:15:11]

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

[commit-bot: I haz the power, 2016-12-01 21:15:45]

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

[lfg, 2016-12-01 22:06:32]

Description was changed from

==========
Allow navigations to frames that aren't being unloaded in the unload and
beforeunload handlers.

This change lets the page navigate frames that aren't being unloaded in
the unload handlers. For example, a popup may want to navigate its
opener while it's being closed.

BUG=660496
==========

to

==========
Allow navigations to frames that aren't being unloaded in the unload handler.

This change lets the page navigate frames that aren't being unloaded in
the unload handler. For example, a popup may want to navigate its
opener while it's being closed.

BUG=660496
==========

[commit-bot: I haz the power, 2016-12-01 22:55:52]

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

[commit-bot: I haz the power, 2016-12-01 22:55:53]

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_...)

[lfg, 2016-12-01 23:26:55]

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

[commit-bot: I haz the power, 2016-12-01 23:27: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, 2016-12-02 01:00:06]

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

[commit-bot: I haz the power, 2016-12-02 01:00:07]

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_...)

[lfg, 2016-12-02 16:46:21]

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

[commit-bot: I haz the power, 2016-12-02 16:46:38]

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, 2016-12-02 18:19:23]

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

[commit-bot: I haz the power, 2016-12-02 18:19:24]

Dry run: This issue passed the CQ dry run.

[lfg, 2016-12-02 18:28:20]

I've added back the global disabler for beforeunload. Please take another look.

[dcheng, 2016-12-08 23:16:53]

Overall, seems reasonable to me. Please do expand the CL description to cover
incidental fixes as well though (FrameLoader now correctly checks that
|targetFrame| rather than always using |m_frame|).

As an aside, we probably want to rename isNavigationAllowed() in a followup...
it's a bit confusing, since we also have canNavigate() on Frame.

https://codereview.chromium.org/2487403002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2487403002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:926: for (const Frame* cur =
this; cur; cur = cur->tree().parent()) {
Nit: no abbreviations in naming

[dcheng, 2016-12-09 04:35:53]

On 2016/12/08 23:16:53, dcheng wrote:
> Overall, seems reasonable to me. Please do expand the CL description to cover
> incidental fixes as well though (FrameLoader now correctly checks that
> |targetFrame| rather than always using |m_frame|).
> 
> As an aside, we probably want to rename isNavigationAllowed() in a followup...
> it's a bit confusing, since we also have canNavigate() on Frame.
> 
>
https://codereview.chromium.org/2487403002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):
> 
>
https://codereview.chromium.org/2487403002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/LocalFrame.cpp:926: for (const Frame* cur
=
> this; cur; cur = cur->tree().parent()) {
> Nit: no abbreviations in naming

Also, would it be possible to have a test case for the bug fix in
FrameLoader::load?

One where we navigate a target frame which is going to be unloaded, but the
source frame itself is not yet being unloaded.
And another of the reverse. And also, confirm that they fail before your CL.

[lfg, 2016-12-12 23:09:03]

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

[commit-bot: I haz the power, 2016-12-12 23:11:10]

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

[lfg, 2016-12-12 23:11:10]

On 2016/12/09 04:35:53, dcheng wrote:
> On 2016/12/08 23:16:53, dcheng wrote:
> > Overall, seems reasonable to me. Please do expand the CL description to
cover
> > incidental fixes as well though (FrameLoader now correctly checks that
> > |targetFrame| rather than always using |m_frame|).
> > 
> > As an aside, we probably want to rename isNavigationAllowed() in a
followup...
> > it's a bit confusing, since we also have canNavigate() on Frame.
> > 
> >
>
https://codereview.chromium.org/2487403002/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2487403002/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/frame/LocalFrame.cpp:926: for (const Frame*
cur
> =
> > this; cur; cur = cur->tree().parent()) {
> > Nit: no abbreviations in naming
> 
> Also, would it be possible to have a test case for the bug fix in
> FrameLoader::load?
> 
> One where we navigate a target frame which is going to be unloaded, but the
> source frame itself is not yet being unloaded.

This isn't possible. If a frame that's being unloaded is navigated, even if
we schedule the navigation, when the original navigation commits all the
scheduled navigations get removed.

> And another of the reverse. And also, confirm that they fail before your CL.

This is already exercised by the test that I added (and it's the reason I had
to fix this). It navigates a sibling frame while unloading.

[lfg, 2016-12-12 23:12:05]

Description was changed from

==========
Allow navigations to frames that aren't being unloaded in the unload handler.

This change lets the page navigate frames that aren't being unloaded in
the unload handler. For example, a popup may want to navigate its
opener while it's being closed.

BUG=660496
==========

to

==========
Allow navigations to frames that aren't being unloaded in the unload handler.

This change lets the page navigate frames that aren't being unloaded in
the unload handler. For example, a popup may want to navigate its
opener while it's being closed.

FrameLoader now also correctly checks if the |targetFrame| can be navigated,
instead of checking if |m_frame| can be navigated.

BUG=660496
==========

[lfg, 2016-12-12 23:12:27]

Description was changed from

==========
Allow navigations to frames that aren't being unloaded in the unload handler.

This change lets the page navigate frames that aren't being unloaded in
the unload handler. For example, a popup may want to navigate its
opener while it's being closed.

FrameLoader now also correctly checks if the |targetFrame| can be navigated,
instead of checking if |m_frame| can be navigated.

BUG=660496
==========

to

==========
Allow navigations to frames that aren't being unloaded in the unload handler.

This change lets the page navigate frames that aren't being unloaded in
the unload handler. For example, a popup may want to navigate its
opener while it's being closed.

FrameLoader now also correctly checks if the |targetFrame| can be
navigated, instead of checking if |m_frame| can be navigated.

BUG=660496
==========

[lfg, 2016-12-12 23:12:40]

Please take another look.

https://codereview.chromium.org/2487403002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2487403002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:926: for (const Frame* cur =
this; cur; cur = cur->tree().parent()) {
On 2016/12/08 23:16:53, dcheng wrote:
> Nit: no abbreviations in naming

Done.

[commit-bot: I haz the power, 2016-12-12 23:15:55]

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

[commit-bot: I haz the power, 2016-12-12 23:15:56]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[lfg, 2016-12-12 23:22:26]

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

[commit-bot: I haz the power, 2016-12-12 23:24:10]

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

[dcheng, 2016-12-13 01:04:53]

On 2016/12/12 23:11:10, lfg wrote:
> On 2016/12/09 04:35:53, dcheng wrote:
> > On 2016/12/08 23:16:53, dcheng wrote:
> > > Overall, seems reasonable to me. Please do expand the CL description to
> cover
> > > incidental fixes as well though (FrameLoader now correctly checks that
> > > |targetFrame| rather than always using |m_frame|).
> > > 
> > > As an aside, we probably want to rename isNavigationAllowed() in a
> followup...
> > > it's a bit confusing, since we also have canNavigate() on Frame.
> > > 
> > >
> >
>
https://codereview.chromium.org/2487403002/diff/100001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2487403002/diff/100001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/frame/LocalFrame.cpp:926: for (const Frame*
> cur
> > =
> > > this; cur; cur = cur->tree().parent()) {
> > > Nit: no abbreviations in naming
> > 
> > Also, would it be possible to have a test case for the bug fix in
> > FrameLoader::load?
> > 
> > One where we navigate a target frame which is going to be unloaded, but the
> > source frame itself is not yet being unloaded.
> 
> This isn't possible. If a frame that's being unloaded is navigated, even if
> we schedule the navigation, when the original navigation commits all the
> scheduled navigations get removed.
> 

What if it's an in-page navigation?

> > And another of the reverse. And also, confirm that they fail before your CL.
> 
> This is already exercised by the test that I added (and it's the reason I had
> to fix this). It navigates a sibling frame while unloading.

I'm specifically referring to the case where we look up a targetFrame in
FrameLoader::load(). I'm trying to understand why the code changes there are
necessary: why isn't falling through to Frame::navigate and letting the target
frame check its own navigation status sufficient?

[commit-bot: I haz the power, 2016-12-13 01:31:23]

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

[commit-bot: I haz the power, 2016-12-13 01:31:24]

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

[lfg, 2016-12-14 20:57:52]

On 2016/12/13 01:04:53, dcheng wrote:
> 
> I'm specifically referring to the case where we look up a targetFrame in
> FrameLoader::load(). I'm trying to understand why the code changes there are
> necessary: why isn't falling through to Frame::navigate and letting the target
> frame check its own navigation status sufficient?

Ah, sorry, it's been a few weeks, I didn't remember correctly. I debugged this
again today, and there's actually already a test that does this:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/load...

The reason the test passes today, is because there was no
FrameNavigationDisabler before dispatching the unload handler, but my
patch restricts that by adding this to the unload handler.

This is only needed because that test uses a synchronous navigation in the
unload handler, causing FrameLoader::load() to be called directly instead
of using the NavigationScheduler.

Do you still think we need another test? What kind of code path are you looking
to test with the test?

[dcheng, 2016-12-16 09:36:03]

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

[dcheng, 2016-12-16 09:36:05]

On 2016/12/14 20:57:52, lfg wrote:
> On 2016/12/13 01:04:53, dcheng wrote:
> > 
> > I'm specifically referring to the case where we look up a targetFrame in
> > FrameLoader::load(). I'm trying to understand why the code changes there are
> > necessary: why isn't falling through to Frame::navigate and letting the
target
> > frame check its own navigation status sufficient?
> 
> Ah, sorry, it's been a few weeks, I didn't remember correctly. I debugged this
> again today, and there's actually already a test that does this:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/load...
> 
> The reason the test passes today, is because there was no
> FrameNavigationDisabler before dispatching the unload handler, but my
> patch restricts that by adding this to the unload handler.
> 
> This is only needed because that test uses a synchronous navigation in the
> unload handler, causing FrameLoader::load() to be called directly instead
> of using the NavigationScheduler.

OK, so the problem is that when we call FrameLoader::load() directly, the
current frame might have navigations disabled, and we'll never get to the part
of the code that forwards it to the targeted frame.

I'm a bit nervous that a future CL can introduce an entry point to
FrameLoader::load and forget to check that navigations allowed. Since this logic
only checks it for the target frame now, we'll bypass the check. I prefer the
check against the FrameLoader's associated frame, because that enforces that
we'll always make the right check. Not having this check led to UXSS in the
past.

Ideally, we could just do the check after we early return after forwarding it to
a |targetFrame|. But I see we do some work before that...

+japhet, is it possible that a back/forward load will ever have a targetFrame?
Or are they mutually exclusive? It seems like they ought to be; otherwise, it'd
affect a bunch of random FrameLoader state before it forwards to the targeted
frame, which seems bad.

> 
> Do you still think we need another test? What kind of code path are you
looking
> to test with the test?

[lfg, 2016-12-19 22:43:35]

Friendly ping!

[lfg, 2016-12-21 23:08:40]

Ping! This is an M56 release blocker, let's try to get this reviewed in time so
that we can fix this regression before the M56 release.

[dcheng, 2016-12-21 23:23:34]

On 2016/12/21 23:08:40, lfg wrote:
> Ping! This is an M56 release blocker, let's try to get this reviewed in time
so
> that we can fix this regression before the M56 release.

Can we just try making the changes I suggested locally and seeing if any tests
break?

[Nate Chapin, 2016-12-27 23:40:42]

On 2016/12/16 09:36:05, dcheng wrote:
> On 2016/12/14 20:57:52, lfg wrote:
> > On 2016/12/13 01:04:53, dcheng wrote:
> > > 
> > > I'm specifically referring to the case where we look up a targetFrame in
> > > FrameLoader::load(). I'm trying to understand why the code changes there
are
> > > necessary: why isn't falling through to Frame::navigate and letting the
> target
> > > frame check its own navigation status sufficient?
> > 
> > Ah, sorry, it's been a few weeks, I didn't remember correctly. I debugged
this
> > again today, and there's actually already a test that does this:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/load...
> > 
> > The reason the test passes today, is because there was no
> > FrameNavigationDisabler before dispatching the unload handler, but my
> > patch restricts that by adding this to the unload handler.
> > 
> > This is only needed because that test uses a synchronous navigation in the
> > unload handler, causing FrameLoader::load() to be called directly instead
> > of using the NavigationScheduler.
> 
> OK, so the problem is that when we call FrameLoader::load() directly, the
> current frame might have navigations disabled, and we'll never get to the part
> of the code that forwards it to the targeted frame.
> 
> I'm a bit nervous that a future CL can introduce an entry point to
> FrameLoader::load and forget to check that navigations allowed. Since this
logic
> only checks it for the target frame now, we'll bypass the check. I prefer the
> check against the FrameLoader's associated frame, because that enforces that
> we'll always make the right check. Not having this check led to UXSS in the
> past.
> 
> Ideally, we could just do the check after we early return after forwarding it
to
> a |targetFrame|. But I see we do some work before that...
> 
> +japhet, is it possible that a back/forward load will ever have a targetFrame?
> Or are they mutually exclusive? It seems like they ought to be; otherwise,
it'd
> affect a bunch of random FrameLoader state before it forwards to the targeted
> frame, which seems bad.

back/forward loads determine their frame upfront outside of blink, so they
shouldn't ever give a targetFrame to blink.

> 
> > 
> > Do you still think we need another test? What kind of code path are you
> looking
> > to test with the test?

[lfg, 2017-01-03 21:09:39]

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

[commit-bot: I haz the power, 2017-01-03 21:10:07]

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-03 23:02:52]

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

[commit-bot: I haz the power, 2017-01-03 23:02:53]

Dry run: This issue passed the CQ dry run.

[lfg, 2017-01-04 00:03:27]

On 2016/12/21 23:23:34, dcheng wrote:
> On 2016/12/21 23:08:40, lfg wrote:
> > Ping! This is an M56 release blocker, let's try to get this reviewed in time
> so
> > that we can fix this regression before the M56 release.
> 
> Can we just try making the changes I suggested locally and seeing if any tests
> break?

I've added a CHECK() in FrameLoader that asserts that m_frame has navigations
allowed. Is that what you meant?

All tests pass, so there are no tests that exercise this behavior. I can't
think of any way of forcing a call on FrameLoader::load() trying to load a
frame that doesn't have navigations allowed, so I'm thinking it should be
either a CHECK or DCHECK, but not an early return.

[dcheng, 2017-01-04 00:52:38]

On 2017/01/04 00:03:27, lfg wrote:
> On 2016/12/21 23:23:34, dcheng wrote:
> > On 2016/12/21 23:08:40, lfg wrote:
> > > Ping! This is an M56 release blocker, let's try to get this reviewed in
time
> > so
> > > that we can fix this regression before the M56 release.
> > 
> > Can we just try making the changes I suggested locally and seeing if any
tests
> > break?
> 
> I've added a CHECK() in FrameLoader that asserts that m_frame has navigations
> allowed. Is that what you meant?
> 
> All tests pass, so there are no tests that exercise this behavior. I can't
> think of any way of forcing a call on FrameLoader::load() trying to load a
> frame that doesn't have navigations allowed, so I'm thinking it should be
> either a CHECK or DCHECK, but not an early return.

It's hard for a reader to determine that all possible code paths are covered:
the CHECK documents this now, but it's also partially redundant with the
targetFrame->isNavigationAllowed() check.

My earlier suggestion was to just restructure this to make the invariants more
obvious:
- One block at the top, to handle navigations that target another FrameLoader.
- Then do the isNavigationAllowed() + early return

Given what Nate said, it seems like this should be possible.

[lfg, 2017-01-04 18:46:22]

On 2017/01/04 00:52:38, dcheng wrote:
> 
> It's hard for a reader to determine that all possible code paths are covered:
> the CHECK documents this now, but it's also partially redundant with the
> targetFrame->isNavigationAllowed() check.
> 
> My earlier suggestion was to just restructure this to make the invariants more
> obvious:
> - One block at the top, to handle navigations that target another FrameLoader.
> - Then do the isNavigationAllowed() + early return
> 
> Given what Nate said, it seems like this should be possible.

What do you think about doing this in a follow-up CL? The code around is very
complicated with lots of early returns, and I'm a bit worried about
unintentionally breaking something while restructuring it.

I think the way it is now, the CHECK is a good guarantee that this isn't called
in the wrong place, and even though it will be sometimes redundant, it shouldn't
be wrong.

[dcheng, 2017-01-04 19:12:43]

On 2017/01/04 18:46:22, lfg wrote:
> On 2017/01/04 00:52:38, dcheng wrote:
> > 
> > It's hard for a reader to determine that all possible code paths are
covered:
> > the CHECK documents this now, but it's also partially redundant with the
> > targetFrame->isNavigationAllowed() check.
> > 
> > My earlier suggestion was to just restructure this to make the invariants
more
> > obvious:
> > - One block at the top, to handle navigations that target another
FrameLoader.
> > - Then do the isNavigationAllowed() + early return
> > 
> > Given what Nate said, it seems like this should be possible.
> 
> What do you think about doing this in a follow-up CL? The code around is very
> complicated with lots of early returns, and I'm a bit worried about
> unintentionally breaking something while restructuring it.
> 
> I think the way it is now, the CHECK is a good guarantee that this isn't
called
> in the wrong place, and even though it will be sometimes redundant, it
shouldn't
> be wrong.

The CHECK() doesn't cover the navigation branches taken before targetFrame
resolution though. The one thing that happens before this is the deferred
history load - does that not need a comparable check?

[lfg, 2017-01-04 19:20:58]

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

[commit-bot: I haz the power, 2017-01-04 19:21:23]

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-04 20:58:28]

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

[commit-bot: I haz the power, 2017-01-04 20:58:29]

Dry run: This issue passed the CQ dry run.

[lfg, 2017-01-04 21:01:38]

On 2017/01/04 19:12:43, dcheng wrote:
> On 2017/01/04 18:46:22, lfg wrote:
> > On 2017/01/04 00:52:38, dcheng wrote:
> > > 
> > > It's hard for a reader to determine that all possible code paths are
> covered:
> > > the CHECK documents this now, but it's also partially redundant with the
> > > targetFrame->isNavigationAllowed() check.
> > > 
> > > My earlier suggestion was to just restructure this to make the invariants
> more
> > > obvious:
> > > - One block at the top, to handle navigations that target another
> FrameLoader.
> > > - Then do the isNavigationAllowed() + early return
> > > 
> > > Given what Nate said, it seems like this should be possible.
> > 
> > What do you think about doing this in a follow-up CL? The code around is
very
> > complicated with lots of early returns, and I'm a bit worried about
> > unintentionally breaking something while restructuring it.
> > 
> > I think the way it is now, the CHECK is a good guarantee that this isn't
> called
> > in the wrong place, and even though it will be sometimes redundant, it
> shouldn't
> > be wrong.
> 
> The CHECK() doesn't cover the navigation branches taken before targetFrame
> resolution though. The one thing that happens before this is the deferred
> history load - does that not need a comparable check?

Yes, I think its' needed. I've re-added it, we should also look at adding
some tests to cover this case. I'll also look at that in the follow-up.

[dcheng, 2017-01-04 23:12:03]

I think I'm largely OK with the current structure as-is, but this code has grown
rather organically. I'm hopeful that we can restructure this to make things a
bit simpler (for example, is it possible to just check this bit in
FrameLoader::load? Or is it possible to get rid of this altogether and depend on
Frame::isDetaching()?)

A few comments -- I know this will loosen the navigation restrictions slightly,
but what we implement seems to be far stricter than what the spec requires
anyway.

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:929: if
(currentFrame->isLocalFrame() &&
I'd rather keep this non-recursive: I don't like having to downcast to local
frames, and it causes behavior differences between local and remote frames.

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1102: if
(m_frame->page()->suspended() && isBackForwardLoadType(frameLoadType)) {
If we move isNavigationAllowed() to early return inside this conditional block,
we can avoid writing isBackForwardLoadType() twice.

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1129:
!toLocalFrame(targetFrame)->isNavigationAllowed()) {
We use targetFrame->navigate(), which (for local frames) delegates to
NavigationScheduler. NavigationScheduler::shouldScheduleNavigation() consults
isNavigationAllowed(), so how come we need to check this here as well?

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1159:
CHECK(m_frame->isNavigationAllowed());
I would be more comfortable with this being an early-return, since this is going
to be merged.

[lfg, 2017-01-09 18:43:48]

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

[commit-bot: I haz the power, 2017-01-09 18:44:24]

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

[lfg, 2017-01-09 19:04:12]

It's not possible to just check this bit on FrameLoader::load(), as the check in
NavigationScheduler is necessary to handle to block synchronous navigations like
javascript and hash navigations.

We could probably get rid of the check in History.cpp, but I don't think that's
a problem, and in the future, I think we should merge canNavigate() with
isNavigationAllowed(), so having it documented there is a good thing for now.

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:929: if
(currentFrame->isLocalFrame() &&
On 2017/01/04 23:12:02, dcheng wrote:
> I'd rather keep this non-recursive: I don't like having to downcast to local
> frames, and it causes behavior differences between local and remote frames.

Done.

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1102: if
(m_frame->page()->suspended() && isBackForwardLoadType(frameLoadType)) {
On 2017/01/04 23:12:02, dcheng wrote:
> If we move isNavigationAllowed() to early return inside this conditional
block,
> we can avoid writing isBackForwardLoadType() twice.

I'm not sure about that. There's also the check for page()->suspended(), and I
think that having the separate early returns is easier to follow than to have
one complicated early return with multiple if statements.

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1129:
!toLocalFrame(targetFrame)->isNavigationAllowed()) {
On 2017/01/04 23:12:02, dcheng wrote:
> We use targetFrame->navigate(), which (for local frames) delegates to
> NavigationScheduler. NavigationScheduler::shouldScheduleNavigation() consults
> isNavigationAllowed(), so how come we need to check this here as well?

targetFrame->navigate() just calls FrameLoader::load() when the argument is a
FrameLoadRequest. There are multiple definitions of LocalFrame::navigate().

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1159:
CHECK(m_frame->isNavigationAllowed());
On 2017/01/04 23:12:02, dcheng wrote:
> I would be more comfortable with this being an early-return, since this is
going
> to be merged.

I've agree with that, but I still think that going forward we should document
this well and be more strict about wrong calls.

[dcheng, 2017-01-09 19:17:30]

LGTM with comments addressed.

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1102: if
(m_frame->page()->suspended() && isBackForwardLoadType(frameLoadType)) {
On 2017/01/09 19:04:12, lfg wrote:
> On 2017/01/04 23:12:02, dcheng wrote:
> > If we move isNavigationAllowed() to early return inside this conditional
> block,
> > we can avoid writing isBackForwardLoadType() twice.
> 
> I'm not sure about that. There's also the check for page()->suspended(), and I
> think that having the separate early returns is easier to follow than to have
> one complicated early return with multiple if statements.

I'm OK with this for now, but let's try to clean up how the history logic is
encoded here in a followup. After this patch, there's three disjoint chunks and
it's a bit hard to understand how they all interact together.

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1129:
!toLocalFrame(targetFrame)->isNavigationAllowed()) {
On 2017/01/09 19:04:12, lfg wrote:
> On 2017/01/04 23:12:02, dcheng wrote:
> > We use targetFrame->navigate(), which (for local frames) delegates to
> > NavigationScheduler. NavigationScheduler::shouldScheduleNavigation()
consults
> > isNavigationAllowed(), so how come we need to check this here as well?
> 
> targetFrame->navigate() just calls FrameLoader::load() when the argument is a
> FrameLoadRequest. There are multiple definitions of LocalFrame::navigate().

OK, this check should be in the block at 1133 - 1144 though, I think: we won't
target targetFrame at all otherwise.

https://codereview.chromium.org/2487403002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2487403002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:928: }
Nit: revert this to the original (inlined in header)

[lfg, 2017-01-09 19:30:37]

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

[commit-bot: I haz the power, 2017-01-09 19:32:29]

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-09 21:13:39]

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

[commit-bot: I haz the power, 2017-01-09 21:13:40]

Dry run: This issue passed the CQ dry run.

[lfg, 2017-01-09 21:18:23]

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1129:
!toLocalFrame(targetFrame)->isNavigationAllowed()) {
On 2017/01/09 19:17:30, dcheng wrote:
> On 2017/01/09 19:04:12, lfg wrote:
> > On 2017/01/04 23:12:02, dcheng wrote:
> > > We use targetFrame->navigate(), which (for local frames) delegates to
> > > NavigationScheduler. NavigationScheduler::shouldScheduleNavigation()
> consults
> > > isNavigationAllowed(), so how come we need to check this here as well?
> > 
> > targetFrame->navigate() just calls FrameLoader::load() when the argument is
a
> > FrameLoadRequest. There are multiple definitions of LocalFrame::navigate().
> 
> OK, this check should be in the block at 1133 - 1144 though, I think: we won't
> target targetFrame at all otherwise.

Yes, that should be correct. Done.

https://codereview.chromium.org/2487403002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2487403002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:928: }
On 2017/01/09 19:17:30, dcheng wrote:
> Nit: revert this to the original (inlined in header)

Done.

[lfg, 2017-01-09 21:19:38]

The CQ bit was checked by lfg@chromium.org

[lfg, 2017-01-09 21:19:38]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2487403002/#ps260001
(title: "addressing comments")

[commit-bot: I haz the power, 2017-01-09 21:20:06]

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-09 21:27:53]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1483996778133330,
"parent_rev": "575e97b26f860d773a649eb6c258f23501c4f896", "commit_rev":
"2267cfcce06c92602376754343e5f45effeee1f1"}

[commit-bot: I haz the power, 2017-01-09 21:28:22]

Description was changed from

==========
Allow navigations to frames that aren't being unloaded in the unload handler.

This change lets the page navigate frames that aren't being unloaded in
the unload handler. For example, a popup may want to navigate its
opener while it's being closed.

FrameLoader now also correctly checks if the |targetFrame| can be
navigated, instead of checking if |m_frame| can be navigated.

BUG=660496
==========

to

==========
Allow navigations to frames that aren't being unloaded in the unload handler.

This change lets the page navigate frames that aren't being unloaded in
the unload handler. For example, a popup may want to navigate its
opener while it's being closed.

FrameLoader now also correctly checks if the |targetFrame| can be
navigated, instead of checking if |m_frame| can be navigated.

BUG=660496

Review-Url: https://codereview.chromium.org/2487403002
Cr-Commit-Position: refs/heads/master@{#442353}
Committed:
https://chromium.googlesource.com/chromium/src/+/2267cfcce06c92602376754343e5...
==========

[commit-bot: I haz the power, 2017-01-09 21:28:26]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/2267cfcce06c92602376754343e5...