New issue
Advanced search Search tips

Issue 697622 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 697629



Sign in to add a comment

Multiple worker worlds could be created on the main thread

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

Issue description

Worklets create worker worlds on the main thread, but DOMWrapperWorld::create does not check for a preexisting worker world in a thread (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp?type=cs&l=96). So, we could have multiple worker worlds in the main thread, but only the most recently created one is tracked by DOMWrapperWorld (the others would still exist because of other refs). We should probably reuse a worker world if it has already been created in a thread.  
 
Blocking: 697629
adithyas@ could you possibly give more background about how you discovered this bug and its severity? What's the impact of DOMWrapperWorld not tracking a worker world?

Adding blocking  issue 697629  based on comment #1 there.
I'm not completely sure on the full severity of this bug (I don't know if this by itself causes any issues). 

I ran into this issue while trying to understand how all "non main" worlds are created and tracked, and it doesn't look like worker worlds created on the main thread are handled correctly. Functions like DOMWrapperWorld::markWrappersInAllWorlds and DOMWrapperWorld::allWorldsInMainThread do not account for main thread worker worlds at all, and resolving this issue would make it easier to fix those methods.
It would certainly only affect worklets, which AFAIK have not yet shipped. So severity is limited by the fact that it shouldn't affect real-world usage yet.
WorkerOrWorkletScriptController::dispose seems to always dispose the world it is associated with, so we have both places that assume that this world is a per-thread singleton, and places that assume they are per-worker-or-worklet.
Cc: -nhiroki@chromium.org
Labels: -Pri-3 OS-All Pri-2
Owner: nhiroki@chromium.org
Status: Assigned (was: Untriaged)
> It would certainly only affect worklets, which AFAIK have not yet shipped. So severity is limited by the fact that it shouldn't affect real-world usage yet.

You're right. This affects only PaintWorklet living on the main thread and it hasn't been shipped yet.

Managing WorkerWorlds using a map like IsolatedWorlds could be a solution. I'll work on it...
> You're right. This affects only PaintWorklet living on the main thread and it hasn't been shipped yet.

Sorry, this affects not only MainThreadWorklet (PaintWorklet) but also ThreadedWorklet (AnimationWorklet, AudioWorklet). Anyway, they haven't been shipped yet.
Status: Started (was: Assigned)
CL: https://codereview.chromium.org/2735823006/
Project Member

Comment 8 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 9 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 10 by bugdroid1@chromium.org, Mar 16 2017

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

Comment 12 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 13 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