Oilpan: Prepare moving InspectorController and InspectorAgents to oilpan.

Source/core/inspector/WorkerDebuggerAgent.cpp

 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 28 matching lines @@
 namespace WebCore {
 
 namespace {
 
 Mutex& workerDebuggerAgentsMutex()
 {
     AtomicallyInitializedStatic(Mutex&, mutex = *new Mutex);
     return mutex;
 }
 
 typedef HashMap<WorkerThread*, WorkerDebuggerAgent*> WorkerDebuggerAgents;

[wibling-chromium, 2014/06/02 08:15:16]

Don't you need to make this a WillBePersistentHeapHashMap etc. to ensure the
WorkerDebuggerAgent is being traced?

 
 WorkerDebuggerAgents& workerDebuggerAgents()
 {
     DEFINE_STATIC_LOCAL(WorkerDebuggerAgents, agents, ());
     return agents;
 }
 
+#if ENABLE(OILPAN)
+class WorkerDebuggerAgentRemover {
+public:
+    WorkerDebuggerAgentRemover(WorkerThread* thread)
+    {
+        m_thread = thread;
+    }
+    ~WorkerDebuggerAgentRemover()
+    {
+        MutexLocker lock(workerDebuggerAgentsMutex());
+        workerDebuggerAgents().remove(m_thread);

[haraken, 2014/05/30 03:24:12]

If you insert printf to ~WorkerDebuggerAgentRemover(), will it be printed? I
want to confirm whether ~WorkerDebuggerAgentRemover() is never called or is
called sometime later.

+    }
+
+private:
+    WorkerThread* m_thread;
+};
+
+typedef WillBePersistentHeapHashMap<WeakMember<WorkerDebuggerAgent>, OwnPtr<WorkerDebuggerAgentRemover> > WorkerDebuggerAgentRemovers;

[haraken, 2014/06/02 04:51:14]

mads@, erik@: I'm guessing that this issue might be related to weak processing
where multiple threads get involved. The main thread creates the
PersistentHeapHashMap, and worker threads register items to the map and request
Oilpan's weak processing to clear the items. Does this work in oilpan?

[Mads Ager (chromium), 2014/06/02 09:00:59]

That does not work, no. If you need to do something like this you need to use
off-heap collections of raw pointers and keep the code that explicitly removes
the elements from the global set. This is also done for the DatabaseManager that
has the same kind of setup.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

One issue with using a persistent heap hash map is that it is not clear which
thread will own the backing store and therefore the backing store can get
deallocated if the owning worker thread goes away. The other threads will then
be left with a stale backing pointer. Therefore, we cannot allow that.

  

+
+WorkerDebuggerAgentRemovers& workerDebuggerAgentRemovers()
+{
+    DEFINE_STATIC_LOCAL(WorkerDebuggerAgentRemovers, removers, ());

[haraken, 2014/05/30 03:24:12]

I don't think it's related, but what happens if you use:

DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<WillBeHeapHashMap<WeakMember<WorkerDebuggerAgent>,
OwnPtr<WorkerDebuggerAgentRemover>>>)

?

+    return removers;
+}
+#endif
 
 class RunInspectorCommandsTask FINAL : public ScriptDebugServer::Task {
 public:
     explicit RunInspectorCommandsTask(WorkerThread* thread)
         : m_thread(thread) { }
     virtual ~RunInspectorCommandsTask() { }
     virtual void run() OVERRIDE
     {
         // Process all queued debugger commands. WorkerThread is certainly
         // alive if this task is being executed.
         while (MessageQueueMessageReceived == m_thread->runLoop().runDebuggerTask(WorkerRunLoop::DontWaitForMessage)) { }
     }
 
 private:
     WorkerThread* m_thread;
 };
 
 } // namespace
 
-PassOwnPtr<WorkerDebuggerAgent> WorkerDebuggerAgent::create(WorkerScriptDebugServer* scriptDebugServer, WorkerGlobalScope* inspectedWorkerGlobalScope, InjectedScriptManager* injectedScriptManager)
+PassOwnPtrWillBeRawPtr<WorkerDebuggerAgent> WorkerDebuggerAgent::create(WorkerScriptDebugServer* scriptDebugServer, WorkerGlobalScope* inspectedWorkerGlobalScope, InjectedScriptManager* injectedScriptManager)
 {
-    return adoptPtr(new WorkerDebuggerAgent(scriptDebugServer, inspectedWorkerGlobalScope, injectedScriptManager));
+    return adoptPtrWillBeNoop(new WorkerDebuggerAgent(scriptDebugServer, inspectedWorkerGlobalScope, injectedScriptManager));
 }
 
 WorkerDebuggerAgent::WorkerDebuggerAgent(WorkerScriptDebugServer* scriptDebugServer, WorkerGlobalScope* inspectedWorkerGlobalScope, InjectedScriptManager* injectedScriptManager)
     : InspectorDebuggerAgent(injectedScriptManager)
     , m_scriptDebugServer(scriptDebugServer)
     , m_inspectedWorkerGlobalScope(inspectedWorkerGlobalScope)
 {
     MutexLocker lock(workerDebuggerAgentsMutex());
-    workerDebuggerAgents().set(inspectedWorkerGlobalScope->thread(), this);
+    WorkerThread* thread = inspectedWorkerGlobalScope->thread();
+    ASSERT(!workerDebuggerAgents().contains(thread));
+    ASSERT(!workerDebuggerAgentRemovers().contains(this));
+    workerDebuggerAgents().set(thread, this);
+    workerDebuggerAgentRemovers().set(this, adoptPtr(new WorkerDebuggerAgentRemover(thread)));

[haraken, 2014/05/30 01:13:20]

The line 113 and 115 need to be removed in oilpan builds, because you removed
the line 124 in oilpan builds, right? Hope it fixes your issue.

[keishi, 2014/05/30 02:01:07]

Won't line 79 in ~WorkerDebuggerAgentRemover work as a replacement for line 124?

[haraken, 2014/05/30 03:24:12]

oh, line 67 should work.

 }
 
 WorkerDebuggerAgent::~WorkerDebuggerAgent()
 {
+#if !ENABLE(OILPAN)
     MutexLocker lock(workerDebuggerAgentsMutex());
     ASSERT(workerDebuggerAgents().contains(m_inspectedWorkerGlobalScope->thread()));
     workerDebuggerAgents().remove(m_inspectedWorkerGlobalScope->thread());
+#endif
+}
+
+void WorkerDebuggerAgent::trace(Visitor* visitor)
+{
+    visitor->trace(m_inspectedWorkerGlobalScope);
+    InspectorDebuggerAgent::trace(visitor);
 }
 
 void WorkerDebuggerAgent::interruptAndDispatchInspectorCommands(WorkerThread* thread)
 {
     MutexLocker lock(workerDebuggerAgentsMutex());
     WorkerDebuggerAgent* agent = workerDebuggerAgents().get(thread);
     if (agent)
         agent->m_scriptDebugServer->interruptAndRunTask(adoptPtr(new RunInspectorCommandsTask(thread)));
 }
 
@@ skipping 25 matching lines @@
 {
     // We don't need to mute console for workers.
 }
 
 void WorkerDebuggerAgent::unmuteConsole()
 {
     // We don't need to mute console for workers.
 }
 
 } // namespace WebCore