Always ignore navigation in Document::detach() in LocalFrame::navigate()

Always ignore navigation in Document::detach() in LocalFrame::navigate()

We already checked that FrameNavigation is enabled before trying to
schedule a LocationChange; however, it was possible to construct a
scenario with an opened window that would use the sync loading path and
bypass this check.

BUG=597532
Committed: https://crrev.com/f93a0e557dd97cc83d8b55953c2f57c5e2dfe07b
Cr-Commit-Position: refs/heads/master@{#383627}

[dcheng, 2016-03-28 21:49:02]

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

[dcheng, 2016-03-28 21:50:59]

Similar to https://codereview.chromium.org/1444183003, but covers the remaining
branch.

This feels pretty meh as a fix (I'd really prefer to do something like
https://crbug.com/561683, but that is proving very difficult to implement). I
also don't have a test, but I'm going to manually test. I have an idea for
followup patches to make this testable as well (using the Blink deprecated test
PPAPI plugin).

[Nate Chapin, 2016-03-28 21:55:44]

Test?

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

https://codereview.chromium.org/1840813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:285: if
(isNavigationAllowed() && isMainFrame() &&
!m_loader.stateMachine()->committedFirstRealDocumentLoad()) {
Would it be cleaner to call the other LocalFrame::navigate() in this if() branch
instead of adding the isNavigationAllowed() check here?

[dcheng, 2016-03-28 22:24:36]

On 2016/03/28 at 21:55:44, japhet wrote:
> Test?

An automated layout test is trickier since it involves using a plugin. I'd like
to do it in a followup to avoid delaying this fix.

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

https://codereview.chromium.org/1840813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:285: if
(isNavigationAllowed() && isMainFrame() &&
!m_loader.stateMachine()->committedFirstRealDocumentLoad()) {
On 2016/03/28 at 21:55:43, Nate Chapin wrote:
> Would it be cleaner to call the other LocalFrame::navigate() in this if()
branch instead of adding the isNavigationAllowed() check here?

Done.

[Nate Chapin, 2016-03-28 22:25:52]

LGTM. Will a unit test not work either?

[dcheng, 2016-03-28 22:30:04]

On 2016/03/28 at 22:25:52, japhet wrote:
> LGTM. Will a unit test not work either?

Not easily, we don't have the unit test infrastructure hooked up for a mock
plugin =(

[dcheng, 2016-03-28 22:30:07]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-03-28 22:30:37]

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

[Nate Chapin, 2016-03-28 22:30:53]

On 2016/03/28 22:30:04, dcheng wrote:
> On 2016/03/28 at 22:25:52, japhet wrote:
> > LGTM. Will a unit test not work either?
> 
> Not easily, we don't have the unit test infrastructure hooked up for a mock
> plugin =(

k. :/

[commit-bot: I haz the power, 2016-03-29 00:16:31]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-29 00:18:44]

Description was changed from

==========
Always ignore navigation in Document::detach() in LocalFrame::navigate()

We already checked that FrameNavigation is enabled before trying to
schedule a LocationChange; however, it was possible to construct a
scenario with an opened window that would use the sync loading path and
bypass this check.

BUG=597532
==========

to

==========
Always ignore navigation in Document::detach() in LocalFrame::navigate()

We already checked that FrameNavigation is enabled before trying to
schedule a LocationChange; however, it was possible to construct a
scenario with an opened window that would use the sync loading path and
bypass this check.

BUG=597532

Committed: https://crrev.com/f93a0e557dd97cc83d8b55953c2f57c5e2dfe07b
Cr-Commit-Position: refs/heads/master@{#383627}
==========

[commit-bot: I haz the power, 2016-03-29 00:18:45]

Patchset 2 (id:??) landed as
https://crrev.com/f93a0e557dd97cc83d8b55953c2f57c5e2dfe07b
Cr-Commit-Position: refs/heads/master@{#383627}