For a worker, mark relative to its wrapper map.

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.

R=jochen,haraken
BUG=686563

[sof, 2017-01-29 07:15:28]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-29 07:15:37]

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-29 08:19:57]

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

[commit-bot: I haz the power, 2017-01-29 08:19:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sof, 2017-01-29 08:28:21]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-29 08:28:26]

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-29 09:36:57]

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

[commit-bot: I haz the power, 2017-01-29 09:36:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sof, 2017-01-29 14:18:11]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-29 14:18:22]

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

[sof, 2017-01-29 15:13:48]

Description was changed from

==========
Handle marking in worker wrapper worlds.

R=
BUG=683406
==========

to

==========
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.

R=
BUG=683406
==========

[jochen (gone - plz use gerrit), 2017-01-29 15:50:43]

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

[jochen (gone - plz use gerrit), 2017-01-29 15:50:44]

https://codereview.chromium.org/2663643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2663643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:145:
DOMWrapperWorld* wrapperWorld =
alternatively, we could stick the wrapper map into a thread local, wdyt?

[commit-bot: I haz the power, 2017-01-29 16:06:13]

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

[commit-bot: I haz the power, 2017-01-29 16:06:13]

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_...)

[sof, 2017-01-29 16:48:51]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-29 16:48:58]

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

[haraken, 2017-01-29 16:49:31]

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

[haraken, 2017-01-29 16:49:32]

https://codereview.chromium.org/2663643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2663643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:145:
DOMWrapperWorld* wrapperWorld =
On 2017/01/29 15:50:44, jochen (travelling til Feb 4) wrote:
> alternatively, we could stick the wrapper map into a thread local, wdyt?

Yeah, if we move m_world to V8PerContextData, we can use the same code path for
the main thread and a worker thread.

I'm not sure how much work it will be. This CL looks good as an immediate fix.

[sof, 2017-01-29 16:52:13]

https://codereview.chromium.org/2663643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2663643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:145:
DOMWrapperWorld* wrapperWorld =
On 2017/01/29 15:50:44, jochen (travelling til Feb 4) wrote:
> alternatively, we could stick the wrapper map into a thread local, wdyt?

That would cut out the middle men here, ps#4 tries that.

I added clearing of the TLS entry upon script controller disposal, to make its
non-null lifetime known & clear.

[jochen (gone - plz use gerrit), 2017-01-29 16:55:31]

lgtm

[haraken, 2017-01-29 16:57:13]

On 2017/01/29 16:52:13, sof wrote:
>
https://codereview.chromium.org/2663643002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):
> 
>
https://codereview.chromium.org/2663643002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:145:
> DOMWrapperWorld* wrapperWorld =
> On 2017/01/29 15:50:44, jochen (travelling til Feb 4) wrote:
> > alternatively, we could stick the wrapper map into a thread local, wdyt?
> 
> That would cut out the middle men here, ps#4 tries that.
> 
> I added clearing of the TLS entry upon script controller disposal, to make its
> non-null lifetime known & clear.

This will work but looks a bit weird to create a thread-local storage since
V8PerContextData is a "thread-local" storage for V8-related per-thread things...

[sof, 2017-01-29 17:24:01]

On 2017/01/29 16:57:13, haraken wrote:
> On 2017/01/29 16:52:13, sof wrote:
> >
>
https://codereview.chromium.org/2663643002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2663643002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:145:
> > DOMWrapperWorld* wrapperWorld =
> > On 2017/01/29 15:50:44, jochen (travelling til Feb 4) wrote:
> > > alternatively, we could stick the wrapper map into a thread local, wdyt?
> > 
> > That would cut out the middle men here, ps#4 tries that.
> > 
> > I added clearing of the TLS entry upon script controller disposal, to make
its
> > non-null lifetime known & clear.
> 
> This will work but looks a bit weird to create a thread-local storage since
> V8PerContextData is a "thread-local" storage for V8-related per-thread
things...

That makes good sense, but I overlooked one DOMWrapperWorld "cow path"
initially. ps#5 simplifies - looks ok?

[haraken, 2017-01-29 17:25:02]

On 2017/01/29 17:24:01, sof wrote:
> On 2017/01/29 16:57:13, haraken wrote:
> > On 2017/01/29 16:52:13, sof wrote:
> > >
> >
>
https://codereview.chromium.org/2663643002/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2663643002/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:145:
> > > DOMWrapperWorld* wrapperWorld =
> > > On 2017/01/29 15:50:44, jochen (travelling til Feb 4) wrote:
> > > > alternatively, we could stick the wrapper map into a thread local, wdyt?
> > > 
> > > That would cut out the middle men here, ps#4 tries that.
> > > 
> > > I added clearing of the TLS entry upon script controller disposal, to make
> its
> > > non-null lifetime known & clear.
> > 
> > This will work but looks a bit weird to create a thread-local storage since
> > V8PerContextData is a "thread-local" storage for V8-related per-thread
> things...
> 
> That makes good sense, but I overlooked one DOMWrapperWorld "cow path"
> initially. ps#5 simplifies - looks ok?

PS5 looks awesome! LGTM.

[sof, 2017-01-29 17:34:59]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-29 17:35:09]

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

[sof, 2017-01-29 17:35:27]

Description was changed from

==========
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.

R=
BUG=683406
==========

to

==========
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.

R=jochen,haraken
BUG=686563
==========

[commit-bot: I haz the power, 2017-01-29 18:44:29]

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

[commit-bot: I haz the power, 2017-01-29 18:44:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sof, 2017-01-29 19:06:37]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-29 19:06:43]

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

[sof, 2017-01-29 20:10:37]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-29 20:10: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-01-29 20:58:44]

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

[NGG1, 2017-01-29 20:58:45]

I'll be able to check tomorrow if this solves really solves our problem. Thanks
for the work!

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

https://codereview.chromium.org/2663643002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:145:
scriptWrappable->markWrapper(visitor);
was this line unnecessary on worker threads?

[sof, 2017-01-29 21:11:13]

This needs more work, some boundary cases not being handled correctly.

I'll be travelling and mostly afk next week, so if anyone wants to take it over,
please do.

[commit-bot: I haz the power, 2017-01-29 22:37:55]

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

[commit-bot: I haz the power, 2017-01-29 22:37:56]

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-01-29 23:13:16]

On 2017/01/29 21:11:13, sof wrote:
> This needs more work, some boundary cases not being handled correctly.
> 
> I'll be travelling and mostly afk next week, so if anyone wants to take it
over,
> please do.

LGTM, will check details tomorrow. Thanks

[jochen (gone - plz use gerrit), 2017-01-30 00:52:27]

jochen@chromium.org changed reviewers:
+ mlippautz@chromium.org

[jochen (gone - plz use gerrit), 2017-01-30 00:52:28]

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

https://codereview.chromium.org/2663643002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:149: if
(dataStore.containsWrapper(scriptWrappable)) {
I guess this line is missing ^^^

[Michael Lippautz, 2017-01-30 01:54:12]

On 2017/01/30 00:52:28, jochen (travelling til Feb 4) wrote:
>
https://codereview.chromium.org/2663643002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):
> 
>
https://codereview.chromium.org/2663643002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:149: if
> (dataStore.containsWrapper(scriptWrappable)) {
> I guess this line is missing ^^^

I can take over on Monday in MTV, if that is of any help. Just let me know.

[sof, 2017-01-30 04:45:31]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 04:45:37]

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

[sof, 2017-01-30 04:50:44]

On 2017/01/30 01:54:12, Michael Lippautz (slow travel) wrote:
> On 2017/01/30 00:52:28, jochen (travelling til Feb 4) wrote:
> >
>
https://codereview.chromium.org/2663643002/diff/120001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2663643002/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:149: if
> > (dataStore.containsWrapper(scriptWrappable)) {
> > I guess this line is missing ^^^
> 
> I can take over on Monday in MTV, if that is of any help. Just let me know.

Thanks, if it hasn't landed, that'd be awesome :) i.e., giving latest ps to the
bots with jochen@'s additional check - that might be it.

And now better rush to the airport.

[sof, 2017-01-30 04:50:56]

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

https://codereview.chromium.org/2663643002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:149: if
(dataStore.containsWrapper(scriptWrappable)) {
On 2017/01/30 00:52:27, jochen (travelling til Feb 4) wrote:
> I guess this line is missing ^^^

ah, thanks much - that might be all.

[commit-bot: I haz the power, 2017-01-30 06:18:28]

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

[commit-bot: I haz the power, 2017-01-30 06:18:30]

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_...)

[NGG, 2017-01-30 12:37:21]

On 2017/01/30 04:50:56, sof wrote:
>
https://codereview.chromium.org/2663643002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):
> 
>
https://codereview.chromium.org/2663643002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:149: if
> (dataStore.containsWrapper(scriptWrappable)) {
> On 2017/01/30 00:52:27, jochen (travelling til Feb 4) wrote:
> > I guess this line is missing ^^^
> 
> ah, thanks much - that might be all.

Unfortunately this doesn't solve our problem, in our project the XHR still gets
GC'd before finishing. My previous minimized example runs correctly now, I'll
try to create a new small example that still fails.

[jochen (gone - plz use gerrit), 2017-01-30 15:34:01]

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

https://codereview.chromium.org/2663643002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:139: if (isolate
&& !isolate->GetCurrentContext().IsEmpty()) {
maybe remove the GetCurrentContext() call? That creates handles, but we're not
necessarily in a handlescope here (and gc creating handles isn't that great
either)

[NGG, 2017-01-30 17:33:49]

On 2017/01/30 12:37:21, NGG wrote:
> Unfortunately this doesn't solve our problem, in our project the XHR still
gets
> GC'd before finishing. My previous minimized example runs correctly now, I'll
> try to create a new small example that still fails.

self.gc() seems to be working, but manually triggering the GC via the DevConsole
can still reproduce the issue.
https://bugs.chromium.org/p/chromium/issues/detail?id=683406#c39

[Michael Lippautz, 2017-01-30 21:58:16]

On 2017/01/30 17:33:49, NGG wrote:
> On 2017/01/30 12:37:21, NGG wrote:
> > Unfortunately this doesn't solve our problem, in our project the XHR still
> gets
> > GC'd before finishing. My previous minimized example runs correctly now,
I'll
> > try to create a new small example that still fails.
> 
> self.gc() seems to be working, but manually triggering the GC via the
DevConsole
> can still reproduce the issue.
> https://bugs.chromium.org/p/chromium/issues/detail?id=683406#c39

FYI: I am looking right now. The current approach doesn't work as we cannot call
GetCurrentContext during a GC, as it would require a new handle scope. I am
implementing this now using TLS.

[sof, 2017-02-03 18:48:28]

On 2017/01/30 21:58:16, Michael Lippautz (slow travel) wrote:
> On 2017/01/30 17:33:49, NGG wrote:
> > On 2017/01/30 12:37:21, NGG wrote:
> > > Unfortunately this doesn't solve our problem, in our project the XHR still
> > gets
> > > GC'd before finishing. My previous minimized example runs correctly now,
> I'll
> > > try to create a new small example that still fails.
> > 
> > self.gc() seems to be working, but manually triggering the GC via the
> DevConsole
> > can still reproduce the issue.
> > https://bugs.chromium.org/p/chromium/issues/detail?id=683406#c39
> 
> FYI: I am looking right now. The current approach doesn't work as we cannot
call
> GetCurrentContext during a GC, as it would require a new handle scope. I am
> implementing this now using TLS.

https://codereview.chromium.org/2665733002 that ended up as, thanks much.
Closing this one.