Bindings: Manage multiple DOMWrapperWorlds for worklets

third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp

Index: third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp b/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp
index 4593c3ef2285369cbe2c479171ad9ca9c709cf0f..467134012e77ec7f2812618078bf2e8d7e4a2775 100644
--- a/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp
@@ -40,6 +40,7 @@
 #include "bindings/core/v8/WindowProxy.h"
 #include "bindings/core/v8/WrapperTypeInfo.h"
 #include "core/dom/ExecutionContext.h"
+#include "wtf/Atomics.h"
 #include "wtf/HashTraits.h"
 #include "wtf/PtrUtil.h"
 #include "wtf/StdLibExtras.h"
@@ -83,11 +84,25 @@ class DOMObjectHolder : public DOMObjectHolderBase {
 
 unsigned DOMWrapperWorld::s_numberOfNonMainWorldsInMainThread = 0;
 
+using WorldMap = HashMap<int, DOMWrapperWorld*>;
+
+static WorldMap& isolatedWorldMap() {
+  DCHECK(isMainThread());
+  DEFINE_STATIC_LOCAL(WorldMap, map, ());
+  return map;
+}
+
+static WorldMap& worldMap() {
+  DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<WorldMap>, map,
+                                  new ThreadSpecific<WorldMap>);
+  return *map;
+}
+
 PassRefPtr<DOMWrapperWorld> DOMWrapperWorld::create(v8::Isolate* isolate,
                                                     WorldType worldType) {
   DCHECK_NE(WorldType::Isolated, worldType);
-  return adoptRef(
-      new DOMWrapperWorld(isolate, worldType, getWorldIdForType(worldType)));
+  return adoptRef(new DOMWrapperWorld(isolate, worldType,
+                                      generateWorldIdForType(worldType)));
 }
 
 DOMWrapperWorld::DOMWrapperWorld(v8::Isolate* isolate,
@@ -97,8 +112,28 @@ DOMWrapperWorld::DOMWrapperWorld(v8::Isolate* isolate,
       m_worldId(worldId),
       m_domDataStore(
           WTF::wrapUnique(new DOMDataStore(isolate, isMainWorld()))) {
-  if (isWorkerWorld())
-    workerWorld() = this;
+  switch (worldType) {
+    case WorldType::Main:
+      // MainWorld is managed separately from worldMap() and isolatedWorldMap().
+      // See mainWorld().
+      break;
+    case WorldType::Isolated: {
+      DCHECK(isMainThread());
+      WorldMap& map = isolatedWorldMap();
+      DCHECK(!map.contains(worldId));
+      map.insert(worldId, this);
+      break;
+    }
+    case WorldType::GarbageCollector:
+    case WorldType::RegExp:
+    case WorldType::Testing:
+    case WorldType::Worker: {
+      WorldMap& map = worldMap();
+      DCHECK(!map.contains(worldId));
+      map.insert(worldId, this);
+      break;
+    }

[peria, 2017/03/14 07:10:49]

Add
default: NOTREACHED();
to catch unexpected behaviors just in case.

[nhiroki, 2017/03/14 09:14:38]

This unexpected behavior could happen only when static_cast<> is used, right?

I'd rather not add 'default' because it disables the compiler's branch coverage
check and would be more error-prone than the case.

[peria, 2017/03/14 09:52:46]

Thank you for the comment. Totally agreed.

+  }
   if (worldId != WorldId::MainWorldId && isMainThread())
     s_numberOfNonMainWorldsInMainThread++;
 }
@@ -111,12 +146,6 @@ DOMWrapperWorld& DOMWrapperWorld::mainWorld() {
   return *cachedMainWorld;
 }
 
-DOMWrapperWorld*& DOMWrapperWorld::workerWorld() {
-  DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<DOMWrapperWorld*>, workerWorld,
-                                  new ThreadSpecific<DOMWrapperWorld*>);
-  return *workerWorld;
-}
-
 PassRefPtr<DOMWrapperWorld> DOMWrapperWorld::fromWorldId(v8::Isolate* isolate,
                                                          int worldId) {
   if (worldId == MainWorldId)
@@ -124,47 +153,40 @@ PassRefPtr<DOMWrapperWorld> DOMWrapperWorld::fromWorldId(v8::Isolate* isolate,
   return ensureIsolatedWorld(isolate, worldId);
 }
 
-typedef HashMap<int, DOMWrapperWorld*> WorldMap;
-static WorldMap& isolatedWorldMap() {
-  ASSERT(isMainThread());
-  DEFINE_STATIC_LOCAL(WorldMap, map, ());
-  return map;
-}
-
 void DOMWrapperWorld::allWorldsInMainThread(
     Vector<RefPtr<DOMWrapperWorld>>& worlds) {
   ASSERT(isMainThread());
   worlds.push_back(&mainWorld());
-  WorldMap& isolatedWorlds = isolatedWorldMap();
-  for (WorldMap::iterator it = isolatedWorlds.begin();
-       it != isolatedWorlds.end(); ++it)
-    worlds.push_back(it->value);
+  for (DOMWrapperWorld* world : worldMap().values())
+    worlds.push_back(world);
+  for (DOMWrapperWorld* world : isolatedWorldMap().values())
+    worlds.push_back(world);
 }
 
 void DOMWrapperWorld::markWrappersInAllWorlds(
     ScriptWrappable* scriptWrappable,
     const ScriptWrappableVisitor* visitor) {

[nhiroki, 2017/03/14 06:25:07]

Question: AFAICS, this function doesn't mark wrappers in the main world. Is this
expected behavior?

[peria, 2017/03/14 07:10:49]

scriptWrappable->markWrapper(visitor) does.
IIUC, each ScriptWrappble object holds a (weak persistent) pointer to its
wrapper in the main world, and the main world exists only in the main thread.

[nhiroki, 2017/03/14 09:14:39]

Thank you for the clarification.

-  // Handle marking in per-worker wrapper worlds.
-  if (!isMainThread()) {
-    DCHECK(ThreadState::current()->isolate());
-    DOMWrapperWorld* worker = workerWorld();
-    if (worker) {
-      DOMDataStore& dataStore = worker->domDataStore();
-      if (dataStore.containsWrapper(scriptWrappable)) {
-        dataStore.markWrapper(scriptWrappable);
-      }
-    }
-    return;
+  if (isMainThread())
+    scriptWrappable->markWrapper(visitor);

[peria, 2017/03/14 07:10:49]

can we put this under the if branch on #181?
Then we don't need #169.

[nhiroki, 2017/03/14 09:14:38]

Moved it back (I wrongly thought this is necessary before marking wrappers on
the main thread, so move it to here :p)

+
+  // Marking for worlds other than the main world and the isolated worlds.
+  DCHECK(ThreadState::current()->isolate());
+  for (DOMWrapperWorld* world : worldMap().values()) {
+    DOMDataStore& dataStore = world->domDataStore();
+    if (dataStore.containsWrapper(scriptWrappable))
+      dataStore.markWrapper(scriptWrappable);
   }
 
-  scriptWrappable->markWrapper(visitor);
+  // The isolated worlds should exist only on the main thread.
+  if (!isMainThread())
+    return;
+
+  // Marking for the isolated worlds.
   WorldMap& isolatedWorlds = isolatedWorldMap();
   for (auto& world : isolatedWorlds.values()) {
     DOMDataStore& dataStore = world->domDataStore();
-    if (dataStore.containsWrapper(scriptWrappable)) {
-      // Marking for the isolated worlds
+    if (dataStore.containsWrapper(scriptWrappable))
       dataStore.markWrapper(scriptWrappable);
-    }
   }
 }
 
@@ -193,8 +215,7 @@ DOMWrapperWorld::~DOMWrapperWorld() {
 void DOMWrapperWorld::dispose() {
   m_domObjectHolders.clear();
   m_domDataStore.reset();
-  if (isWorkerWorld())
-    workerWorld() = nullptr;
+  worldMap().remove(m_worldId);
 }
 
 #if DCHECK_IS_ON()
@@ -210,16 +231,14 @@ PassRefPtr<DOMWrapperWorld> DOMWrapperWorld::ensureIsolatedWorld(
   ASSERT(isIsolatedWorldId(worldId));
 
   WorldMap& map = isolatedWorldMap();
-  WorldMap::AddResult result = map.insert(worldId, nullptr);
-  RefPtr<DOMWrapperWorld> world = result.storedValue->value;
-  if (world) {
-    ASSERT(world->worldId() == worldId);
+  auto it = map.find(worldId);
+  if (it != map.end()) {
+    RefPtr<DOMWrapperWorld> world = it->value;
+    DCHECK_EQ(worldId, world->worldId());
     return world.release();
   }
 
-  world = adoptRef(new DOMWrapperWorld(isolate, WorldType::Isolated, worldId));
-  result.storedValue->value = world.get();
-  return world.release();
+  return adoptRef(new DOMWrapperWorld(isolate, WorldType::Isolated, worldId));
 }
 
 typedef HashMap<int, RefPtr<SecurityOrigin>> IsolatedWorldSecurityOriginMap;
@@ -323,7 +342,12 @@ void DOMWrapperWorld::weakCallbackForDOMObjectHolder(
   holderBase->world()->unregisterDOMObjectHolder(holderBase);
 }
 
-int DOMWrapperWorld::getWorldIdForType(WorldType worldType) {
+// The last used world ID. Must be used with atomic operations because this is

[peria, 2017/03/14 07:10:49]

code health: remove the first sentence. It explains no additional information.

[nhiroki, 2017/03/14 09:14:39]

Done.

+// accessed from multiple threads.
+static int s_lastUsedWorldId =

[peria, 2017/03/14 07:10:49]

First ID to be used is UnspecifiedWorldIdStart, and it is not "last used world
ID" before the first  assignment.
Please rename this static variable.

[Yuki, 2017/03/14 08:06:00]

nit: Better to put this variable inside generateWorldIdForType so that no one
can access to this by accident.  Should we use DEFINE_THREAD_SAFE_STATIC_LOCAL
then?

[nhiroki, 2017/03/14 09:14:38]

Replaced UnspecifiedWorldIdStart with IsolatedWorldIdLimit instead of renaming
the variable because I didn't come up with a better name...

[nhiroki, 2017/03/14 09:14:39]

Done.

+    DOMWrapperWorld::WorldId::UnspecifiedWorldIdStart;
+
+int DOMWrapperWorld::generateWorldIdForType(WorldType worldType) {
   switch (worldType) {
     case WorldType::Main:
       return MainWorldId;
@@ -333,17 +357,10 @@ int DOMWrapperWorld::getWorldIdForType(WorldType worldType) {
       NOTREACHED();
       return InvalidWorldId;
     case WorldType::GarbageCollector:
-      return GarbageCollectorWorldId;
     case WorldType::RegExp:
-      return RegExpWorldId;
     case WorldType::Testing:
-      return TestingWorldId;
-    // Currently, WorldId for a worker/worklet is a fixed value, but this
-    // doesn't work when multiple worklets are created on a thread.
-    // TODO(nhiroki): Expand the identifier space for workers/worklets.
-    // (https://crbug.com/697622)
     case WorldType::Worker:
-      return WorkerWorldId;
+      return WTF::atomicIncrement(&s_lastUsedWorldId);

[Yuki, 2017/03/14 08:06:00]

nit: I understand it's super unlikely, but should we check an overflow?  I'm
okay to crash if we hit an overflow.

[nhiroki, 2017/03/14 09:14:39]

Done.

   }
   NOTREACHED();
   return InvalidWorldId;