New issue
Advanced search Search tips

Issue 677253 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

WindowProxy should correctly maintain the lifecycle of contexts

Project Member Reported by haraken@chromium.org, Dec 28 2016

Issue description

The current lifecycle management of WindowProxy's contexts is really messy.

People are abusing WindowProxy::isContextInitialized/isGlobalInitialized without understanding when they return true/false. We should introduce a clearer lifecycle model to WindowProxy.


 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/488433ae435fbcd7ea4eac85f7042e7623f6bdef

commit 488433ae435fbcd7ea4eac85f7042e7623f6bdef
Author: haraken <haraken@chromium.org>
Date: Wed Dec 28 10:04:32 2016

Minor clean-up on WindowProxy::disposeContext()

We don't need to check isContextInitialized() in clearForClose() and
clearForNavigation() because it is already checked in WindowProxy::disposeContext().

Also this CL moves the ScriptState::Scope into disposeContext()
Though we don't need to enter ScriptState::Scope in case of clearForClose()
by accident, it looks safer to enter the scope always.

BUG= 677253 

Review-Url: https://codereview.chromium.org/2601033002
Cr-Commit-Position: refs/heads/master@{#440839}

[modify] https://crrev.com/488433ae435fbcd7ea4eac85f7042e7623f6bdef/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp

Project Member

Comment 2 by bugdroid1@chromium.org, Dec 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/45830933ad06e7f8a213e97269522469b1602863

commit 45830933ad06e7f8a213e97269522469b1602863
Author: haraken <haraken@chromium.org>
Date: Wed Dec 28 10:08:14 2016

Remove existingWindowProxy()

The only call site of existingWindowProxy() is V8Initializer::messageHandlerInMainThread().
However, as commented in the code, that code should not be needed because
if the context is during initialization it should already have bailed out at
the if(!scriptState->contextIsValid()) check at the very beginning of
V8Initializer::messageHandlerInMainThread().

If the reasoning is correct, we can simply remove all code around existingWindowProxy().

BUG= 677253 

Review-Url: https://codereview.chromium.org/2607743002
Cr-Commit-Position: refs/heads/master@{#440840}

[modify] https://crrev.com/45830933ad06e7f8a213e97269522469b1602863/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
[modify] https://crrev.com/45830933ad06e7f8a213e97269522469b1602863/third_party/WebKit/Source/bindings/core/v8/ScriptController.h
[modify] https://crrev.com/45830933ad06e7f8a213e97269522469b1602863/third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp
[modify] https://crrev.com/45830933ad06e7f8a213e97269522469b1602863/third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp
[modify] https://crrev.com/45830933ad06e7f8a213e97269522469b1602863/third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3d044ba6f81e1bd0cd255d4351a811976b80bd50

commit 3d044ba6f81e1bd0cd255d4351a811976b80bd50
Author: haraken <haraken@chromium.org>
Date: Wed Dec 28 10:44:27 2016

Remove an isContextInitialized() check from ScriptController::executeScriptInIsolatedWorld

By checking scriptState->contextIsValid(), we don't need to check isContextInitialized().

BUG= 677253 

Review-Url: https://codereview.chromium.org/2605853002
Cr-Commit-Position: refs/heads/master@{#440841}

[modify] https://crrev.com/3d044ba6f81e1bd0cd255d4351a811976b80bd50/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/403eecaa2fc7ea026528eecbdeeda3483b45ca6f

commit 403eecaa2fc7ea026528eecbdeeda3483b45ca6f
Author: haraken <haraken@chromium.org>
Date: Wed Dec 28 10:50:28 2016

Remove an isGlobalInitialized() check from WindowProxy::updateDocument()

Context states transit from Uninitialized to Initialized, and then to Detached.

- isGlobalInitialized() returns true when the context state is Initialized or Detached.
- isContextInitialized() returns true when the context state is Initialized.

Thus !isContextInitialized() is stronger than !isGlobalInitialized().
Hence we can remove if(!isGlobalInitialized()) from WindowProxy::updateDocument().

BUG= 677253 

Review-Url: https://codereview.chromium.org/2603953002
Cr-Commit-Position: refs/heads/master@{#440842}

[modify] https://crrev.com/403eecaa2fc7ea026528eecbdeeda3483b45ca6f/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b336c47b04fb8a5d5508397496d7c1e58e5681ee

commit b336c47b04fb8a5d5508397496d7c1e58e5681ee
Author: haraken <haraken@chromium.org>
Date: Thu Jan 05 12:52:27 2017

Remove an isContextInitialized() check from WindowProxyManager::updateSecurityOrigin()

The check is not needed since it is already checked in WindowProxy::updateSecurityOrigin().

BUG= 677253 

Review-Url: https://codereview.chromium.org/2606863003
Cr-Commit-Position: refs/heads/master@{#441640}

[modify] https://crrev.com/b336c47b04fb8a5d5508397496d7c1e58e5681ee/third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp

Project Member

Comment 6 by bugdroid1@chromium.org, Jan 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cbb1ee21e72f53c373d4a3468bfc8a6dfc10f514

commit cbb1ee21e72f53c373d4a3468bfc8a6dfc10f514
Author: haraken <haraken@chromium.org>
Date: Thu Jan 05 14:03:52 2017

Remove the declaration of WindowProxy::updateDocumentWrapper

The method is not defined.

BUG= 677253 

Review-Url: https://codereview.chromium.org/2612953003
Cr-Commit-Position: refs/heads/master@{#441646}

[modify] https://crrev.com/cbb1ee21e72f53c373d4a3468bfc8a6dfc10f514/third_party/WebKit/Source/bindings/core/v8/WindowProxy.h

Project Member

Comment 7 by bugdroid1@chromium.org, Jan 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5807dc6e5f6be9c395dc81b43335055f2d6a2a97

commit 5807dc6e5f6be9c395dc81b43335055f2d6a2a97
Author: haraken <haraken@chromium.org>
Date: Fri Jan 06 01:46:07 2017

Remove ScriptController::namedItemAdded/Removed

They are unnecessary indirection to WindowProxy::namedItemAdded/Removed.

Also this CL updates a dcheck in WindowProxy::namedItemAdded/Removed to verify
that those methods are called after the window proxy is initialized.

(The check will be replaced with a better lifecycle model in a follow-up CL.)

BUG= 677253 

Review-Url: https://codereview.chromium.org/2616903002
Cr-Commit-Position: refs/heads/master@{#441803}

[modify] https://crrev.com/5807dc6e5f6be9c395dc81b43335055f2d6a2a97/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
[modify] https://crrev.com/5807dc6e5f6be9c395dc81b43335055f2d6a2a97/third_party/WebKit/Source/bindings/core/v8/ScriptController.h
[modify] https://crrev.com/5807dc6e5f6be9c395dc81b43335055f2d6a2a97/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp
[modify] https://crrev.com/5807dc6e5f6be9c395dc81b43335055f2d6a2a97/third_party/WebKit/Source/core/html/HTMLDocument.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40458d4dd913d5fd6f4f1bb2da083a8a7136a9af

commit 40458d4dd913d5fd6f4f1bb2da083a8a7136a9af
Author: haraken <haraken@chromium.org>
Date: Fri Jan 06 07:09:44 2017

Remove ScriptController::initializeMainWorld

ScriptController::updateDocument is unnecessarily complicated.

Assuming that ScriptController::updateDocument is never called after
the context is detached (I added a dcheck to verify the fact in WindowProxy::updateDocument()),
we can simplify ScriptController::updateDocument.

It also enables us to remove ScriptController::initializeMainWorld.

BUG= 677253 

Review-Url: https://codereview.chromium.org/2608163009
Cr-Commit-Position: refs/heads/master@{#441896}

[modify] https://crrev.com/40458d4dd913d5fd6f4f1bb2da083a8a7136a9af/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
[modify] https://crrev.com/40458d4dd913d5fd6f4f1bb2da083a8a7136a9af/third_party/WebKit/Source/bindings/core/v8/ScriptController.h
[modify] https://crrev.com/40458d4dd913d5fd6f4f1bb2da083a8a7136a9af/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp
[modify] https://crrev.com/40458d4dd913d5fd6f4f1bb2da083a8a7136a9af/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp
[modify] https://crrev.com/40458d4dd913d5fd6f4f1bb2da083a8a7136a9af/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/40458d4dd913d5fd6f4f1bb2da083a8a7136a9af/third_party/WebKit/Source/core/page/Page.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/efc489008d2094b83736b130cb8f5349ff0c4309

commit efc489008d2094b83736b130cb8f5349ff0c4309
Author: haraken <haraken@chromium.org>
Date: Fri Jan 06 09:09:11 2017

Remove isGlobalInitialized

Now it's unused. We can remove it.

BUG= 677253 

Review-Url: https://codereview.chromium.org/2619523003
Cr-Commit-Position: refs/heads/master@{#441906}

[modify] https://crrev.com/efc489008d2094b83736b130cb8f5349ff0c4309/third_party/WebKit/Source/bindings/core/v8/WindowProxy.h

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/559b67fbc41fba740506cd5d6f3d20d20f65039b

commit 559b67fbc41fba740506cd5d6f3d20d20f65039b
Author: haraken <haraken@chromium.org>
Date: Fri Jan 06 09:20:42 2017

Remove an isContextInitialized() check from ScriptController::windowProxy()

WindowProxy::initializeIfNeeded() is no-op when isContextInitialized() returns true.
So we can remove the check from ScriptController::windowProxy() and unconditionally
call WindowProxy::initializeIfNeeded().

This CL will cause a subtle change: Before this CL, dispatchDidClearWindowObjectInMainWorld
was not called if WindowProxy gets initialized via WindowProxy::setGlobal. After this CL,
dispatchDidClearWindowObjectInMainWorld will be called always when WindowProxy gets initialized.
I don't think the difference matters (and I guess the new behavior will be more correct).

BUG= 677253 

Review-Url: https://codereview.chromium.org/2613893002
Cr-Commit-Position: refs/heads/master@{#441908}

[modify] https://crrev.com/559b67fbc41fba740506cd5d6f3d20d20f65039b/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
[modify] https://crrev.com/559b67fbc41fba740506cd5d6f3d20d20f65039b/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp

Project Member

Comment 11 by bugdroid1@chromium.org, Jan 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/381723f8f6664f04b9524e6212e218e1b1ec7510

commit 381723f8f6664f04b9524e6212e218e1b1ec7510
Author: haraken <haraken@chromium.org>
Date: Tue Jan 10 06:21:42 2017

Introduce a lifecycle model to WindowProxy

This CL intorduces the following lifecycle model to WindowProxy:

  ContextUninitialized => ContextInitialized => ContextDetached

This clarifies the lifecycle and enables us to insert dchecks to detect
invalid lifecycle transitions.

BUG= 677253 

Review-Url: https://codereview.chromium.org/2615213002
Cr-Commit-Position: refs/heads/master@{#442519}

[modify] https://crrev.com/381723f8f6664f04b9524e6212e218e1b1ec7510/third_party/WebKit/Source/bindings/core/v8/V8BindingForTesting.cpp
[modify] https://crrev.com/381723f8f6664f04b9524e6212e218e1b1ec7510/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp
[modify] https://crrev.com/381723f8f6664f04b9524e6212e218e1b1ec7510/third_party/WebKit/Source/bindings/core/v8/WindowProxy.h

Status: Fixed (was: Untriaged)

Sign in to add a comment