DOMWindow::navigator should return navigator w/o frame when detached.

DOMWindow::navigator should return a navigator w/o frame when detached.

Currently DOMWindow::navigator returns a navigator associated with a frame
even when the window is detached from the frame and another window
is attached to it. That means calling frame()->document() may return
an incorrect document, for example.

This CL makes LocalDOMWindow::navigator return a navigator with
a null frame when the window is not associated to the frame.

BUG=522791
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201055

[yhirano, 2015-08-21 14:05:11]

PS1 is crashy:


gamepad/gamepad-detached-no-crash.html
vibration/vibration-detached-no-crash.html
fast/dom/Window/property-access-on-cached-window-after-frame-removed.html
fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced.html
http/tests/navigation/beacon-detached-no-crash.html

[yhirano, 2015-08-21 14:33:29]

Tests pass with PS2 on my environment (three tests fail, but they fail without
any change).

[yhirano, 2015-08-24 02:14:33]

yhirano@chromium.org changed reviewers:
+ haraken@chromium.org, kinuko@chromium.org

[yhirano, 2015-08-24 02:14:53]

haraken@, kinuko@, can you take a look?

[haraken, 2015-08-24 02:22:16]

LGTM with a comment.

https://codereview.chromium.org/1308723003/diff/40001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1308723003/diff/40001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:664: if (frame() && frame()->domWindow() !=
this) {

It would be better to use:

  if (!isCurrentlyDisplayedInFrame())
    return nullptr;

[kinuko, 2015-08-24 02:49:53]

lgtm/2

[yhirano, 2015-08-24 03:26:55]

Patchset #4 (id:60001) has been deleted

[yhirano, 2015-08-24 03:27:11]

https://codereview.chromium.org/1308723003/diff/40001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1308723003/diff/40001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:664: if (frame() && frame()->domWindow() !=
this) {
On 2015/08/24 02:22:15, haraken wrote:
> 
> It would be better to use:
> 
>   if (!isCurrentlyDisplayedInFrame())
>     return nullptr;
> 

Done.

[yhirano, 2015-08-24 03:27:23]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-08-24 03:27:24]

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

[commit-bot: I haz the power, 2015-08-24 03:27:31]

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

[commit-bot: I haz the power, 2015-08-24 04:29:49]

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

[commit-bot: I haz the power, 2015-08-24 04:29:50]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yhirano, 2015-08-24 06:24:29]

After talking with haraken@, I checked firefox's behavior and it returns a
non-null navigator when detached. So I changed the implementation so that it
returns Navigator::create(nullptr) instead of returning null.

Can you review the change again?

[haraken, 2015-08-24 06:28:21]

LGTM

https://codereview.chromium.org/1308723003/diff/120001/Source/core/frame/Loca...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1308723003/diff/120001/Source/core/frame/Loca...
Source/core/frame/LocalDOMWindow.cpp:665: m_navigator =
Navigator::create(isCurrentlyDisplayedInFrame() ? frame() : nullptr);

Can you add a comment on this?

[yhirano, 2015-08-24 06:52:35]

https://codereview.chromium.org/1308723003/diff/120001/Source/core/frame/Loca...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1308723003/diff/120001/Source/core/frame/Loca...
Source/core/frame/LocalDOMWindow.cpp:665: m_navigator =
Navigator::create(isCurrentlyDisplayedInFrame() ? frame() : nullptr);
On 2015/08/24 06:28:21, haraken wrote:
> 
> Can you add a comment on this?

Done.

[yhirano, 2015-08-24 08:06:30]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-08-24 08:06:30]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1308723003/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-08-24 08:06:36]

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

[commit-bot: I haz the power, 2015-08-24 10:02:34]

Committed patchset #7 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201055