Oilpan: Remove raw pointer to ExecutionContext from WorkerInspectorProxy

Source/core/inspector/InspectorWorkerAgent.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 33 matching lines @@
 #include "wtf/text/WTFString.h"
 
 namespace blink {
 
 namespace WorkerAgentState {
 static const char workerInspectionEnabled[] = "workerInspectionEnabled";
 static const char autoconnectToWorkers[] = "autoconnectToWorkers";
 };
 
 class InspectorWorkerAgent::WorkerAgentClient final : public WorkerInspectorProxy::PageInspector {
-    WTF_MAKE_FAST_ALLOCATED(InspectorWorkerAgent::WorkerAgentClient);
+    WTF_MAKE_FAST_ALLOCATED_WILL_BE_REMOVED(InspectorWorkerAgent::WorkerAgentClient);
 public:
+    static PassOwnPtrWillBeRawPtr<WorkerAgentClient> create(InspectorFrontend::Worker* frontend, WorkerInspectorProxy* proxy, const String& id, PageConsoleAgent* consoleAgent)
+    {
+        return adoptPtrWillBeNoop(new WorkerAgentClient(frontend, proxy, id, consoleAgent));
+    }
+
     WorkerAgentClient(InspectorFrontend::Worker* frontend, WorkerInspectorProxy* proxy, const String& id, PageConsoleAgent* consoleAgent)
         : m_frontend(frontend)
         , m_proxy(proxy)
         , m_id(id)
         , m_connected(false)
         , m_consoleAgent(consoleAgent)
     {
         ASSERT(!proxy->pageInspector());
     }
     ~WorkerAgentClient() override
     {
-        disconnectFromWorker();
+    }
+    DEFINE_INLINE_VIRTUAL_TRACE()
+    {
+        visitor->trace(m_proxy);
+        visitor->trace(m_consoleAgent);
+        WorkerInspectorProxy::PageInspector::trace(visitor);
     }
 
     String id() const { return m_id; }
     WorkerInspectorProxy* proxy() const { return m_proxy; }
 
     void connectToWorker()
     {
         if (m_connected)
             return;
         m_connected = true;
         m_proxy->connectToInspector(this);
     }
 
     void disconnectFromWorker()

[haraken, 2015/07/30 07:47:31]

Shall we:

- Rename disconnectFromWorker to dispose.
- Clear m_proxy and m_consoleAgent in dispose.
- Add ASSERT(!m_proxy) in the destructor.

?

[keishi, 2015/08/03 09:35:56]

Done.

     {
         if (!m_connected)
             return;
         m_connected = false;
         m_proxy->disconnectFromInspector();
     }
 
 private:
     // WorkerInspectorProxy::PageInspector implementation
     void dispatchMessageFromWorker(const String& message) override
     {
         m_frontend->dispatchMessageFromWorker(m_id, message);
     }
     // WorkerInspectorProxy::PageInspector implementation
     void workerConsoleAgentEnabled(WorkerGlobalScopeProxy* proxy) override
     {
         m_consoleAgent->workerConsoleAgentEnabled(proxy);
     }
 
     InspectorFrontend::Worker* m_frontend;

[haraken, 2015/07/30 07:47:31]

Are you sure that this pointer doesn't become stale?

[keishi, 2015/08/03 09:35:56]

When FrontEnd is destroyed InspectorBaseAgent::clearFrontend() is called. It
calls InspectorWorkerAgent::disable which calls
InspectorWorkerAgent::destroyWorkerAgentClients() which calls
InspectorWorkerAgent::WorkerAgentClient::disconnectFromWorker().
I've added m_frontend clearing in
InspectorWorkerAgent::WorkerAgentClient::dispose (former disconnectFromWorker)
so it won't be stale.

-    WorkerInspectorProxy* m_proxy;
+    RawPtrWillBeMember<WorkerInspectorProxy> m_proxy;
     String m_id;
     bool m_connected;
-    PageConsoleAgent* m_consoleAgent;
+    RawPtrWillBeMember<PageConsoleAgent> m_consoleAgent;
 };
 
 PassOwnPtrWillBeRawPtr<InspectorWorkerAgent> InspectorWorkerAgent::create(PageConsoleAgent* consoleAgent)
 {
     return adoptPtrWillBeNoop(new InspectorWorkerAgent(consoleAgent));
 }
 
 InspectorWorkerAgent::InspectorWorkerAgent(PageConsoleAgent* consoleAgent)
     : InspectorBaseAgent<InspectorWorkerAgent, InspectorFrontend::Worker>("Worker")
     , m_consoleAgent(consoleAgent)
@@ skipping 86 matching lines @@
     if (!m_tracingSessionId.isEmpty())
         workerInspectorProxy->writeTimelineStartedEvent(m_tracingSessionId, id);
 }
 
 void InspectorWorkerAgent::workerTerminated(WorkerInspectorProxy* proxy)
 {
     m_workerInfos.remove(proxy);
     for (WorkerClients::iterator it = m_idToClient.begin(); it != m_idToClient.end(); ++it) {
         if (proxy == it->value->proxy()) {
             frontend()->workerTerminated(it->key);
-            delete it->value;
+            it->value->disconnectFromWorker();
             m_idToClient.remove(it);
             return;
         }
     }
 }
 
 void InspectorWorkerAgent::createWorkerAgentClientsForExistingWorkers()
 {
     for (auto& info : m_workerInfos)
         createWorkerAgentClient(info.key, info.value.url, info.value.id);
 }
 
 void InspectorWorkerAgent::destroyWorkerAgentClients()
 {
     for (auto& client : m_idToClient) {

[yurys, 2015/07/30 09:01:39]

style nit: drop {}

[keishi, 2015/08/03 09:35:56]

Done.

         client.value->disconnectFromWorker();
-        delete client.value;
     }
     m_idToClient.clear();
 }
 
 void InspectorWorkerAgent::createWorkerAgentClient(WorkerInspectorProxy* workerInspectorProxy, const String& url, const String& id)
 {
-    WorkerAgentClient* client = new WorkerAgentClient(frontend(), workerInspectorProxy, id, m_consoleAgent);
-    m_idToClient.set(id, client);
+    WorkerAgentClient* client = m_idToClient.set(id, WorkerAgentClient::create(frontend(), workerInspectorProxy, id, m_consoleAgent)).storedValue->value.get();

[yurys, 2015/07/30 09:01:39]

Please extract client creation on a separate line for better readability.

[keishi, 2015/08/03 09:35:56]

Done.

 
     ASSERT(frontend());
     bool autoconnectToWorkers = m_state->getBoolean(WorkerAgentState::autoconnectToWorkers);
     if (autoconnectToWorkers)
         client->connectToWorker();
     frontend()->workerCreated(id, url, autoconnectToWorkers);
 }
 
+DEFINE_TRACE(InspectorWorkerAgent)
+{
+#if ENABLE(OILPAN)
+    visitor->trace(m_idToClient);
+#endif
+    InspectorBaseAgent<InspectorWorkerAgent, InspectorFrontend::Worker>::trace(visitor);
+}
+
 } // namespace blink