Reinitialize WindowProxy on navigations.

Ensure WindowProxy reinitialization on navigations if needed.

When returning a Window reference to JS, Blink calls ToV8() to
convert the C++ DOMWindow pointer to an associated JS DOM wrapper,
which ensures the associated WindowProxy is initialized. During a
navigation that requires switching DOMWindows, the WindowProxy is
detached from the old DOMWindow by calling setDOMWindow() and
later reattached to the new DOMWindow by calling
ScriptController::updateDocument().

As an optimization, 246e25c5bd72fac9dce3b9b1254e5590d0444d09
skipped initialization of the WindowProxy if it's global proxy
reference was null. Skipping initialization is safe here since a
null global proxy reference means that ToV8() was never called,
and JS cannot hold any references to the Window object. Thus, it
is unnecessary to ensure that the global proxy is attached to a
DOMWindow.

40458d4dd913d5fd6f4f1bb2da083a8a7136a9af cleaned up the complex
WindowProxy initialization logic but introduced a subtle bug:
it changed updateDocument() to be a no-op if the WindowProxy is
not initialized. Unfortunately, this means that any existing
script references to that Window object will be broken unless
WindowProxy reinitialization is triggered by something else,
such as loading a <script> tag in the new Document or getting
a new reference to the Window.

BUG=690178
Review-Url: https://codereview.chromium.org/2732483004
Cr-Commit-Position: refs/heads/master@{#454552}
Committed: https://chromium.googlesource.com/chromium/src/+/bee44e2b6c7a93b28e1082743784045447498476

[Yuki, 2017-03-03 07:51:04]

yukishiino@chromium.org changed reviewers:
+ yukishiino@chromium.org

[Yuki, 2017-03-03 07:51:04]

Not directly related to this CL, but I'm wondering that we probably should
distinguish "detached because of navigation, so reinitialization is expected"
from "detached, reinitialization is not expected".  WindowProxy::LifeCycle
currently does not distinguish these two cases.

Maybe, we'd need to address it later.  This CL itself looks a right way.

[dcheng, 2017-03-03 07:55:48]

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

[commit-bot: I haz the power, 2017-03-03 07:56:02]

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

[dcheng, 2017-03-03 08:22:36]

Description was changed from

==========
Reinitialize WindowProxy on navigations.

DO NOT COMMIT, THIS LOOPS INFINITELY.

BUG=690178
==========

to

==========
Ensure WindowProxy reinitialization on navigations if needed.

When returning a Window reference to JS, Blink calls ToV8() to
convert the C++ DOMWindow pointer to an associated JS DOM wrapper,
which ensures the associated WindowProxy is initialized. During a
navigation that requires switching DOMWindows, the WindowProxy is
detached from the old DOMWindow by calling setDOMWindow() and
later reattached to the new DOMWindow by calling
ScriptController::updateDocument().

As an optimization, 246e25c5bd72fac9dce3b9b1254e5590d0444d09
skipped initialization of the WindowProxy if it's global proxy
reference was null. Skipping initialization is safe here since a
null global proxy reference means that ToV8() was never called,
and JS cannot hold any references to the Window object. Thus, it
is unnecessary to ensure that the global proxy is attached to a
DOMWindow.

40458d4dd913d5fd6f4f1bb2da083a8a7136a9af cleaned up the complex
WindowProxy initialization logic but introduced a subtle bug:
it changed updateDocument() to be a no-op if the WindowProxy is
not initialized. Unfortunately, this means that any existing
script references to that Window object will be broken unless
WindowProxy reinitialization is triggered by something else,
such as loading a <script> tag in the new Document or getting
a new reference to the Window.

BUG=690178
==========

[dcheng, 2017-03-03 08:23:28]

On 2017/03/03 07:51:04, Yuki(slow_til_mar08) wrote:
> Not directly related to this CL, but I'm wondering that we probably should
> distinguish "detached because of navigation, so reinitialization is expected"
> from "detached, reinitialization is not expected".  WindowProxy::LifeCycle
> currently does not distinguish these two cases.

Yeah, I agree. This is pretty subtle and hard to understand.

> 
> Maybe, we'd need to address it later.  This CL itself looks a right way.

Yeah, this needs to be merged to M57 and M58 =/

PTAL, test is fixed.

[Yuki, 2017-03-03 08:28:53]

LGTM!  Thanks for the fix.

[dcheng, 2017-03-03 08:38:40]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2017-03-03 08:38:41]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2017-03-03 08:38: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-03-03 09:25:02]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488530321032820,
"parent_rev": "4830d657333322f85bb11a5ff431aaab85d8cb3c", "commit_rev":
"bee44e2b6c7a93b28e1082743784045447498476"}

[commit-bot: I haz the power, 2017-03-03 09:25:44]

Description was changed from

==========
Ensure WindowProxy reinitialization on navigations if needed.

When returning a Window reference to JS, Blink calls ToV8() to
convert the C++ DOMWindow pointer to an associated JS DOM wrapper,
which ensures the associated WindowProxy is initialized. During a
navigation that requires switching DOMWindows, the WindowProxy is
detached from the old DOMWindow by calling setDOMWindow() and
later reattached to the new DOMWindow by calling
ScriptController::updateDocument().

As an optimization, 246e25c5bd72fac9dce3b9b1254e5590d0444d09
skipped initialization of the WindowProxy if it's global proxy
reference was null. Skipping initialization is safe here since a
null global proxy reference means that ToV8() was never called,
and JS cannot hold any references to the Window object. Thus, it
is unnecessary to ensure that the global proxy is attached to a
DOMWindow.

40458d4dd913d5fd6f4f1bb2da083a8a7136a9af cleaned up the complex
WindowProxy initialization logic but introduced a subtle bug:
it changed updateDocument() to be a no-op if the WindowProxy is
not initialized. Unfortunately, this means that any existing
script references to that Window object will be broken unless
WindowProxy reinitialization is triggered by something else,
such as loading a <script> tag in the new Document or getting
a new reference to the Window.

BUG=690178
==========

to

==========
Ensure WindowProxy reinitialization on navigations if needed.

When returning a Window reference to JS, Blink calls ToV8() to
convert the C++ DOMWindow pointer to an associated JS DOM wrapper,
which ensures the associated WindowProxy is initialized. During a
navigation that requires switching DOMWindows, the WindowProxy is
detached from the old DOMWindow by calling setDOMWindow() and
later reattached to the new DOMWindow by calling
ScriptController::updateDocument().

As an optimization, 246e25c5bd72fac9dce3b9b1254e5590d0444d09
skipped initialization of the WindowProxy if it's global proxy
reference was null. Skipping initialization is safe here since a
null global proxy reference means that ToV8() was never called,
and JS cannot hold any references to the Window object. Thus, it
is unnecessary to ensure that the global proxy is attached to a
DOMWindow.

40458d4dd913d5fd6f4f1bb2da083a8a7136a9af cleaned up the complex
WindowProxy initialization logic but introduced a subtle bug:
it changed updateDocument() to be a no-op if the WindowProxy is
not initialized. Unfortunately, this means that any existing
script references to that Window object will be broken unless
WindowProxy reinitialization is triggered by something else,
such as loading a <script> tag in the new Document or getting
a new reference to the Window.

BUG=690178

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

[commit-bot: I haz the power, 2017-03-03 09:25:45]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/bee44e2b6c7a93b28e1082743784...

[haraken, 2017-03-05 07:12:47]

LGTM, thanks for the fix! I apologize I couldn't work on this.