Oilpan: Remove raw pointer to ExecutionContext from WorkerInspectorProxy

Oilpan: Remove raw pointer to ExecutionContext from WorkerInspectorProxy

Prepares moving WorkerInspectorProxy and WorkerInspectorProxy::PageInspector to the heap.

BUG=509911
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201548

[keishi, 2015-07-29 21:16:23]

keishi@chromium.org changed reviewers:
+ haraken@chromium.org, oilpan-reviews@chromium.org, yutak@chromium.org

[keishi, 2015-07-29 21:16:23]

PTAL

[haraken, 2015-07-30 07:47:32]

https://codereview.chromium.org/1253293002/diff/20001/Source/core/inspector/I...
File Source/core/inspector/InspectorWorkerAgent.cpp (right):

https://codereview.chromium.org/1253293002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InspectorWorkerAgent.cpp:91: void disconnectFromWorker()

Shall we:

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

?

https://codereview.chromium.org/1253293002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InspectorWorkerAgent.cpp:111: InspectorFrontend::Worker*
m_frontend;

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

https://codereview.chromium.org/1253293002/diff/20001/Source/core/inspector/I...
File Source/core/inspector/InspectorWorkerAgent.h (right):

https://codereview.chromium.org/1253293002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InspectorWorkerAgent.h:90: PageConsoleAgent*
m_consoleAgent;

This should be a Member. (A raw pointer in an on-heap object has a risk of
becoming a stale pointer.)

https://codereview.chromium.org/1253293002/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerInspectorProxy.h (right):

https://codereview.chromium.org/1253293002/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerInspectorProxy.h:56: WorkerThread* m_workerThread;

This raw pointer would be safe since the m_workerThread is explicitly cleared in
workerThreadTerminated().

https://codereview.chromium.org/1253293002/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerInspectorProxy.h:59: WorkerGlobalScopeProxy*
m_workerGlobalScopeProxy;

I guess this pointer has a risk of becoming a stale pointer if:

1) WorkerGlobalScopeProxy gets destroyed.
2) GC is triggered and the WorkerInspectorProxy gets collected.

Between 1) and 2), WorkerInspectorProxy::m_workerGlobalScopeProxy can be stale.
I think you need to call:

   m_workerInspectorProxy->setWorkerGlobalScopeProxy(nullptr);

in WorkerMessagingProxy's destructor.

[yurys, 2015-07-30 09:01:38]

yurys@chromium.org changed reviewers:
+ dgozman@chromium.org, kozyatinskiy@chromium.org, yurys@chromium.org

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

https://codereview.chromium.org/1253293002/diff/20001/Source/core/inspector/I...
File Source/core/inspector/InspectorWorkerAgent.cpp (right):

https://codereview.chromium.org/1253293002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InspectorWorkerAgent.cpp:237: for (auto& client :
m_idToClient) {
style nit: drop {}

https://codereview.chromium.org/1253293002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InspectorWorkerAgent.cpp:245: WorkerAgentClient* client =
m_idToClient.set(id, WorkerAgentClient::create(frontend(), workerInspectorProxy,
id, m_consoleAgent)).storedValue->value.get();
Please extract client creation on a separate line for better readability.

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

https://codereview.chromium.org/1253293002/diff/20001/Source/core/inspector/I...
File Source/core/inspector/InspectorWorkerAgent.cpp (right):

https://codereview.chromium.org/1253293002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InspectorWorkerAgent.cpp:91: void disconnectFromWorker()
On 2015/07/30 07:47:31, haraken wrote:
> 
> Shall we:
> 
> - Rename disconnectFromWorker to dispose.
> - Clear m_proxy and m_consoleAgent in dispose.
> - Add ASSERT(!m_proxy) in the destructor.
> 
> ?

Done.

https://codereview.chromium.org/1253293002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InspectorWorkerAgent.cpp:111: InspectorFrontend::Worker*
m_frontend;
On 2015/07/30 07:47:31, haraken wrote:
> 
> Are you sure that this pointer doesn't become stale?

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.

https://codereview.chromium.org/1253293002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InspectorWorkerAgent.cpp:237: for (auto& client :
m_idToClient) {
On 2015/07/30 09:01:39, yurys wrote:
> style nit: drop {}

Done.

https://codereview.chromium.org/1253293002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InspectorWorkerAgent.cpp:245: WorkerAgentClient* client =
m_idToClient.set(id, WorkerAgentClient::create(frontend(), workerInspectorProxy,
id, m_consoleAgent)).storedValue->value.get();
On 2015/07/30 09:01:39, yurys wrote:
> Please extract client creation on a separate line for better readability.

Done.

https://codereview.chromium.org/1253293002/diff/20001/Source/core/inspector/I...
File Source/core/inspector/InspectorWorkerAgent.h (right):

https://codereview.chromium.org/1253293002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InspectorWorkerAgent.h:90: PageConsoleAgent*
m_consoleAgent;
On 2015/07/30 07:47:32, haraken wrote:
> 
> This should be a Member. (A raw pointer in an on-heap object has a risk of
> becoming a stale pointer.)
> 

Done.

https://codereview.chromium.org/1253293002/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerInspectorProxy.h (right):

https://codereview.chromium.org/1253293002/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerInspectorProxy.h:59: WorkerGlobalScopeProxy*
m_workerGlobalScopeProxy;
On 2015/07/30 07:47:32, haraken wrote:
> 
> I guess this pointer has a risk of becoming a stale pointer if:
> 
> 1) WorkerGlobalScopeProxy gets destroyed.
> 2) GC is triggered and the WorkerInspectorProxy gets collected.
> 
> Between 1) and 2), WorkerInspectorProxy::m_workerGlobalScopeProxy can be
stale.
> I think you need to call:
> 
>    m_workerInspectorProxy->setWorkerGlobalScopeProxy(nullptr);
> 
> in WorkerMessagingProxy's destructor.

Done.

[haraken, 2015-08-03 13:54:20]

LGTM if the inspector guy is fine with this.

[yurys, 2015-08-05 23:01:18]

lgtm

[keishi, 2015-08-06 06:46:04]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2015-08-06 06:46:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1253293002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1253293002/60001

[commit-bot: I haz the power, 2015-08-06 07:02:57]

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

[commit-bot: I haz the power, 2015-08-06 07:02:58]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[keishi, 2015-08-07 07:10:23]

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

[commit-bot: I haz the power, 2015-08-07 07:10:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1253293002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1253293002/80001

[commit-bot: I haz the power, 2015-08-07 07:18:51]

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

[commit-bot: I haz the power, 2015-08-07 07:18:52]

Dry run: Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/72965)

[keishi, 2015-08-11 04:55:40]

The CQ bit was checked by keishi@chromium.org

[keishi, 2015-08-11 04:55:41]

The patchset sent to the CQ was uploaded after l-g-t-m from yurys@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1253293002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-08-11 04:55:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1253293002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1253293002/140001

[commit-bot: I haz the power, 2015-08-11 05:19:26]

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

[commit-bot: I haz the power, 2015-08-11 05:19:27]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[keishi, 2015-08-31 09:08:46]

Patchset #11 (id:200001) has been deleted

[keishi, 2015-08-31 09:08:51]

The CQ bit was checked by keishi@chromium.org

[keishi, 2015-08-31 09:08:51]

The patchset sent to the CQ was uploaded after l-g-t-m from yurys@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1253293002/#ps180001
(title: " ")

[commit-bot: I haz the power, 2015-08-31 09:09:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1253293002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1253293002/180001

[commit-bot: I haz the power, 2015-08-31 10:51:09]

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

[commit-bot: I haz the power, 2015-08-31 10:51:11]

Failed to apply patch for Source/core/workers/WorkerInspectorProxy.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/workers/WorkerInspectorProxy.h
  Hunk #2 FAILED at 20.
  Hunk #3 succeeded at 53 (offset 1 line).
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/workers/WorkerInspectorProxy.h.rej

Patch:       Source/core/workers/WorkerInspectorProxy.h
Index: Source/core/workers/WorkerInspectorProxy.h
diff --git a/Source/core/workers/WorkerInspectorProxy.h
b/Source/core/workers/WorkerInspectorProxy.h
index
31ceee807b1cf3eb1733cdae62c69c64a4bc17f6..81bab2fb868ea2619409fd248589ce0c4caf2977
100644
--- a/Source/core/workers/WorkerInspectorProxy.h
+++ b/Source/core/workers/WorkerInspectorProxy.h
@@ -6,6 +6,7 @@
 #define WorkerInspectorProxy_h
 
 #include "core/CoreExport.h"
+#include "platform/heap/Handle.h"
 #include "public/platform/WebThread.h"
 #include "wtf/Forward.h"
 
@@ -19,17 +20,19 @@ class WorkerThread;
 
 // A proxy for talking to the worker inspector on the worker thread.
 // All of these methods should be called on the main thread.
-class CORE_EXPORT WorkerInspectorProxy final {
+class CORE_EXPORT WorkerInspectorProxy final : public
NoBaseWillBeGarbageCollectedFinalized<WorkerInspectorProxy> {
 public:
-    static PassOwnPtr<WorkerInspectorProxy> create();
+    static PassOwnPtrWillBeRawPtr<WorkerInspectorProxy> create();
 
     ~WorkerInspectorProxy();
+    DECLARE_TRACE();
 
-    class PageInspector {
+    class PageInspector : public
NoBaseWillBeGarbageCollectedFinalized<PageInspector> {
     public:
         virtual ~PageInspector() { }
         virtual void dispatchMessageFromWorker(const String&) = 0;
         virtual void workerConsoleAgentEnabled(WorkerGlobalScopeProxy*) = 0;
+        DEFINE_INLINE_VIRTUAL_TRACE() { }
     };
 
     void workerThreadCreated(ExecutionContext*, WorkerThread*, const KURL&);
@@ -51,8 +54,8 @@ private:
     void addDebuggerTaskForWorker(const WebTraceLocation&,
PassOwnPtr<WebThread::Task>);
 
     WorkerThread* m_workerThread;
-    ExecutionContext* m_executionContext;
-    WorkerInspectorProxy::PageInspector* m_pageInspector;
+    RawPtrWillBeMember<ExecutionContext> m_executionContext;
+    RawPtrWillBeMember<WorkerInspectorProxy::PageInspector> m_pageInspector;
     WorkerGlobalScopeProxy* m_workerGlobalScopeProxy;
 };

[commit-bot: I haz the power, 2015-08-31 10:51:14]

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

[keishi, 2015-08-31 11:28:24]

The CQ bit was checked by keishi@chromium.org

[keishi, 2015-08-31 11:28:25]

The patchset sent to the CQ was uploaded after l-g-t-m from yurys@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1253293002/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-08-31 11:28:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1253293002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1253293002/220001

[commit-bot: I haz the power, 2015-08-31 11:56:49]

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

[commit-bot: I haz the power, 2015-08-31 11:56:50]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[keishi, 2015-09-01 09:11:16]

The CQ bit was checked by keishi@chromium.org

[keishi, 2015-09-01 09:11:16]

The patchset sent to the CQ was uploaded after l-g-t-m from yurys@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1253293002/#ps240001
(title: " ")

[commit-bot: I haz the power, 2015-09-01 09:11:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1253293002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1253293002/240001

[commit-bot: I haz the power, 2015-09-01 10:44:59]

Committed patchset #12 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201548