Bindings: Manage multiple DOMWrapperWorlds for worklets (2)

Bindings: Manage multiple DOMWrapperWorlds for worklets (2)

This is a follow-up CL for https://crrev.com/2735823006/

This CL...
- merges isolatedWorldMap() into worldMap() for simplification,
- fixes ArrayBuffer accumulation to correctly handle all worlds on non-main
  thread, and
- adds unit tests for DOMWrapperWorld.

BUG=697622, 697629
Review-Url: https://codereview.chromium.org/2747163002
Cr-Commit-Position: refs/heads/master@{#457357}
Committed: https://chromium.googlesource.com/chromium/src/+/178fc4a5d430afe4ae1cd9fabbf1a631dd09e965

[nhiroki, 2017-03-15 06:42:00]

Patchset #2 (id:20001) has been deleted

[nhiroki, 2017-03-15 06:42:29]

Description was changed from

==========
Bindings: Simplify DOMWrapperWorld management

BUG=697622, 697629
==========

to

==========
Bindings: Manage multiple DOMWrapperWorlds for worklets (2)

BUG=697622, 697629
==========

[nhiroki, 2017-03-15 06:43:19]

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

[commit-bot: I haz the power, 2017-03-15 06:43:31]

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

[nhiroki, 2017-03-15 06:52:02]

Description was changed from

==========
Bindings: Manage multiple DOMWrapperWorlds for worklets (2)

BUG=697622, 697629
==========

to

==========
Bindings: Manage multiple DOMWrapperWorlds for worklets (2)

This is a follow-up CL for https://crrev.com/2735823006/

This CL...
- merges isolatedWorldMap() into worldMap() for simplification,
- fixes ArrayBuffer accumulation to correctly handle all worlds on non-main
  thread, and
- adds unit tests for DOMWrapperWorld.

BUG=697622, 697629
==========

[nhiroki, 2017-03-15 06:52:25]

Patchset #1 (id:1) has been deleted

[nhiroki, 2017-03-15 07:04:10]

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

[nhiroki, 2017-03-15 07:04:21]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-03-15 07:04:35]

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

[nhiroki, 2017-03-15 07:05:58]

nhiroki@chromium.org changed reviewers:
+ haraken@chromium.org, jbroman@chromium.org, peria@chromium.org,
yukishiino@chromium.org

[nhiroki, 2017-03-15 07:05:58]

Hi, can you review this?

+haraken@,yukishiino@,peria@ for the entire CL
+jbroman@ for SerializedScriptValue.cpp

Thanks.

[peria, 2017-03-15 07:26:22]

https://codereview.chromium.org/2747163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (left):

https://codereview.chromium.org/2747163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:203: WorldMap&
map = isolatedWorldMap();
This change makes nonMainWorldsInCurrentThread() and allWorldsInCurrentThread()
wrong.
Plus, IIUC, this part confirms this object did not migrate between threads.

[nhiroki, 2017-03-15 07:27:51]

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

[commit-bot: I haz the power, 2017-03-15 07:28:10]

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

[nhiroki, 2017-03-15 07:38:08]

https://codereview.chromium.org/2747163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (left):

https://codereview.chromium.org/2747163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:203: WorldMap&
map = isolatedWorldMap();
On 2017/03/15 07:26:21, peria wrote:
> This change makes nonMainWorldsInCurrentThread() and
allWorldsInCurrentThread()
> wrong.

Can you provide an example of the case?

> Plus, IIUC, this part confirms this object did not migrate between threads.

I'm confused... can DOMWrapperWorld be migrated between threads?

[peria, 2017-03-15 07:52:11]

https://codereview.chromium.org/2747163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (left):

https://codereview.chromium.org/2747163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:203: WorldMap&
map = isolatedWorldMap();
On 2017/03/15 07:38:08, nhiroki (slow) wrote:
> On 2017/03/15 07:26:21, peria wrote:
> > This change makes nonMainWorldsInCurrentThread() and
> allWorldsInCurrentThread()
> > wrong.
> 
> Can you provide an example of the case?

1. nonMainWorldsInCurrentThread() -> true
2. Create a worklet
3. nonMainWorldsInCurrentThread() -> false (correct)
4. Delete the worklet
5. nonMainWorldsInCurrentThread() -> false (wrong)

> 
> > Plus, IIUC, this part confirms this object did not migrate between threads.
> 
> I'm confused... can DOMWrapperWorld be migrated between threads?

It's technically possible, I think, although it must not happen.
I think it is a reason why we had line #205-209 here.

https://codereview.chromium.org/2747163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/2747163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:87: static bool
nonMainWorldsInCurrentThread();
Optional: Can we rename this method? Current name looks like a getter.

[nhiroki, 2017-03-15 08:20:45]

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

[commit-bot: I haz the power, 2017-03-15 08:21:03]

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

[nhiroki, 2017-03-15 08:21:26]

Thank you. Updated.

https://codereview.chromium.org/2747163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (left):

https://codereview.chromium.org/2747163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:203: WorldMap&
map = isolatedWorldMap();
On 2017/03/15 07:52:10, peria wrote:
> On 2017/03/15 07:38:08, nhiroki (slow) wrote:
> > On 2017/03/15 07:26:21, peria wrote:
> > > This change makes nonMainWorldsInCurrentThread() and
> > allWorldsInCurrentThread()
> > > wrong.
> > 
> > Can you provide an example of the case?
> 
> 1. nonMainWorldsInCurrentThread() -> true
> 2. Create a worklet
> 3. nonMainWorldsInCurrentThread() -> false (correct)
> 4. Delete the worklet
> 5. nonMainWorldsInCurrentThread() -> false (wrong)

The step 4 runs DOMWrapperWorld::dispose() that remove an entry from worldMap(),
so the nonMainWorldsInCurrentThread() should return true at the step 5 (I might
be missing something...)

> > > Plus, IIUC, this part confirms this object did not migrate between
threads.
> > 
> > I'm confused... can DOMWrapperWorld be migrated between threads?
> 
> It's technically possible, I think, although it must not happen.
> I think it is a reason why we had line #205-209 here.

Ah, you mean there can be a threading bug so we should have assertions like the
existing code, right? Added such an assertion (If necessary, we could have a
thread checker at the ctor and dtor).

https://codereview.chromium.org/2747163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/2747163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:87: static bool
nonMainWorldsInCurrentThread();
On 2017/03/15 07:52:10, peria wrote:
> Optional: Can we rename this method? Current name looks like a getter.

Renamed to nonMainWorldsExistInCurrentThread(). Does this make sense?

[peria, 2017-03-15 08:37:28]

https://codereview.chromium.org/2747163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/2747163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:87: static bool
nonMainWorldsInCurrentThread();
On 2017/03/15 08:21:26, nhiroki (slow) wrote:
> On 2017/03/15 07:52:10, peria wrote:
> > Optional: Can we rename this method? Current name looks like a getter.
> 
> Renamed to nonMainWorldsExistInCurrentThread(). Does this make sense?

:)

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

https://codereview.chromium.org/2747163002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:190:
worldMap().remove(m_worldId);
Can't we move this into the destructor?
IMHO, dispose() should have less works, and I feel it strange if something
touches this map after dispose() is called.

[Yuki, 2017-03-15 08:48:47]

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

https://codereview.chromium.org/2747163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:110: switch
(worldType) {
super nit: s/worldType/m_worldType/

https://codereview.chromium.org/2747163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:120:
DCHECK(!map.contains(worldId));
super nit: s/worldId/m_worldId/

https://codereview.chromium.org/2747163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (left):

https://codereview.chromium.org/2747163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:88: return
s_numberOfNonMainWorldsInMainThread;
This function is used just for a performance hack.  So the speed performance is
super important.

Non-inline function wouldn't be acceptable.

Accessing to TLS wouldn't be acceptable.

Probably, we can simply keep this hack as is, though I understand your intent. 
If you're okay, I'd like to rename this to have "Exist(s)", though.
e.g. nonMainWorldExistsInMainThread

https://codereview.chromium.org/2747163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/2747163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:87: static bool
nonMainWorldsInCurrentThread();
nonMainWorlds seems returning a kind of vector<World>.

What about nonMainWorldExistsInCurrentThread()
or nonMainWorldsExistInCurrentThread()?

I think it's intuitive that allWorldsInCurrentThread below returns a
vector<World>.

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

https://codereview.chromium.org/2747163002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:190:
worldMap().remove(m_worldId);
On 2017/03/15 08:37:28, peria wrote:
> Can't we move this into the destructor?
> IMHO, dispose() should have less works, and I feel it strange if something
> touches this map after dispose() is called.

It may change the existing behavior, and it's hard to predict what'd happen.  If
we want, we'd better do it in a separate CL.

[haraken, 2017-03-15 08:56:09]

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

https://codereview.chromium.org/2747163002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:135:
PassRefPtr<DOMWrapperWorld> DOMWrapperWorld::fromWorldId(v8::Isolate* isolate,

This method looks a bit weird. fromWorldId initializes the main world and an
isolated world but don't initialize a worker world.

https://codereview.chromium.org/2747163002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:149: bool
DOMWrapperWorld::nonMainWorldsExistInCurrentThread() {

This method must be extremely fast. Are you sure that worldMap().isEmpty() is as
fast as looking up a global variable?

https://codereview.chromium.org/2747163002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:184: dispose();

Why do we need to call dispose() for an isWorkerWorld() case?

https://codereview.chromium.org/2747163002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:190:
worldMap().remove(m_worldId);
On 2017/03/15 08:37:28, peria wrote:
> Can't we move this into the destructor?
> IMHO, dispose() should have less works, and I feel it strange if something
> touches this map after dispose() is called.

I'd prefer keeping it here since the world stopped working once we reset
m_domDataStore. It's dangerous to keep a broken world in the world map.

[nhiroki, 2017-03-15 10:14:12]

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

[commit-bot: I haz the power, 2017-03-15 10:14:24]

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

[nhiroki, 2017-03-15 10:22:18]

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

[commit-bot: I haz the power, 2017-03-15 10:22:33]

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

[nhiroki, 2017-03-15 10:25:39]

Patchset #6 (id:160001) has been deleted

[commit-bot: I haz the power, 2017-03-15 10:26:26]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[nhiroki, 2017-03-15 10:27:30]

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

[commit-bot: I haz the power, 2017-03-15 10:27:43]

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

[nhiroki, 2017-03-15 11:01:23]

Thank you for reviewing this. Updated.

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

https://codereview.chromium.org/2747163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:110: switch
(worldType) {
On 2017/03/15 08:48:47, Yuki wrote:
> super nit: s/worldType/m_worldType/

Done.

https://codereview.chromium.org/2747163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:120:
DCHECK(!map.contains(worldId));
On 2017/03/15 08:48:47, Yuki wrote:
> super nit: s/worldId/m_worldId/

Done.

https://codereview.chromium.org/2747163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (left):

https://codereview.chromium.org/2747163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:88: return
s_numberOfNonMainWorldsInMainThread;
On 2017/03/15 08:48:47, Yuki wrote:
> This function is used just for a performance hack.  So the speed performance
is
> super important.
> 
> Non-inline function wouldn't be acceptable.
> 
> Accessing to TLS wouldn't be acceptable.
> 
> Probably, we can simply keep this hack as is, though I understand your intent.

> If you're okay, I'd like to rename this to have "Exist(s)", though.
> e.g. nonMainWorldExistsInMainThread

OK, then I reverted this change and added a comment about performance.

https://codereview.chromium.org/2747163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/2747163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:87: static bool
nonMainWorldsInCurrentThread();
On 2017/03/15 08:48:47, Yuki wrote:
> nonMainWorlds seems returning a kind of vector<World>.
> 
> What about nonMainWorldExistsInCurrentThread()
> or nonMainWorldsExistInCurrentThread()?
> 
> I think it's intuitive that allWorldsInCurrentThread below returns a
> vector<World>.

Done.

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

https://codereview.chromium.org/2747163002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:135:
PassRefPtr<DOMWrapperWorld> DOMWrapperWorld::fromWorldId(v8::Isolate* isolate,
On 2017/03/15 08:56:09, haraken wrote:
> 
> This method looks a bit weird. fromWorldId initializes the main world and an
> isolated world but don't initialize a worker world.

Yeah, this is strange.

I think initializing a worker world here would be wrong. Conceptually, worldId
is generated in the ctor of DOMWrapperWorld and clients of DOMWrapperWorld
should not use arbitrary worldId. Probably we should stop creating IsolatedWorld
with a given worldId from out of DOMWrapperWorld and return nullptr if the world
does not exist.

I'm not familiar with callers of fromWorldId() yet (some callers could depend on
this behavior...?) so I'd prefer to work on it in a separate CL.

https://codereview.chromium.org/2747163002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:149: bool
DOMWrapperWorld::nonMainWorldsExistInCurrentThread() {
On 2017/03/15 08:56:09, haraken wrote:
> 
> This method must be extremely fast. Are you sure that worldMap().isEmpty() is
as
> fast as looking up a global variable?
> 

TLS access would be expensive on some platforms, so it'd be better to avoid
it... I reverted this change.

https://codereview.chromium.org/2747163002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:184: dispose();
On 2017/03/15 08:56:09, haraken wrote:
> 
> Why do we need to call dispose() for an isWorkerWorld() case?

Yeah, it's not necessary. Moved.

https://codereview.chromium.org/2747163002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:190:
worldMap().remove(m_worldId);
On 2017/03/15 08:56:09, haraken wrote:
> On 2017/03/15 08:37:28, peria wrote:
> > Can't we move this into the destructor?
> > IMHO, dispose() should have less works, and I feel it strange if something
> > touches this map after dispose() is called.
> 
> I'd prefer keeping it here since the world stopped working once we reset
> m_domDataStore. It's dangerous to keep a broken world in the world map.

Probably we can inline the entire dispose() into the dtor.

DOMWrapperWorld::dispose() is called only from WorkerOrWorkletScriptController
that is an owner of DOMWrapperWorld. This controller will be destroyed
immediately after calling DOMWrapperWorld::dispose() and destroys the world in
its dtor.

I'll consider it in a separate CL.

[Yuki, 2017-03-15 11:54:07]

LGTM.

https://codereview.chromium.org/2747163002/diff/140002/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorldTest.cpp (right):

https://codereview.chromium.org/2747163002/diff/140002/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorldTest.cpp:95:
TEST(DOMWrapperWorldTest, basic) {
nit: s/basic/Basic/ maybe?

Most tests seem starting with a capital case, but some start with a lower case.

[commit-bot: I haz the power, 2017-03-15 12:31:57]

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

[commit-bot: I haz the power, 2017-03-15 12:31:58]

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

[haraken, 2017-03-15 13:18:10]

LGTM

[jbroman, 2017-03-15 14:09:34]

SerializedScriptValue lgtm

https://codereview.chromium.org/2747163002/diff/140002/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/2747163002/diff/140002/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:180: for
(size_t i = 0; i < worlds.size(); i++) {
super-nit: you could use a range-based loop here:

for (const auto& world : worlds) {
  v8::Local<v8::Object> wrapper = world->domDataStore().get(object, isolate);
  // ...
}

[nhiroki, 2017-03-15 14:45:05]

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

[commit-bot: I haz the power, 2017-03-15 14:45:29]

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

[nhiroki, 2017-03-15 14:47:42]

Thank you!

https://codereview.chromium.org/2747163002/diff/140002/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorldTest.cpp (right):

https://codereview.chromium.org/2747163002/diff/140002/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorldTest.cpp:95:
TEST(DOMWrapperWorldTest, basic) {
On 2017/03/15 11:54:07, Yuki wrote:
> nit: s/basic/Basic/ maybe?
> 
> Most tests seem starting with a capital case, but some start with a lower
case.

Done.

https://codereview.chromium.org/2747163002/diff/140002/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/2747163002/diff/140002/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:180: for
(size_t i = 0; i < worlds.size(); i++) {
On 2017/03/15 14:09:34, jbroman wrote:
> super-nit: you could use a range-based loop here:
> 
> for (const auto& world : worlds) {
>   v8::Local<v8::Object> wrapper = world->domDataStore().get(object, isolate);
>   // ...
> }

Done.

[commit-bot: I haz the power, 2017-03-15 16:24:41]

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

[commit-bot: I haz the power, 2017-03-15 16:24:43]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2017-03-16 05:03:10]

peria@, friendly ping?

[peria, 2017-03-16 05:12:00]

LGTM

I'm sorry to be late.

[nhiroki, 2017-03-16 05:15:08]

On 2017/03/16 05:12:00, peria wrote:
> LGTM
> 
> I'm sorry to be late.

No problem! Thank you :)

[nhiroki, 2017-03-16 05:15:17]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2017-03-16 05:15:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jbroman@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2747163002/#ps180001
(title: "address review comments & may fix use-after-free bug in the test")

[commit-bot: I haz the power, 2017-03-16 05:15:37]

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-03-16 05:22:28]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1489641317120520,
"parent_rev": "be8002aa2f99d13a07546f12018d4add0b7e1be8", "commit_rev":
"178fc4a5d430afe4ae1cd9fabbf1a631dd09e965"}

[commit-bot: I haz the power, 2017-03-16 05:23:54]

Description was changed from

==========
Bindings: Manage multiple DOMWrapperWorlds for worklets (2)

This is a follow-up CL for https://crrev.com/2735823006/

This CL...
- merges isolatedWorldMap() into worldMap() for simplification,
- fixes ArrayBuffer accumulation to correctly handle all worlds on non-main
  thread, and
- adds unit tests for DOMWrapperWorld.

BUG=697622, 697629
==========

to

==========
Bindings: Manage multiple DOMWrapperWorlds for worklets (2)

This is a follow-up CL for https://crrev.com/2735823006/

This CL...
- merges isolatedWorldMap() into worldMap() for simplification,
- fixes ArrayBuffer accumulation to correctly handle all worlds on non-main
  thread, and
- adds unit tests for DOMWrapperWorld.

BUG=697622, 697629

Review-Url: https://codereview.chromium.org/2747163002
Cr-Commit-Position: refs/heads/master@{#457357}
Committed:
https://chromium.googlesource.com/chromium/src/+/178fc4a5d430afe4ae1cd9fabbf1...
==========

[commit-bot: I haz the power, 2017-03-16 05:23:55]

Committed patchset #7 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/178fc4a5d430afe4ae1cd9fabbf1...