[wrapper-tracing] For a worker, mark relative to its wrapper map.

[wrapper-tracing] For a worker, mark relative to its wrapper map.

If marking a ScriptWrappable's wrapper object in a worker context, do that by
consulting the worker's underlying DOMWrapperMap.

Store the worker world in TLS, as we cannot rely on the context of the isolate
during marking as we might mark without a stack (and thus context).

BUG=chromium:686563
patch from issue 2663643002 at patchset 140001 (http://crrev.com/2663643002#ps140001)

Review-Url: https://codereview.chromium.org/2665733002
Cr-Commit-Position: refs/heads/master@{#447604}
Committed: https://chromium.googlesource.com/chromium/src/+/ae599161857edda9ecbed70494c220f7f711600f

[Michael Lippautz, 2017-01-30 22:50:35]

Patchset #2 (id:20001) has been deleted

[Michael Lippautz, 2017-01-31 05:18:26]

Patchset #3 (id:60001) has been deleted

[Michael Lippautz, 2017-01-31 05:18:32]

Patchset #2 (id:40001) has been deleted

[Michael Lippautz, 2017-01-31 05:18:50]

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

[commit-bot: I haz the power, 2017-01-31 05:19:11]

Dry run: 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-01-31 07:22:14]

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

[commit-bot: I haz the power, 2017-01-31 07:22:16]

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

[Michael Lippautz, 2017-02-01 07:55:13]

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

[commit-bot: I haz the power, 2017-02-01 07:55:32]

Dry run: 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-02-01 09:40:58]

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

[commit-bot: I haz the power, 2017-02-01 09:40:58]

Dry run: This issue passed the CQ dry run.

[Michael Lippautz, 2017-02-01 15:03:52]

Description was changed from

==========
[wrapper-tracing] For a worker, mark relative to its wrapper map.

If marking a ScriptWrappable's wrapper object in a worker context, do that by
consulting the worker's underlying DOMWrapperMap.

BUG=686563

patch from issue 2663643002 at patchset 140001
(http://crrev.com/2663643002#ps140001)
==========

to

==========
[wrapper-tracing] For a worker, mark relative to its wrapper map.

If marking a ScriptWrappable's wrapper object in a worker context, do that by
consulting the worker's underlying DOMWrapperMap.

We cannot rely on the context of the isolate during marking as we might mark
without
a stack (and thus context).

BUG=686563

patch from issue 2663643002 at patchset 140001
(http://crrev.com/2663643002#ps140001)
==========

[Michael Lippautz, 2017-02-01 15:04:39]

Description was changed from

==========
[wrapper-tracing] For a worker, mark relative to its wrapper map.

If marking a ScriptWrappable's wrapper object in a worker context, do that by
consulting the worker's underlying DOMWrapperMap.

We cannot rely on the context of the isolate during marking as we might mark
without
a stack (and thus context).

BUG=686563

patch from issue 2663643002 at patchset 140001
(http://crrev.com/2663643002#ps140001)
==========

to

==========
[wrapper-tracing] For a worker, mark relative to its wrapper map.

If marking a ScriptWrappable's wrapper object in a worker context, do that by
consulting the worker's underlying DOMWrapperMap.

Store the worker world in TLS, as we cannot rely on the context of the isolate
during marking as we might mark without a stack (and thus context).

BUG=686563

patch from issue 2663643002 at patchset 140001
(http://crrev.com/2663643002#ps140001)
==========

[Michael Lippautz, 2017-02-01 15:05:58]

Description was changed from

==========
[wrapper-tracing] For a worker, mark relative to its wrapper map.

If marking a ScriptWrappable's wrapper object in a worker context, do that by
consulting the worker's underlying DOMWrapperMap.

Store the worker world in TLS, as we cannot rely on the context of the isolate
during marking as we might mark without a stack (and thus context).

BUG=686563

patch from issue 2663643002 at patchset 140001
(http://crrev.com/2663643002#ps140001)
==========

to

==========
[wrapper-tracing] For a worker, mark relative to its wrapper map.

If marking a ScriptWrappable's wrapper object in a worker context, do that by
consulting the worker's underlying DOMWrapperMap.

Store the worker world in TLS, as we cannot rely on the context of the isolate
during marking as we might mark without a stack (and thus context).

BUG=686563

patch from issue 2663643002 at patchset 140001
(http://crrev.com/2663643002#ps140001)
==========

[Michael Lippautz, 2017-02-01 15:05:58]

mlippautz@chromium.org changed reviewers:
+ haraken@chromium.org, jochen@chromium.org, sigbjorn@opera.com

[Michael Lippautz, 2017-02-01 15:06:28]

ptal; body still slightly aligned to  CET ;)

[Michael Lippautz, 2017-02-01 15:06:36]

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

[commit-bot: I haz the power, 2017-02-01 15:06:47]

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

[NGG1, 2017-02-01 16:00:46]

ngg@tresorit.com changed reviewers:
+ ngg@tresorit.com

[NGG1, 2017-02-01 16:00:47]

It seems to work for me now both with self.gc() and forcing it from the
DevConsole. lgtm

https://codereview.chromium.org/2665733002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2665733002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:143: v8::Isolate*
isolate = ThreadState::current()->isolate();
isolate is not used anymore (InContext() was removed), do you really need to
check if it is not null?

[haraken, 2017-02-01 16:18:17]

https://codereview.chromium.org/2665733002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2665733002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:108:
DOMWrapperWorld*& DOMWrapperWorld::workerWorld() {

Can we avoid introducing workerWorld() by using
DOMWrapperWorld::current(isolate)? I think that 
DOMWrapperWorld::current(isolate) should return the worker world.

[NGG1, 2017-02-01 16:25:28]

https://codereview.chromium.org/2665733002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2665733002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:108:
DOMWrapperWorld*& DOMWrapperWorld::workerWorld() {
On 2017/02/01 at 16:18:16, haraken wrote:
> Can we avoid introducing workerWorld() by using
DOMWrapperWorld::current(isolate)? I think that 
DOMWrapperWorld::current(isolate) should return the worker world.

DOMWrapperWorld::current(isolate) creates a handle to the context which (if I
understand correctly) should be avoided during GC.

[Michael Lippautz, 2017-02-01 16:30:29]

On 2017/02/01 16:18:17, haraken wrote:
>
https://codereview.chromium.org/2665733002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):
> 
>
https://codereview.chromium.org/2665733002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:108:
> DOMWrapperWorld*& DOMWrapperWorld::workerWorld() {
> 
> Can we avoid introducing workerWorld() by using
> DOMWrapperWorld::current(isolate)? I think that 
> DOMWrapperWorld::current(isolate) should return the worker world.

Unfortunately not always: (1) we need to have a context for this to work which
we don't always have, and (2) getting the context would require a handle which
we cannot create.

In general we cannot use the context in this method.

[Michael Lippautz, 2017-02-01 16:36:21]

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

[commit-bot: I haz the power, 2017-02-01 16:36:30]

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

[haraken, 2017-02-01 17:23:02]

https://codereview.chromium.org/2665733002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2665733002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:108:
DOMWrapperWorld*& DOMWrapperWorld::workerWorld() {

Can you unify mainWorld() and workerWorld() into one method like
perThreadWorld()?

perThreadWorld() creates a new world and sets it on ThreadSpecific. The main
thread or a worker thread can obtain its world by calling perThreadWorld().

I guess it will clean up the code a bit. (You no longer need to set/resert
workerWorld().)

https://codereview.chromium.org/2665733002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:144: if (isolate)
{

Can this be null? If this is null, we'll end up with missing marking the object.

https://codereview.chromium.org/2665733002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:192: if
(m_worldId == WorkerWorldId) {

This must be true, right? The line 188 is saying that this call path should not
hit on the main thread.

[Michael Lippautz, 2017-02-01 18:00:38]

https://codereview.chromium.org/2665733002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2665733002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:108:
DOMWrapperWorld*& DOMWrapperWorld::workerWorld() {
On 2017/02/01 17:23:02, haraken wrote:
> 
> Can you unify mainWorld() and workerWorld() into one method like
> perThreadWorld()?
> 
> perThreadWorld() creates a new world and sets it on ThreadSpecific. The main
> thread or a worker thread can obtain its world by calling perThreadWorld().
> 
> I guess it will clean up the code a bit. (You no longer need to set/resert
> workerWorld().)

Would you be fine with doing that in a separate patch? I'd really like to get
the release block stable bug out of the way first.

The non-trivial part is that the creation of mainWorld is bundled in mainWorld
while workerWorld creation happens lazily. I'd rather have this cleanup sit in a
separate patch.

https://codereview.chromium.org/2665733002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:144: if (isolate)
{
On 2017/02/01 17:23:02, haraken wrote:
> 
> Can this be null? If this is null, we'll end up with missing marking the
object.

Done.

https://codereview.chromium.org/2665733002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:192: if
(m_worldId == WorkerWorldId) {
On 2017/02/01 17:23:02, haraken wrote:
> 
> This must be true, right? The line 188 is saying that this call path should
not
> hit on the main thread.

Could also be an isolated world as far as I can see.

[Michael Lippautz, 2017-02-01 18:00:43]

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

[commit-bot: I haz the power, 2017-02-01 18:01:13]

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

[haraken, 2017-02-01 18:57:56]

On 2017/02/01 18:00:38, Michael Lippautz (slow travel) wrote:
>
https://codereview.chromium.org/2665733002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):
> 
>
https://codereview.chromium.org/2665733002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:108:
> DOMWrapperWorld*& DOMWrapperWorld::workerWorld() {
> On 2017/02/01 17:23:02, haraken wrote:
> > 
> > Can you unify mainWorld() and workerWorld() into one method like
> > perThreadWorld()?
> > 
> > perThreadWorld() creates a new world and sets it on ThreadSpecific. The main
> > thread or a worker thread can obtain its world by calling perThreadWorld().
> > 
> > I guess it will clean up the code a bit. (You no longer need to set/resert
> > workerWorld().)
> 
> Would you be fine with doing that in a separate patch? I'd really like to get
> the release block stable bug out of the way first.
> 
> The non-trivial part is that the creation of mainWorld is bundled in mainWorld
> while workerWorld creation happens lazily. I'd rather have this cleanup sit in
a
> separate patch.

Makes sense. LGTM with that fixed in a separate patch.

[commit-bot: I haz the power, 2017-02-01 20:49:28]

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

[commit-bot: I haz the power, 2017-02-01 20:49:29]

Dry run: This issue passed the CQ dry run.

[Michael Lippautz, 2017-02-01 20:56:24]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2017-02-01 20:56:24]

The patchset sent to the CQ was uploaded after l-g-t-m from ngg@tresorit.com
Link to the patchset: https://codereview.chromium.org/2665733002/#ps160001
(title: "Address comments")

[commit-bot: I haz the power, 2017-02-01 20:56:45]

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

[Michael Lippautz, 2017-02-01 20:57:59]

Description was changed from

==========
[wrapper-tracing] For a worker, mark relative to its wrapper map.

If marking a ScriptWrappable's wrapper object in a worker context, do that by
consulting the worker's underlying DOMWrapperMap.

Store the worker world in TLS, as we cannot rely on the context of the isolate
during marking as we might mark without a stack (and thus context).

BUG=686563

patch from issue 2663643002 at patchset 140001
(http://crrev.com/2663643002#ps140001)
==========

to

==========
[wrapper-tracing] For a worker, mark relative to its wrapper map.

If marking a ScriptWrappable's wrapper object in a worker context, do that by
consulting the worker's underlying DOMWrapperMap.

Store the worker world in TLS, as we cannot rely on the context of the isolate
during marking as we might mark without a stack (and thus context).

BUG=chromium:686563

patch from issue 2663643002 at patchset 140001
(http://crrev.com/2663643002#ps140001)
==========

[commit-bot: I haz the power, 2017-02-01 21:01:19]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485982584351830,
"parent_rev": "e33ba85e0048b6998e3c7fbb02a6cece1ceae07b", "commit_rev":
"ae599161857edda9ecbed70494c220f7f711600f"}

[commit-bot: I haz the power, 2017-02-01 21:03:02]

Description was changed from

==========
[wrapper-tracing] For a worker, mark relative to its wrapper map.

If marking a ScriptWrappable's wrapper object in a worker context, do that by
consulting the worker's underlying DOMWrapperMap.

Store the worker world in TLS, as we cannot rely on the context of the isolate
during marking as we might mark without a stack (and thus context).

BUG=chromium:686563

patch from issue 2663643002 at patchset 140001
(http://crrev.com/2663643002#ps140001)
==========

to

==========
[wrapper-tracing] For a worker, mark relative to its wrapper map.

If marking a ScriptWrappable's wrapper object in a worker context, do that by
consulting the worker's underlying DOMWrapperMap.

Store the worker world in TLS, as we cannot rely on the context of the isolate
during marking as we might mark without a stack (and thus context).

BUG=chromium:686563

patch from issue 2663643002 at patchset 140001
(http://crrev.com/2663643002#ps140001)

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

[commit-bot: I haz the power, 2017-02-01 21:03:04]

Committed patchset #6 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/ae599161857edda9ecbed70494c2...

[sof, 2017-02-01 22:05:18]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2017-02-01 22:05:19]

lgtm

https://codereview.chromium.org/2665733002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2665733002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:95: if (worldId
== WorkerWorldId) {
nit: isWorkerWorld()

https://codereview.chromium.org/2665733002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:190: if
(m_worldId == WorkerWorldId) {
isWorkerWorld()

[sof, 2017-02-06 11:45:49]

Not entirely sure, but it does look like this added a flaky crash,


https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7__dbg...

[sof, 2017-02-06 12:45:01]

On 2017/02/06 11:45:49, sof wrote:
> Not entirely sure, but it does look like this added a flaky crash,
> 
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7__dbg...

Better stack here,
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Tru...

looks like the workerWorld() unregistration needs to (eagerly) happen in
DOMWrapperWorld::dispose().