New issue
Advanced search Search tips

Issue 697629 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 697622



Sign in to add a comment

DOMWrapperWorld::allWorldsInMainThread doesn't account for worker worlds created on the main thread

Project Member Reported by adithyas@chromium.org, Mar 1 2017

Issue description

Worklets can create worker worlds on the main thread. The current implementation doesn't account for these worlds. Either the name of the method should be changed, or its implementation.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp?type=cs&l=128
 
Status: Available (was: Untriaged)
I think this should probably include the worker world (which can exist on the main thread now, due to worklets). It looks like this could cause array buffer wrappers to not be properly neutered in postMessage out of such a worklet.

Once  bug 697622  is fixed, this should be as simple as pulling the worker world out of TLS, if it exists.
Blockedon: 697622
Labels: -Pri-3 OS-All Pri-2
Owner: nhiroki@chromium.org
Status: Started (was: Available)
CL: https://codereview.chromium.org/2735823006/
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 9 2017

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

commit 611dfce737a7df5d5a1e88e8528d405ebed37b8d
Author: nhiroki <nhiroki@chromium.org>
Date: Thu Mar 09 08:44:55 2017

Bindings: Separate WorldIdConstants to WorldTypes and WorldId

This is a clean-up CL and doesn't change behavior.

For improving extensibility, this CL separates WorldIdConstants to 2 parts:
WorldTypes and WorldId. This encapsulates identifier allocation logic in
DOMWrapperWorld[1] and makes it easier to expand the identifier space for
Worklets[2].

[1] WorldIds for IsolatedWorlds still need to be given from out of
    DOMWrapperWorld because of its unique convention to allocate the identifier.
[2] https://codereview.chromium.org/2735823006/

BUG= 697622 ,  697629 

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

[modify] https://crrev.com/611dfce737a7df5d5a1e88e8528d405ebed37b8d/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp
[modify] https://crrev.com/611dfce737a7df5d5a1e88e8528d405ebed37b8d/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h
[modify] https://crrev.com/611dfce737a7df5d5a1e88e8528d405ebed37b8d/third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp
[modify] https://crrev.com/611dfce737a7df5d5a1e88e8528d405ebed37b8d/third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp
[modify] https://crrev.com/611dfce737a7df5d5a1e88e8528d405ebed37b8d/third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
[modify] https://crrev.com/611dfce737a7df5d5a1e88e8528d405ebed37b8d/third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp
[modify] https://crrev.com/611dfce737a7df5d5a1e88e8528d405ebed37b8d/third_party/WebKit/Source/modules/fetch/DataConsumerHandleTestUtil.cpp
[modify] https://crrev.com/611dfce737a7df5d5a1e88e8528d405ebed37b8d/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 14 2017

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

commit 0fe6bc1c1b65086da86d225ac7dfe9b00a317a1e
Author: nhiroki <nhiroki@chromium.org>
Date: Tue Mar 14 13:22:54 2017

Bindings: Manage multiple DOMWrapperWorlds for worklets

Before this CL, there are 2 issues:

1) DOMWrapperWorld for a worker-or-worklet is managed as per-thread singleton.
   This doesn't work when multiple worklets are created on the same thread.
2) DOMWrapperWorld assumes that a worker-or-worklet world is never created on
   the main thread, but actually it's possible for the main thread worklet
   (i.e. PaintWorklet).

This CL introduces a per-thread |worldMap| to manage multiple worker-or-worklet
worlds on a single thread for the issue 1), and teaches the main thread that
there can be worker-or-worklet worlds for the  issue 2 ).

BUG= 697622 ,  697629 

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

[modify] https://crrev.com/0fe6bc1c1b65086da86d225ac7dfe9b00a317a1e/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp
[modify] https://crrev.com/0fe6bc1c1b65086da86d225ac7dfe9b00a317a1e/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 16 2017

Labels: M-59
Status: Fixed (was: Started)
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 17 2017

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

commit 1d85fa7d0be347292a5805ec87ca3d2d10a8c57f
Author: nhiroki <nhiroki@chromium.org>
Date: Fri Mar 17 12:24:56 2017

Bindings: Remove DOMWrapperWorld::fromWorldId() for cleanup

This CL removes DOMWrapperWorld::fromWorldId() that looks like a simple getter
but actually has a side effect like this:
1) When a given id is for the main world, ensures the main world and returns it.
2) When a given id is for an isolated world, ensures the isolated world and
   returns it.
3) Otherwise, looks up a given id in the world map and then returns the finding.
   If the world doesn't exist, returns nullptr.

After this CL, mainWorld() is used for 1) and ensureIsolatedWorld() is used for
2). fromWorldId() hasn't been used for 3), so this CL does not introduce an
alternative for the case.

Also, this CL pushes the main world into the world map for cleanup. Before this
CL, the main world is managed separately from other worlds. Managing all worlds
in the single map would be simpler.

BUG= 697622 ,  697629 

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

[modify] https://crrev.com/1d85fa7d0be347292a5805ec87ca3d2d10a8c57f/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp
[modify] https://crrev.com/1d85fa7d0be347292a5805ec87ca3d2d10a8c57f/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h
[modify] https://crrev.com/1d85fa7d0be347292a5805ec87ca3d2d10a8c57f/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorldTest.cpp
[modify] https://crrev.com/1d85fa7d0be347292a5805ec87ca3d2d10a8c57f/third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp
[modify] https://crrev.com/1d85fa7d0be347292a5805ec87ca3d2d10a8c57f/third_party/WebKit/Source/web/SuspendableScriptExecutor.h
[modify] https://crrev.com/1d85fa7d0be347292a5805ec87ca3d2d10a8c57f/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Mar 29 2017

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

commit 3e5d3fc45d06833189606b0ad750b9dee95ef50a
Author: nhiroki <nhiroki@chromium.org>
Date: Wed Mar 29 05:53:18 2017

Bindings: Manage the main world separately from other worlds

This CL speculatively reverts a part of the CL[1].

The CL[1] pushed the main world into the world map for cleanup, but that CL
seems to cause a performance regression on a perf test (see  issue 704778 )
probably because of changing the hashmap traits and/or the world map management.

[1] https://codereview.chromium.org/2753013003/

BUG= 697622 ,  697629 ,  704778 

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

[modify] https://crrev.com/3e5d3fc45d06833189606b0ad750b9dee95ef50a/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp
[modify] https://crrev.com/3e5d3fc45d06833189606b0ad750b9dee95ef50a/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorldTest.cpp

Sign in to add a comment