Re: Allow navigations to frames that aren't being unloaded in the unload and (issue 2487403002 by lfg@chromium.org)

[l...@chromium.org]

Ping!

https://codereview.chromium.org/2487403002/

[dch...@chromium.org]

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?

https://codereview.chromium.org/2487403002/

[l...@chromium.org]

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/Document.cpp?l=2965

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


https://codereview.chromium.org/2487403002/

[dch...@chromium.org]

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/Document.cpp?l=2965
>
> 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().

https://codereview.chromium.org/2487403002/

[l...@chromium.org]

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/Document.cpp?l=2965
> >
> > 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.


https://codereview.chromium.org/2487403002/

[dch...@chromium.org]

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.

[l...@chromium.org]

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

https://codereview.chromium.org/2487403002/

[dch...@chromium.org]

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/Source/core/frame/LocalFrame.cpp
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

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

https://codereview.chromium.org/2487403002/

[dch...@chromium.org]

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.

https://codereview.chromium.org/2487403002/

[l...@chromium.org]

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/Source/core/frame/LocalFrame.cpp
> > File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):
> >
> >
>
https://codereview.chromium.org/2487403002/diff/100001/third_party/WebKit/Source/core/frame/LocalFrame.cpp#newcode926
> > 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.


https://codereview.chromium.org/2487403002/

[l...@chromium.org]

Please take another look.

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

https://codereview.chromium.org/2487403002/diff/100001/third_party/WebKit/Source/core/frame/LocalFrame.cpp#newcode926
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.

https://codereview.chromium.org/2487403002/

[dch...@chromium.org]

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/Source/core/frame/LocalFrame.cpp
> > > File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):
> > >
> > >
> >
>
https://codereview.chromium.org/2487403002/diff/100001/third_party/WebKit/Source/core/frame/LocalFrame.cpp#newcode926
> > > 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?

https://codereview.chromium.org/2487403002/

[l...@chromium.org]

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/loader/unload-hyperlink-targeted.html

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?


https://codereview.chromium.org/2487403002/

[dch...@chromium.org]

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/loader/unload-hyperlink-targeted.html
>
> 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.

[l...@chromium.org]

Friendly ping!

https://codereview.chromium.org/2487403002/

[l...@chromium.org]

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.


https://codereview.chromium.org/2487403002/

[dch...@chromium.org]

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?

https://codereview.chromium.org/2487403002/

[jap...@chromium.org]

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

[l...@chromium.org]

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.


https://codereview.chromium.org/2487403002/

[dch...@chromium.org]

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.

https://codereview.chromium.org/2487403002/

[l...@chromium.org]

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.


https://codereview.chromium.org/2487403002/

[dch...@chromium.org]

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?

https://codereview.chromium.org/2487403002/

[l...@chromium.org]

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.


https://codereview.chromium.org/2487403002/

[dch...@chromium.org]

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/Source/core/frame/LocalFrame.cpp
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Source/core/frame/LocalFrame.cpp#newcode929
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/Source/core/loader/FrameLoader.cpp
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode1102
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/Source/core/loader/FrameLoader.cpp#newcode1129
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/Source/core/loader/FrameLoader.cpp#newcode1159
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.

https://codereview.chromium.org/2487403002/

[l...@chromium.org]

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/Source/core/frame/LocalFrame.cpp
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Source/core/frame/LocalFrame.cpp#newcode929
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/Source/core/loader/FrameLoader.cpp
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode1102
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/Source/core/loader/FrameLoader.cpp#newcode1129
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/Source/core/loader/FrameLoader.cpp#newcode1159
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.

https://codereview.chromium.org/2487403002/

[dch...@chromium.org]

LGTM with comments addressed.

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

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode1102
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/Source/core/loader/FrameLoader.cpp#newcode1129
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/Source/core/frame/LocalFrame.cpp
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

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

https://codereview.chromium.org/2487403002/

[l...@chromium.org]

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

https://codereview.chromium.org/2487403002/diff/220001/third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode1129
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/Source/core/frame/LocalFrame.cpp
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2487403002/diff/240001/third_party/WebKit/Source/core/frame/LocalFrame.cpp#newcode928
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.

https://codereview.chromium.org/2487403002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487403002/260001


https://codereview.chromium.org/2487403002/

[commit-bot@chromium.org via codereview.chromium.org]

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

https://codereview.chromium.org/2487403002/