Oilpan: Prepare moving InspectorController and InspectorAgents to oilpan.

Oilpan: Prepare moving InspectorController and InspectorAgents to oilpan.

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

[keishi, 2014-05-29 14:16:00]

Work in progress patch. 

I have a problem in WorkerDebuggerAgent.cpp:

workerDebuggerAgents() keeps a map from the thread to the agent.
We can't use m_inspectedWorkerGlobalScope in the destructor so I added
WorkerDebuggerAgentRemover to remove the agent from the map before destruction.
However when I run this, ~WorkerDebuggerAgentRemover is not called before
~WorkerDebuggerAgent.
~WorkerDebuggerAgent is called.
And then WorkerDebuggerAgent::WorkerDebuggerAgent is called, and it hits the
ASSERT because the entry for the thread was not removed.

Am I doing something wrong that causes ~WorkerDebuggerAgentRemover not to be
called?

[haraken, 2014-05-30 01:13:19]

https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
Source/core/inspector/WorkerDebuggerAgent.cpp:116:
workerDebuggerAgentRemovers().set(this, adoptPtr(new
WorkerDebuggerAgentRemover(thread)));

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.

[haraken, 2014-05-30 01:15:26]

Also let's consider breaking this kind of large CL into more pieces from next
time. I guess using RefCountedGarbageCollected will help us to move a large DOM
hierarchy incrementally.

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

https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
Source/core/inspector/WorkerDebuggerAgent.cpp:116:
workerDebuggerAgentRemovers().set(this, adoptPtr(new
WorkerDebuggerAgentRemover(thread)));
On 2014/05/30 01:13:20, haraken wrote:
> 
> 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.

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

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

hmm, as far as I see the code, I cannot find anything suspicious.

Probably you might want to minimize the change to reproduce the problem. For
example, can you reproduce the problem only by modifying
WorkerDebuggerAgentRemover (I think you can do it by making
InspectorDebuggerAgent a RefCountedGarbageCollected)?

https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
Source/core/inspector/WorkerDebuggerAgent.cpp:67:
workerDebuggerAgents().remove(m_thread);

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

https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
Source/core/inspector/WorkerDebuggerAgent.cpp:78:
DEFINE_STATIC_LOCAL(WorkerDebuggerAgentRemovers, removers, ());

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

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

?

https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
Source/core/inspector/WorkerDebuggerAgent.cpp:116:
workerDebuggerAgentRemovers().set(this, adoptPtr(new
WorkerDebuggerAgentRemover(thread)));
On 2014/05/30 02:01:07, keishi wrote:
> On 2014/05/30 01:13:20, haraken wrote:
> > 
> > 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.
> 
> Won't line 79 in ~WorkerDebuggerAgentRemover work as a replacement for line
124?

oh, line 67 should work.

[keishi, 2014-06-02 03:56:50]

On 2014/05/30 03:24:12, haraken wrote:
> hmm, as far as I see the code, I cannot find anything suspicious.
> 
> Probably you might want to minimize the change to reproduce the problem. For
> example, can you reproduce the problem only by modifying
> WorkerDebuggerAgentRemover (I think you can do it by making
> InspectorDebuggerAgent a RefCountedGarbageCollected)?
> 
>
https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
> File Source/core/inspector/WorkerDebuggerAgent.cpp (right):
> 
>
https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
> Source/core/inspector/WorkerDebuggerAgent.cpp:67:
> workerDebuggerAgents().remove(m_thread);
> 
> If you insert printf to ~WorkerDebuggerAgentRemover(), will it be printed? I
> want to confirm whether ~WorkerDebuggerAgentRemover() is never called or is
> called sometime later.
> 
>
https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
> Source/core/inspector/WorkerDebuggerAgent.cpp:78:
> DEFINE_STATIC_LOCAL(WorkerDebuggerAgentRemovers, removers, ());
> 
> I don't think it's related, but what happens if you use:
> 
>
DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<WillBeHeapHashMap<WeakMember<WorkerDebuggerAgent>,
> OwnPtr<WorkerDebuggerAgentRemover>>>)
> 
> ?
> 
>
https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
> Source/core/inspector/WorkerDebuggerAgent.cpp:116:
> workerDebuggerAgentRemovers().set(this, adoptPtr(new
> WorkerDebuggerAgentRemover(thread)));
> On 2014/05/30 02:01:07, keishi wrote:
> > On 2014/05/30 01:13:20, haraken wrote:
> > > 
> > > 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.
> > 
> > Won't line 79 in ~WorkerDebuggerAgentRemover work as a replacement for line
> 124?
> 
> oh, line 67 should work.

It seems like simply assigning a WorkerDebuggerAgent to a Persistent in a worker
thread causes assertion failure in the next gc.
ASSERTION FAILED: m_magic == magic
../../third_party/WebKit/Source/platform/heap/Heap.h(1275) : void
WebCore::HeapObjectHeader::checkHeader() const
1   0x62d158b WebCore::HeapObjectHeader::checkHeader() const
2   0x62ddfa1 WebCore::Visitor::checkGCInfo(void const*, WebCore::GCInfo const*)
3   0x9df2b2e WebCore::DefaultTraceTrait<WebCore::WorkerDebuggerAgent,
false>::checkGCInfo(WebCore::Visitor*, WebCore::WorkerDebuggerAgent const*)
4   0x9df2999
WebCore::TraceTrait<WebCore::WorkerDebuggerAgent>::checkGCInfo(WebCore::Visitor*,
WebCore::WorkerDebuggerAgent const*)
5   0x9df292a void
WebCore::Visitor::mark<WebCore::WorkerDebuggerAgent>(WebCore::WorkerDebuggerAgent*)
6   0x9df2897 WebCore::Persistent<WebCore::WorkerDebuggerAgent,
WebCore::ThreadLocalPersistents<(WebCore::ThreadAffinity)0>
>::trace(WebCore::Visitor*)
7   0x9df282d
WebCore::TraceMethodDelegate<WebCore::Persistent<WebCore::WorkerDebuggerAgent,
WebCore::ThreadLocalPersistents<(WebCore::ThreadAffinity)0> >,
&(WebCore::Persistent<WebCore::WorkerDebuggerAgent,
WebCore::ThreadLocalPersistents<(WebCore::ThreadAffinity)0>
>::trace(WebCore::Visitor*))>::trampoline(WebCore::Visitor*, void*)
8   0x62d6689 WebCore::PersistentNode::trace(WebCore::Visitor*)
9   0x62ddf33 WebCore::PersistentAnchor::trace(WebCore::Visitor*)
10  0x62dddbd WebCore::TraceMethodDelegate<WebCore::PersistentAnchor,
&(WebCore::PersistentAnchor::trace(WebCore::Visitor*))>::trampoline(WebCore::Visitor*,
void*)
11  0x62d6689 WebCore::PersistentNode::trace(WebCore::Visitor*)
12  0x62d3e57 WebCore::ThreadState::visitPersistents(WebCore::Visitor*)
13  0x62d3c3e WebCore::ThreadState::trace(WebCore::Visitor*)
14  0x62d3a80 WebCore::ThreadState::visitRoots(WebCore::Visitor*)
15  0x62c1851 WebCore::Heap::collectGarbage(WebCore::ThreadState::StackState)
16  0x62c3802 WebCore::Heap::collectAllGarbage()
17  0x62d378f WebCore::ThreadState::cleanup()
18  0x62d388f WebCore::ThreadState::detach()
19  0x9deded1 WebCore::WorkerThread::workerThread()
20  0x9deda8b WebCore::WorkerThread::workerThreadStart(void*)
21  0x17b57d77 WTF::threadEntryPoint(void*)
22  0x17b5860f WTF::wtfThreadEntryPoint(void*)

I couldn't reproduce the issue when moving just the WorkerDebuggerAgent to the
heap. 

Patch set 2 uses RawPtrs but passes all inspector/ tests.

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

https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
Source/core/inspector/WorkerDebuggerAgent.cpp:74: typedef
WillBePersistentHeapHashMap<WeakMember<WorkerDebuggerAgent>,
OwnPtr<WorkerDebuggerAgentRemover> > WorkerDebuggerAgentRemovers;

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?

[keishi, 2014-06-02 06:54:55]

On 2014/06/02 04:51:14, haraken wrote:
>
https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
> File Source/core/inspector/WorkerDebuggerAgent.cpp (right):
> 
>
https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
> Source/core/inspector/WorkerDebuggerAgent.cpp:74: typedef
> WillBePersistentHeapHashMap<WeakMember<WorkerDebuggerAgent>,
> OwnPtr<WorkerDebuggerAgentRemover> > WorkerDebuggerAgentRemovers;
> 
> 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?

I tried using CrossThreadPersistent but I hit the same assertion.
I;ve uploaded the patch and log here.
https://codereview.chromium.org/306053010/

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

https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
Source/core/inspector/WorkerDebuggerAgent.cpp:49: typedef HashMap<WorkerThread*,
WorkerDebuggerAgent*> WorkerDebuggerAgents;
Don't you need to make this a WillBePersistentHeapHashMap etc. to ensure the
WorkerDebuggerAgent is being traced?

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

Quick comment haven't looked at the actual patch much.

https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/20001/Source/core/inspector/Wo...
Source/core/inspector/WorkerDebuggerAgent.cpp:74: typedef
WillBePersistentHeapHashMap<WeakMember<WorkerDebuggerAgent>,
OwnPtr<WorkerDebuggerAgentRemover> > WorkerDebuggerAgentRemovers;
On 2014/06/02 04:51:14, haraken wrote:
> 
> 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?

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.

[keishi, 2014-06-02 12:06:31]

https://codereview.chromium.org/307943002/diff/60001/Source/core/inspector/Wo...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/60001/Source/core/inspector/Wo...
Source/core/inspector/WorkerDebuggerAgent.cpp:89:
workerDebuggerAgents().add(m_thread, nullptr/*adoptPtr(new
Persistent<WorkerDebuggerAgent>(this))*/);
I tried to do the same thing that DatabaseManager  is doing but adding this line
causes this crash. I can't tell what I'm doing wrong. Any ideas?

(lldb) r --debug --no-retry --single-process
inspector/profiler/heap-snapshot-loader.html
Process 18265 launched: '../../out/Debug/Content
Shell.app/Contents/MacOS/Content Shell' (i386)
objc[18265]: Class MockCrApp is implemented in both
/Users/keishi/chromium/src/out/Debug/libblink_web.dylib and
/Users/keishi/chromium/src/out/Debug/Content
Shell.app/Contents/Frameworks/Content Shell Framework.framework/Content Shell
Framework. One of the two will be used. Which one is undefined.
Process 18265 stopped
* thread #1: tid = 0xa3498f, 0x8fe01030 dyld`_dyld_start, stop reason = exec
    frame #0: 0x8fe01030 dyld`_dyld_start
dyld`_dyld_start:
-> 0x8fe01030:  popl   %edx
   0x8fe01031:  pushl  $0x0
   0x8fe01033:  movl   %esp, %ebp
   0x8fe01035:  andl   $-0x10, %esp

[Mads Ager (chromium), 2014-06-03 10:19:34]

https://codereview.chromium.org/307943002/diff/60001/Source/core/inspector/Wo...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/60001/Source/core/inspector/Wo...
Source/core/inspector/WorkerDebuggerAgent.cpp:89:
workerDebuggerAgents().add(m_thread, nullptr/*adoptPtr(new
Persistent<WorkerDebuggerAgent>(this))*/);
On 2014/06/02 12:06:31, keishi wrote:
> I tried to do the same thing that DatabaseManager  is doing but adding this
line
> causes this crash. I can't tell what I'm doing wrong. Any ideas?
> 
> (lldb) r --debug --no-retry --single-process
> inspector/profiler/heap-snapshot-loader.html
> Process 18265 launched: '../../out/Debug/Content
> Shell.app/Contents/MacOS/Content Shell' (i386)
> objc[18265]: Class MockCrApp is implemented in both
> /Users/keishi/chromium/src/out/Debug/libblink_web.dylib and
> /Users/keishi/chromium/src/out/Debug/Content
> Shell.app/Contents/Frameworks/Content Shell Framework.framework/Content Shell
> Framework. One of the two will be used. Which one is undefined.
> Process 18265 stopped
> * thread #1: tid = 0xa3498f, 0x8fe01030 dyld`_dyld_start, stop reason = exec
>     frame #0: 0x8fe01030 dyld`_dyld_start
> dyld`_dyld_start:
> -> 0x8fe01030:  popl   %edx
>    0x8fe01031:  pushl  $0x0
>    0x8fe01033:  movl   %esp, %ebp
>    0x8fe01035:  andl   $-0x10, %esp

I don't use lldb but I think your debugger is playing games with you here. I
have patched in your changes and when running this I get:

Command: 

gdb --args ./out/Debug/content_shell --dump-render-tree --single-process
--no-sandbox -

Crash:

ASSERTION FAILED: !workerDebuggerAgents().contains(m_thread)
../../third_party/WebKit/Source/core/inspector/WorkerDebuggerAgent.cpp(87) :
WebCore::WorkerDebuggerAgent::WorkerDebuggerAgent(WebCore::WorkerScriptDebugServer
*, WebCore::WorkerGlobalScope *, WebCore::InjectedScriptManager *)
1   0x7fffed1a5721
2   0x7fffed1a5619
3   0x7fffed1a7e01
4   0x7fffed279554
5   0x7fffed27228b
6   0x7fffed2721a7
7   0x7fffed272aea
8   0x7fffed286227
9   0x7fffed286115
10  0x7fffe5897ba0
11  0x7fffe5898078
12  0x7fffe6cc2e9a
13  0x7fffe69ef3fd clone

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffca05e700 (LWP 8531)]
0x00007fffed1a5730 in WebCore::WorkerDebuggerAgent::WorkerDebuggerAgent (
    this=0x7fffca861dc0, scriptDebugServer=0x2e283799fb20, 
    inspectedWorkerGlobalScope=0x7fffca861858, 
    injectedScriptManager=0x1b9f52dd7e80)
    at ../../third_party/WebKit/Source/core/inspector/WorkerDebuggerAgent.cpp:87
87	    ASSERT(!workerDebuggerAgents().contains(m_thread));

That makes a lot of sense because the destructor is not pulling the thread out
of the set. Restoring the destructor removes that assertion failure from this
test.

[keishi, 2014-06-04 19:17:40]

On 2014/06/03 10:19:34, Mads Ager (chromium) wrote:
>
https://codereview.chromium.org/307943002/diff/60001/Source/core/inspector/Wo...
> File Source/core/inspector/WorkerDebuggerAgent.cpp (right):
> 
>
https://codereview.chromium.org/307943002/diff/60001/Source/core/inspector/Wo...
> Source/core/inspector/WorkerDebuggerAgent.cpp:89:
> workerDebuggerAgents().add(m_thread, nullptr/*adoptPtr(new
> Persistent<WorkerDebuggerAgent>(this))*/);
> On 2014/06/02 12:06:31, keishi wrote:
> > I tried to do the same thing that DatabaseManager  is doing but adding this
> line
> > causes this crash. I can't tell what I'm doing wrong. Any ideas?
> > 
> > (lldb) r --debug --no-retry --single-process
> > inspector/profiler/heap-snapshot-loader.html
> > Process 18265 launched: '../../out/Debug/Content
> > Shell.app/Contents/MacOS/Content Shell' (i386)
> > objc[18265]: Class MockCrApp is implemented in both
> > /Users/keishi/chromium/src/out/Debug/libblink_web.dylib and
> > /Users/keishi/chromium/src/out/Debug/Content
> > Shell.app/Contents/Frameworks/Content Shell Framework.framework/Content
Shell
> > Framework. One of the two will be used. Which one is undefined.
> > Process 18265 stopped
> > * thread #1: tid = 0xa3498f, 0x8fe01030 dyld`_dyld_start, stop reason = exec
> >     frame #0: 0x8fe01030 dyld`_dyld_start
> > dyld`_dyld_start:
> > -> 0x8fe01030:  popl   %edx
> >    0x8fe01031:  pushl  $0x0
> >    0x8fe01033:  movl   %esp, %ebp
> >    0x8fe01035:  andl   $-0x10, %esp
> 
> I don't use lldb but I think your debugger is playing games with you here. I
> have patched in your changes and when running this I get:
> 
> Command: 
> 
> gdb --args ./out/Debug/content_shell --dump-render-tree --single-process
> --no-sandbox -
> 
> Crash:
> 
> ASSERTION FAILED: !workerDebuggerAgents().contains(m_thread)
> ../../third_party/WebKit/Source/core/inspector/WorkerDebuggerAgent.cpp(87) :
>
WebCore::WorkerDebuggerAgent::WorkerDebuggerAgent(WebCore::WorkerScriptDebugServer
> *, WebCore::WorkerGlobalScope *, WebCore::InjectedScriptManager *)
> 1   0x7fffed1a5721
> 2   0x7fffed1a5619
> 3   0x7fffed1a7e01
> 4   0x7fffed279554
> 5   0x7fffed27228b
> 6   0x7fffed2721a7
> 7   0x7fffed272aea
> 8   0x7fffed286227
> 9   0x7fffed286115
> 10  0x7fffe5897ba0
> 11  0x7fffe5898078
> 12  0x7fffe6cc2e9a
> 13  0x7fffe69ef3fd clone
> 
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7fffca05e700 (LWP 8531)]
> 0x00007fffed1a5730 in WebCore::WorkerDebuggerAgent::WorkerDebuggerAgent (
>     this=0x7fffca861dc0, scriptDebugServer=0x2e283799fb20, 
>     inspectedWorkerGlobalScope=0x7fffca861858, 
>     injectedScriptManager=0x1b9f52dd7e80)
>     at
../../third_party/WebKit/Source/core/inspector/WorkerDebuggerAgent.cpp:87
> 87	    ASSERT(!workerDebuggerAgents().contains(m_thread));
> 
> That makes a lot of sense because the destructor is not pulling the thread out
> of the set. Restoring the destructor removes that assertion failure from this
> test.

Thanks! I'm never using lldb again!

I found out that the WorkerDebuggerAgent constructor can be called from a
different thread but with the same WebThread pointer. It means
workerDebuggerAgents().set() will overwrite an OwnPtr and that causes Persistent
to be destroyed in a different thread than where it was allocated. So I had to
use a CrossThreadPersistent.
However when I use CrossThreadPersistent I seem to hit the ASSERT below.
I have no idea why this is happening. Is there some way around this?

ASSERTION FAILED: m_magic == magic
../../third_party/WebKit/Source/platform/heap/Heap.h(1275) : void
WebCore::HeapObjectHeader::checkHeader() const
1   0x7ffff6db7c37
2   0x7ffff6dbf425 WebCore::Visitor::checkGCInfo(void const*, WebCore::GCInfo
const*)
3   0x7fffed1811b5
4   0x7fffed1810bd
5   0x7fffed181088
6   0x7fffed181042
7   0x7fffed180ff6
8   0x7ffff6dbaf89
9   0x7ffff6dbf3de
10  0x7ffff6dbf346
11  0x7ffff6dbaf89
12  0x7ffff6db912f WebCore::ThreadState::visitRoots(WebCore::Visitor*)
13  0x7ffff6dad121
WebCore::Heap::collectGarbage(WebCore::ThreadState::StackState)
14  0x7ffff6dae6fa WebCore::Heap::collectAllGarbage()
15  0x7ffff6db8f1b WebCore::ThreadState::cleanup()
16  0x7ffff6db900a WebCore::ThreadState::detach()
17  0x7fffed25fe78
18  0x7fffed25fae5
19  0x7fffe586aba0
20  0x7fffe586b078
21  0x7fffe6c95e9a
22  0x7fffe69c23fd clone

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffcac5f700 (LWP 29062)]
0x00007ffff6db7c46 in WebCore::HeapObjectHeader::checkHeader
(this=0x7fffca421da8)
    at ../../third_party/WebKit/Source/platform/heap/Heap.h:1275
1275	    ASSERT(m_magic == magic);

[tkent, 2014-06-05 00:53:46]

On 2014/06/04 19:17:40, keishi wrote:
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7fffcac5f700 (LWP 29062)]
> 0x00007ffff6db7c46 in WebCore::HeapObjectHeader::checkHeader
> (this=0x7fffca421da8)
>     at ../../third_party/WebKit/Source/platform/heap/Heap.h:1275
> 1275	    ASSERT(m_magic == magic);

I think it's typically trace-after-free or heap corruption.

Can you split the patch to multiple pieces?  It's too large to review carefully.
e.g.
 - InspectorHistory and InspectorHistory::Action
 - InspectorAgent
 - others...

[haraken, 2014-06-05 01:26:48]

On 2014/06/04 19:17:40, keishi wrote:
> On 2014/06/03 10:19:34, Mads Ager (chromium) wrote:
> >
>
https://codereview.chromium.org/307943002/diff/60001/Source/core/inspector/Wo...
> > File Source/core/inspector/WorkerDebuggerAgent.cpp (right):
> > 
> >
>
https://codereview.chromium.org/307943002/diff/60001/Source/core/inspector/Wo...
> > Source/core/inspector/WorkerDebuggerAgent.cpp:89:
> > workerDebuggerAgents().add(m_thread, nullptr/*adoptPtr(new
> > Persistent<WorkerDebuggerAgent>(this))*/);
> > On 2014/06/02 12:06:31, keishi wrote:
> > > I tried to do the same thing that DatabaseManager  is doing but adding
this
> > line
> > > causes this crash. I can't tell what I'm doing wrong. Any ideas?
> > > 
> > > (lldb) r --debug --no-retry --single-process
> > > inspector/profiler/heap-snapshot-loader.html
> > > Process 18265 launched: '../../out/Debug/Content
> > > Shell.app/Contents/MacOS/Content Shell' (i386)
> > > objc[18265]: Class MockCrApp is implemented in both
> > > /Users/keishi/chromium/src/out/Debug/libblink_web.dylib and
> > > /Users/keishi/chromium/src/out/Debug/Content
> > > Shell.app/Contents/Frameworks/Content Shell Framework.framework/Content
> Shell
> > > Framework. One of the two will be used. Which one is undefined.
> > > Process 18265 stopped
> > > * thread #1: tid = 0xa3498f, 0x8fe01030 dyld`_dyld_start, stop reason =
exec
> > >     frame #0: 0x8fe01030 dyld`_dyld_start
> > > dyld`_dyld_start:
> > > -> 0x8fe01030:  popl   %edx
> > >    0x8fe01031:  pushl  $0x0
> > >    0x8fe01033:  movl   %esp, %ebp
> > >    0x8fe01035:  andl   $-0x10, %esp
> > 
> > I don't use lldb but I think your debugger is playing games with you here. I
> > have patched in your changes and when running this I get:
> > 
> > Command: 
> > 
> > gdb --args ./out/Debug/content_shell --dump-render-tree --single-process
> > --no-sandbox -
> > 
> > Crash:
> > 
> > ASSERTION FAILED: !workerDebuggerAgents().contains(m_thread)
> > ../../third_party/WebKit/Source/core/inspector/WorkerDebuggerAgent.cpp(87) :
> >
>
WebCore::WorkerDebuggerAgent::WorkerDebuggerAgent(WebCore::WorkerScriptDebugServer
> > *, WebCore::WorkerGlobalScope *, WebCore::InjectedScriptManager *)
> > 1   0x7fffed1a5721
> > 2   0x7fffed1a5619
> > 3   0x7fffed1a7e01
> > 4   0x7fffed279554
> > 5   0x7fffed27228b
> > 6   0x7fffed2721a7
> > 7   0x7fffed272aea
> > 8   0x7fffed286227
> > 9   0x7fffed286115
> > 10  0x7fffe5897ba0
> > 11  0x7fffe5898078
> > 12  0x7fffe6cc2e9a
> > 13  0x7fffe69ef3fd clone
> > 
> > Program received signal SIGSEGV, Segmentation fault.
> > [Switching to Thread 0x7fffca05e700 (LWP 8531)]
> > 0x00007fffed1a5730 in WebCore::WorkerDebuggerAgent::WorkerDebuggerAgent (
> >     this=0x7fffca861dc0, scriptDebugServer=0x2e283799fb20, 
> >     inspectedWorkerGlobalScope=0x7fffca861858, 
> >     injectedScriptManager=0x1b9f52dd7e80)
> >     at
> ../../third_party/WebKit/Source/core/inspector/WorkerDebuggerAgent.cpp:87
> > 87	    ASSERT(!workerDebuggerAgents().contains(m_thread));
> > 
> > That makes a lot of sense because the destructor is not pulling the thread
out
> > of the set. Restoring the destructor removes that assertion failure from
this
> > test.
> 
> Thanks! I'm never using lldb again!
> 
> I found out that the WorkerDebuggerAgent constructor can be called from a
> different thread but with the same WebThread pointer. It means
> workerDebuggerAgents().set() will overwrite an OwnPtr and that causes
Persistent
> to be destroyed in a different thread than where it was allocated. So I had to
> use a CrossThreadPersistent.
> However when I use CrossThreadPersistent I seem to hit the ASSERT below.
> I have no idea why this is happening. Is there some way around this?
> 
> ASSERTION FAILED: m_magic == magic
> ../../third_party/WebKit/Source/platform/heap/Heap.h(1275) : void
> WebCore::HeapObjectHeader::checkHeader() const
> 1   0x7ffff6db7c37
> 2   0x7ffff6dbf425 WebCore::Visitor::checkGCInfo(void const*, WebCore::GCInfo
> const*)
> 3   0x7fffed1811b5
> 4   0x7fffed1810bd
> 5   0x7fffed181088
> 6   0x7fffed181042
> 7   0x7fffed180ff6
> 8   0x7ffff6dbaf89
> 9   0x7ffff6dbf3de
> 10  0x7ffff6dbf346
> 11  0x7ffff6dbaf89
> 12  0x7ffff6db912f WebCore::ThreadState::visitRoots(WebCore::Visitor*)
> 13  0x7ffff6dad121
> WebCore::Heap::collectGarbage(WebCore::ThreadState::StackState)
> 14  0x7ffff6dae6fa WebCore::Heap::collectAllGarbage()
> 15  0x7ffff6db8f1b WebCore::ThreadState::cleanup()
> 16  0x7ffff6db900a WebCore::ThreadState::detach()
> 17  0x7fffed25fe78
> 18  0x7fffed25fae5
> 19  0x7fffe586aba0
> 20  0x7fffe586b078
> 21  0x7fffe6c95e9a
> 22  0x7fffe69c23fd clone
> 
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7fffcac5f700 (LWP 29062)]
> 0x00007ffff6db7c46 in WebCore::HeapObjectHeader::checkHeader
> (this=0x7fffca421da8)
>     at ../../third_party/WebKit/Source/platform/heap/Heap.h:1275
> 1275	    ASSERT(m_magic == magic);

Yeah, minimizing the CL would be helpful.

(FWIW, when I used CrossThreadPersistent in ExecutionContextTask, I hit the same
crash (and couldn't figure out what's going on). It's possible that
CrossThreadPersistent implementation has some bugs.)

[haraken, 2014-06-06 00:55:46]

On 2014/06/05 01:26:48, haraken wrote:
> On 2014/06/04 19:17:40, keishi wrote:
> > On 2014/06/03 10:19:34, Mads Ager (chromium) wrote:
> > >
> >
>
https://codereview.chromium.org/307943002/diff/60001/Source/core/inspector/Wo...
> > > File Source/core/inspector/WorkerDebuggerAgent.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/307943002/diff/60001/Source/core/inspector/Wo...
> > > Source/core/inspector/WorkerDebuggerAgent.cpp:89:
> > > workerDebuggerAgents().add(m_thread, nullptr/*adoptPtr(new
> > > Persistent<WorkerDebuggerAgent>(this))*/);
> > > On 2014/06/02 12:06:31, keishi wrote:
> > > > I tried to do the same thing that DatabaseManager  is doing but adding
> this
> > > line
> > > > causes this crash. I can't tell what I'm doing wrong. Any ideas?
> > > > 
> > > > (lldb) r --debug --no-retry --single-process
> > > > inspector/profiler/heap-snapshot-loader.html
> > > > Process 18265 launched: '../../out/Debug/Content
> > > > Shell.app/Contents/MacOS/Content Shell' (i386)
> > > > objc[18265]: Class MockCrApp is implemented in both
> > > > /Users/keishi/chromium/src/out/Debug/libblink_web.dylib and
> > > > /Users/keishi/chromium/src/out/Debug/Content
> > > > Shell.app/Contents/Frameworks/Content Shell Framework.framework/Content
> > Shell
> > > > Framework. One of the two will be used. Which one is undefined.
> > > > Process 18265 stopped
> > > > * thread #1: tid = 0xa3498f, 0x8fe01030 dyld`_dyld_start, stop reason =
> exec
> > > >     frame #0: 0x8fe01030 dyld`_dyld_start
> > > > dyld`_dyld_start:
> > > > -> 0x8fe01030:  popl   %edx
> > > >    0x8fe01031:  pushl  $0x0
> > > >    0x8fe01033:  movl   %esp, %ebp
> > > >    0x8fe01035:  andl   $-0x10, %esp
> > > 
> > > I don't use lldb but I think your debugger is playing games with you here.
I
> > > have patched in your changes and when running this I get:
> > > 
> > > Command: 
> > > 
> > > gdb --args ./out/Debug/content_shell --dump-render-tree --single-process
> > > --no-sandbox -
> > > 
> > > Crash:
> > > 
> > > ASSERTION FAILED: !workerDebuggerAgents().contains(m_thread)
> > > ../../third_party/WebKit/Source/core/inspector/WorkerDebuggerAgent.cpp(87)
:
> > >
> >
>
WebCore::WorkerDebuggerAgent::WorkerDebuggerAgent(WebCore::WorkerScriptDebugServer
> > > *, WebCore::WorkerGlobalScope *, WebCore::InjectedScriptManager *)
> > > 1   0x7fffed1a5721
> > > 2   0x7fffed1a5619
> > > 3   0x7fffed1a7e01
> > > 4   0x7fffed279554
> > > 5   0x7fffed27228b
> > > 6   0x7fffed2721a7
> > > 7   0x7fffed272aea
> > > 8   0x7fffed286227
> > > 9   0x7fffed286115
> > > 10  0x7fffe5897ba0
> > > 11  0x7fffe5898078
> > > 12  0x7fffe6cc2e9a
> > > 13  0x7fffe69ef3fd clone
> > > 
> > > Program received signal SIGSEGV, Segmentation fault.
> > > [Switching to Thread 0x7fffca05e700 (LWP 8531)]
> > > 0x00007fffed1a5730 in WebCore::WorkerDebuggerAgent::WorkerDebuggerAgent (
> > >     this=0x7fffca861dc0, scriptDebugServer=0x2e283799fb20, 
> > >     inspectedWorkerGlobalScope=0x7fffca861858, 
> > >     injectedScriptManager=0x1b9f52dd7e80)
> > >     at
> > ../../third_party/WebKit/Source/core/inspector/WorkerDebuggerAgent.cpp:87
> > > 87	    ASSERT(!workerDebuggerAgents().contains(m_thread));
> > > 
> > > That makes a lot of sense because the destructor is not pulling the thread
> out
> > > of the set. Restoring the destructor removes that assertion failure from
> this
> > > test.
> > 
> > Thanks! I'm never using lldb again!
> > 
> > I found out that the WorkerDebuggerAgent constructor can be called from a
> > different thread but with the same WebThread pointer. It means
> > workerDebuggerAgents().set() will overwrite an OwnPtr and that causes
> Persistent
> > to be destroyed in a different thread than where it was allocated. So I had
to
> > use a CrossThreadPersistent.
> > However when I use CrossThreadPersistent I seem to hit the ASSERT below.
> > I have no idea why this is happening. Is there some way around this?
> > 
> > ASSERTION FAILED: m_magic == magic
> > ../../third_party/WebKit/Source/platform/heap/Heap.h(1275) : void
> > WebCore::HeapObjectHeader::checkHeader() const
> > 1   0x7ffff6db7c37
> > 2   0x7ffff6dbf425 WebCore::Visitor::checkGCInfo(void const*,
WebCore::GCInfo
> > const*)
> > 3   0x7fffed1811b5
> > 4   0x7fffed1810bd
> > 5   0x7fffed181088
> > 6   0x7fffed181042
> > 7   0x7fffed180ff6
> > 8   0x7ffff6dbaf89
> > 9   0x7ffff6dbf3de
> > 10  0x7ffff6dbf346
> > 11  0x7ffff6dbaf89
> > 12  0x7ffff6db912f WebCore::ThreadState::visitRoots(WebCore::Visitor*)
> > 13  0x7ffff6dad121
> > WebCore::Heap::collectGarbage(WebCore::ThreadState::StackState)
> > 14  0x7ffff6dae6fa WebCore::Heap::collectAllGarbage()
> > 15  0x7ffff6db8f1b WebCore::ThreadState::cleanup()
> > 16  0x7ffff6db900a WebCore::ThreadState::detach()
> > 17  0x7fffed25fe78
> > 18  0x7fffed25fae5
> > 19  0x7fffe586aba0
> > 20  0x7fffe586b078
> > 21  0x7fffe6c95e9a
> > 22  0x7fffe69c23fd clone
> > 
> > Program received signal SIGSEGV, Segmentation fault.
> > [Switching to Thread 0x7fffcac5f700 (LWP 29062)]
> > 0x00007ffff6db7c46 in WebCore::HeapObjectHeader::checkHeader
> > (this=0x7fffca421da8)
> >     at ../../third_party/WebKit/Source/platform/heap/Heap.h:1275
> > 1275	    ASSERT(m_magic == magic);
> 
> Yeah, minimizing the CL would be helpful.
> 
> (FWIW, when I used CrossThreadPersistent in ExecutionContextTask, I hit the
same
> crash (and couldn't figure out what's going on). It's possible that
> CrossThreadPersistent implementation has some bugs.)

It looks like this crash is caused by the same issue as ExecutionContextTask;
i.e., the problem is that a CrossThreadPersistent is not allowed to outlive the
thread that allocated the underlying object. We currently don't have a mechanism
to allocate a GCed object that outlives the thread that allocated the object.

[keishi, 2014-06-09 14:19:44]

Thanks for all the help. I finally figured it out. I had an error and the
CrossThreadPersistent was outliving the WorkerThread. I fixed that and now all
inspector/ tests pass.
I'm going to split this into smaller CLs.

[keishi, 2014-06-10 08:50:02]

On 2014/06/09 14:19:44, keishi wrote:
> Thanks for all the help. I finally figured it out. I had an error and the
> CrossThreadPersistent was outliving the WorkerThread. I fixed that and now all
> inspector/ tests pass.
> I'm going to split this into smaller CLs.

I separated out the non InspectorAgent/InspectorController changes.
https://codereview.chromium.org/323043002/
Please take a look. Thanks!

[haraken, 2014-06-10 09:24:59]

On 2014/06/10 08:50:02, keishi wrote:
> On 2014/06/09 14:19:44, keishi wrote:
> > Thanks for all the help. I finally figured it out. I had an error and the
> > CrossThreadPersistent was outliving the WorkerThread. I fixed that and now
all
> > inspector/ tests pass.
> > I'm going to split this into smaller CLs.
> 
> I separated out the non InspectorAgent/InspectorController changes.
> https://codereview.chromium.org/323043002/
> Please take a look. Thanks!

Just to confirm: You want to land the patch set 7 first, and then
https://codereview.chromium.org/323043002/, right?

[keishi, 2014-06-10 09:30:38]

On 2014/06/10 09:24:59, haraken wrote:
> On 2014/06/10 08:50:02, keishi wrote:
> > On 2014/06/09 14:19:44, keishi wrote:
> > > Thanks for all the help. I finally figured it out. I had an error and the
> > > CrossThreadPersistent was outliving the WorkerThread. I fixed that and now
> all
> > > inspector/ tests pass.
> > > I'm going to split this into smaller CLs.
> > 
> > I separated out the non InspectorAgent/InspectorController changes.
> > https://codereview.chromium.org/323043002/
> > Please take a look. Thanks!
> 
> Just to confirm: You want to land the patch set 7 first, and then
> https://codereview.chromium.org/323043002/, right?

Sorry, yes.

[tkent, 2014-06-10 09:50:55]

https://codereview.chromium.org/307943002/diff/140001/Source/core/inspector/I...
File Source/core/inspector/InspectorController.cpp (right):

https://codereview.chromium.org/307943002/diff/140001/Source/core/inspector/I...
Source/core/inspector/InspectorController.cpp:88:
m_agents->append(InspectorInspectorAgent::create(m_page,
injectedScriptManager));
You may introduce |InspectorAgentRegistry& agents()| and use it.

https://codereview.chromium.org/307943002/diff/140001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMAgent.h (right):

https://codereview.chromium.org/307943002/diff/140001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.h:276:
OwnPtrWillBeMember<InspectorHistory> m_history;
InspectorHistory isn't on-heap yet, rigtht?

https://codereview.chromium.org/307943002/diff/140001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.h:277: OwnPtrWillBeMember<DOMEditor>
m_domEditor;
Ditto.

https://codereview.chromium.org/307943002/diff/140001/Source/core/inspector/W...
File Source/core/inspector/WorkerInspectorController.cpp (right):

https://codereview.chromium.org/307943002/diff/140001/Source/core/inspector/W...
Source/core/inspector/WorkerInspectorController.cpp:100:
m_agents->append(WorkerRuntimeAgent::create(m_injectedScriptManager.get(),
m_debugServer.get(), workerGlobalScope));
You may add |InspectorAgentRegistry& agents() { return *m_agents; }| and use it.

[pfeldman, 2014-06-10 10:42:54]

I glanced through it real quick. Could you leave raw pointers as is?

https://codereview.chromium.org/307943002/diff/140001/Source/core/inspector/I...
File Source/core/inspector/InspectorApplicationCacheAgent.h (right):

https://codereview.chromium.org/307943002/diff/140001/Source/core/inspector/I...
Source/core/inspector/InspectorApplicationCacheAgent.h:77:
RawPtrWillBeMember<InspectorPageAgent> m_pageAgent;
Could you leave raw pointers in place? We know these objects' lifetime and
ownership and rely upon it. This way object lifetime can be tracked and true
retainers are obvious.

https://codereview.chromium.org/307943002/diff/140001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/307943002/diff/140001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.h:199:
RawPtrWillBeMember<InspectorDOMAgent> m_domAgent;
Same for these

[pfeldman, 2014-06-10 10:48:04]

(unless there is a written policy on "no raw pointers to heap objects" you could
quote here. if there is, that is fine with me.)

[haraken, 2014-06-10 11:00:21]

On 2014/06/10 10:48:04, pfeldman_ooo wrote:
> (unless there is a written policy on "no raw pointers to heap objects" you
could
> quote here. if there is, that is fine with me.)

There is a policy on "no raw pointers to heap objects" :)

Raw pointers to on-heap objects can become stale unless you're very careful
about the lifetime management. Rather than paying the attention, we'd like to
adopt a policy "Use a Member if the pointer is strong, use a WeakMember if the
pointer is weak" and let the GC manage the lifetime.

[pfeldman, 2014-06-10 11:09:01]

Is it written somewhere? I like the way content/ api rules are listed on
chromium.org, do we have something for oilpan we refer to?

These are currently raw, which means we are already careful about managing them.
Clear policy on unconditionally migrating them all would be of help here.

Sometimes I prefer my asan bot to point me at failure rather than leaking entire
world of connected objects dangling on some node that is stuck in the bindings.

[pfeldman, 2014-06-10 11:14:12]

I don't see it in the list of rules here: 
http://www.chromium.org/blink/blink-gc

[haraken, 2014-06-10 11:22:14]

> I don't see it in the list of rules here: 
> http://www.chromium.org/blink/blink-gc

Sorry, let me add it (I don't have an edit permission at the moment).

> These are currently raw, which means we are already careful about managing
them.
> Clear policy on unconditionally migrating them all would be of help here.

If you use Members to each other, you don't need the clear code, like this:

// before
class A {
  RefPtr<B> m_b;
};

class B {
  void clear() { m_a = 0; }
  A* m_a;  // back pointer
};

// after
class A {
  Member<B> m_b;
};

class B {
  Member<A> m_a;
};

I think the after version is better because there is no risk of producing stale
pointers even if you forget to call clear() in some call path. (In other words,
in oilpan, risks of use-after-free are replaced with risks of memory leak. Leak
would be better than crashing.) That's why Oilpan wants to replace raw pointers
to on-heap objects with Members/WeakMembers and let the GC automatically manage
the lifetime instead of relying on manual management.

[pfeldman, 2014-06-10 11:30:17]

Your motivation is clear and I don't question its logic.

In this very case, a dozen of agents live as long as InspectorController does,
they raw point to each other knowing that they all live and die together (and
are careful about desctruction). So if we want to convert this case to members,
we should unconditionally convert everything. And if so, it is worth being
agreed upon and noted in the rules section. That's all I'm suggesting.

[haraken, 2014-06-10 11:34:49]

On 2014/06/10 11:30:17, pfeldman_ooo wrote:
> Your motivation is clear and I don't question its logic.
> 
> In this very case, a dozen of agents live as long as InspectorController does,
> they raw point to each other knowing that they all live and die together (and
> are careful about desctruction). So if we want to convert this case to
members,
> we should unconditionally convert everything. And if so, it is worth being
> agreed upon and noted in the rules section. That's all I'm suggesting.

Thanks, that's our plan (though we're not sure if we can make the change in one
go in this CL), so I'll add the description to the rules section.

[pfeldman, 2014-06-10 11:38:59]

Could we agree on that rule in a broader audience though? It is somewhat
controversial to my taste: in some sense what this refactoring is doing is
erasing the knowledge of the ownership from inspector controller / agents
infrastructure. I used to be able to tell what owns what via looking at the
code, I no longer can do that. Reducing the amount of design information that
code carries is at least as bad as stale pointers from the code maintenance
perspective. Asan would tell us about stale pointers, but there is no tool that
would restore the design principles that were used when creating the system, the
ones being erased.

[haraken, 2014-06-10 11:49:10]

On 2014/06/10 11:38:59, pfeldman_ooo wrote:
> Could we agree on that rule in a broader audience though? It is somewhat
> controversial to my taste: in some sense what this refactoring is doing is
> erasing the knowledge of the ownership from inspector controller / agents
> infrastructure. I used to be able to tell what owns what via looking at the
> code, I no longer can do that. Reducing the amount of design information that
> code carries is at least as bad as stale pointers from the code maintenance
> perspective. Asan would tell us about stale pointers, but there is no tool
that
> would restore the design principles that were used when creating the system,
the
> ones being erased.

ah, understood; this is a valid concern.

When we moved Page to the heap (Previously Pages were held by OwnPtrs; Oilpan
changed the OwnPtrs to Members), we lost the ownership information. As a result,
it became unclear when we should call Page::willBeDestroyed().

On the other hand, it's not easy to leave OwnPtrs as it, because OwnPtrs can
create a cycle and lead to leaks.

Mads: Do you have any idea on this?

[Mads Ager (chromium), 2014-06-10 11:59:06]

On 2014/06/10 11:49:10, haraken wrote:
> On 2014/06/10 11:38:59, pfeldman_ooo wrote:
> > Could we agree on that rule in a broader audience though? It is somewhat
> > controversial to my taste: in some sense what this refactoring is doing is
> > erasing the knowledge of the ownership from inspector controller / agents
> > infrastructure. I used to be able to tell what owns what via looking at the
> > code, I no longer can do that. Reducing the amount of design information
that
> > code carries is at least as bad as stale pointers from the code maintenance
> > perspective. Asan would tell us about stale pointers, but there is no tool
> that
> > would restore the design principles that were used when creating the system,
> the
> > ones being erased.
> 
> ah, understood; this is a valid concern.
> 
> When we moved Page to the heap (Previously Pages were held by OwnPtrs; Oilpan
> changed the OwnPtrs to Members), we lost the ownership information. As a
result,
> it became unclear when we should call Page::willBeDestroyed().
> 
> On the other hand, it's not easy to leave OwnPtrs as it, because OwnPtrs can
> create a cycle and lead to leaks.
> 
> Mads: Do you have any idea on this?

In a garbage collected system the garbage collector needs to know about *all*
pointers into the heap which rules out using raw pointers. With oilpan OwnPtrs
no longer make sense. If you hold a pointer to something and you are planning on
potentially using it you will trace it and keep it alive. It doesn't matter if
someone else will trace it too. Objects that are references from live objects
are not allowed to die, period. That is where the safety of a garbage collected
system is coming from.

I don't think there is anything we can nor should do here. We need to tell the
GC about *all* pointers. We can introduce some artificial pointer types that
contain the word Own and we can trace raw pointers without putting them in
Members. But that will be misleading and give the wrong impression of how the
system is operating.

In this particular case I find the Members here reassuring. It is a clear
indication that if this object is alive, so are the agents. They are kept alive
by this object and by the registry.

[keishi, 2014-06-10 13:11:12]

https://codereview.chromium.org/307943002/diff/140001/Source/core/inspector/I...
File Source/core/inspector/InspectorController.cpp (right):

https://codereview.chromium.org/307943002/diff/140001/Source/core/inspector/I...
Source/core/inspector/InspectorController.cpp:88:
m_agents->append(InspectorInspectorAgent::create(m_page,
injectedScriptManager));
On 2014/06/10 09:50:55, tkent wrote:
> You may introduce |InspectorAgentRegistry& agents()| and use it.

Done.

https://codereview.chromium.org/307943002/diff/140001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMAgent.h (right):

https://codereview.chromium.org/307943002/diff/140001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.h:276:
OwnPtrWillBeMember<InspectorHistory> m_history;
On 2014/06/10 09:50:55, tkent wrote:
> InspectorHistory isn't on-heap yet, rigtht?

I will land this CL first
https://codereview.chromium.org/298873004/

[pfeldman, 2014-06-10 13:24:59]

Mads, should we modify the rule number 4 in the blink-gc list and update the
team with it then? Ruling out raw pointers as a class is an easy one to
formalize. And it seems to clear out a lot of confusion and prevent threads like
this. You could also outline the ownership blurring as a known issue and say
that it is something oilpan has to sacrifice.

I mean it might have been clear to others from the start, but it was not to me.
I would end up using raw pointer just to emphasize that I knew what I was doing
in some object of the known lifetime.

[Mads Ager (chromium), 2014-06-10 13:41:43]

On 2014/06/10 13:24:59, pfeldman_ooo wrote:
> Mads, should we modify the rule number 4 in the blink-gc list and update the
> team with it then? Ruling out raw pointers as a class is an easy one to
> formalize. And it seems to clear out a lot of confusion and prevent threads
like
> this. You could also outline the ownership blurring as a known issue and say
> that it is something oilpan has to sacrifice.

Yes, we should. :)

(I actually think we are making things clearer and not blurring anything. Using
raw pointers that could become stale if you accidentally get the OwnPtr
destructed before you zap the last raw pointer seems a lot worse than keeping
the object pointed to alive. But that is a different discussion.)
 
> I mean it might have been clear to others from the start, but it was not to
me.
> I would end up using raw pointer just to emphasize that I knew what I was
doing
> in some object of the known lifetime.

Thanks for pointing out that the rules are not up to date. It's been a while
since I actually looked at the rules that we have put down in writing. I'll go
over them tomorrow and add a paragraph stating that all pointers to heap
allocated objects have to be explicitly handled by using Member or WeakMember.
Having a garbage collector does change the design principles used for managing
lifetime and we should be explicit about the rule.

[pfeldman, 2014-06-10 14:05:10]

thank you!

[tkent, 2014-06-11 05:37:15]

lgtm

[haraken, 2014-06-11 06:09:48]

This CL leaves a lot of raw pointers in on-heap objects. These raw pointers are
potentially dangerous for the following reason:

class A {  // off-heap
};

class B : public GarbageCollected<B> {
  A* m_a;  // Making B a GarbageCollected object delays the destruction of B
until the next GC is triggered. Thus m_a becomes a stale pointer unless we have
code that explicitly clears m_a when A is destructed.
};

For all raw pointers in on-heap objects, we need to make sure that they are
safe. (It's safe to leave raw pointers to on-heap objects as is at the moment,
but it would be better to replace them with Members/WeakMembers in the process
of the safety check.)

I understand this is a pain, but I think we need to do the safety check when
moving the object to the heap; otherwise we will end up leaving raw pointers
that have a risk of use-after-free.

I checked several raw pointers in this CL, but not all. There are too many...

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
File Source/core/inspector/InspectorApplicationCacheAgent.h (right):

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorApplicationCacheAgent.h:78:
InspectorFrontend::ApplicationCache* m_frontend;

This raw pointer looks safe because m_frontend is explicitly cleared in
clearFrontend(). For the same reason, all back pointers to Frontend objects in
this CL look safe.

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.h:205: HashMap<CSSStyleSheet*,
RefPtr<InspectorStyleSheet> > m_cssStyleSheetToInspectorStyleSheet;

I think this should be WillBeHeapHashMap<RawPtrWillBeMember<CSSStyleSheet>,
RefPtr<InspectorStyleSheet>>.

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.h:206: typedef HashMap<Document*,
OwnPtr<HashSet<CSSStyleSheet*> > > DocumentStyleSheets;

I think this should be HeapHashMap<Member<Document>,
OwnPtrWillBeMember<HeapHashSet<CSSStyleSheet>>>.

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.h:208: HashSet<Document*>
m_invalidatedDocuments;

This should be WillBeHeapHashSet<RawPtrWillBeMember<Document>>.

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
File Source/core/inspector/InspectorCanvasAgent.h (right):

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorCanvasAgent.h:101: InjectedScriptManager*
m_injectedScriptManager;

Raw pointers back to InjectedScriptManager look safe since InjectedScriptManager
should outlive agents.

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorCanvasAgent.h:105: typedef HashMap<LocalFrame*,
bool> FramesWithUninstrumentedCanvases;

This raw pointer looks safe since it's explicitly cleared in
frameDetachedFromParent().

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
File Source/core/inspector/InspectorController.h (right):

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorController.h:157: Page* m_page;

Page is on-heap, so we should use RawPtrWillBeMember<Page>. I think it's OK to
have a strong pointer back to the Page because this pointer is explicitly
cleared in Page::willBeDestroyed().

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorController.h:158: InspectorClient*
m_inspectorClient;

This raw pointer looks safe, since it's explicitly cleared in
Page::willBeDestroyed().

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMDebuggerAgent.h (right):

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMDebuggerAgent.h:131: HashMap<Node*, uint32_t>
m_domBreakpoints;

I think this Node pointer is strong. You can use
WillBeHeapHashMap<RawPtrWillBeMember<Node>, uint32_t>.

[Mads Ager (chromium), 2014-06-11 06:20:48]

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
File Source/core/inspector/InspectorController.h (right):

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorController.h:157: Page* m_page;
We should make m_page a Member as well and trace it. This is a back pointer to
the page and the InspectorController lives and dies with the page so we should
trace in both directions.

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
File Source/core/inspector/InspectorInspectorAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorInspectorAgent.cpp:67:
m_instrumentingAgents->setInspectorInspectorAgent(0);
The lifetime is not completely clear to me here. m_instrumentingAgents is a raw
pointer to an InstrumentingAgents object which holds a raw pointer back to this
InspectorInspectorAgent. Are we guaranteed that the InspectorInspectorAgent and
the InstrumentingAgents will live and die together so that we do not have to
clear the pointer here? It seems to me that we would have to move
InstrumentingAgents to the heap as well and trace in both directions if they are
supposed to live and die together or use weak pointers to perform the clearing
if needed?

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
File Source/core/inspector/InspectorResourceAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorResourceAgent.cpp:294:
ASSERT(!m_instrumentingAgents->inspectorResourceAgent());
Same question here. Since we are not changing m_instrumentingAgents in this
change I'm not sure that we can remove these asserts. What guarantees that the
instrumenting agents and the inspector resource agent live and die together
here?

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

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorWorkerAgent.cpp:122:
m_instrumentingAgents->setInspectorWorkerAgent(0);
Ditto.

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/P...
File Source/core/inspector/PageRuntimeAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/P...
Source/core/inspector/PageRuntimeAgent.cpp:59:
m_instrumentingAgents->setPageRuntimeAgent(0);
And here.

[keishi, 2014-06-11 14:10:56]

I added a comment to every raw pointer member

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.h:205: HashMap<CSSStyleSheet*,
RefPtr<InspectorStyleSheet> > m_cssStyleSheetToInspectorStyleSheet;
On 2014/06/11 06:09:48, haraken wrote:
> 
> I think this should be WillBeHeapHashMap<RawPtrWillBeMember<CSSStyleSheet>,
> RefPtr<InspectorStyleSheet>>.

Done.

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.h:206: typedef HashMap<Document*,
OwnPtr<HashSet<CSSStyleSheet*> > > DocumentStyleSheets;
On 2014/06/11 06:09:48, haraken wrote:
> 
> I think this should be HeapHashMap<Member<Document>,
> OwnPtrWillBeMember<HeapHashSet<CSSStyleSheet>>>.

Done.

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.h:208: HashSet<Document*>
m_invalidatedDocuments;
On 2014/06/11 06:09:48, haraken wrote:
> 
> This should be WillBeHeapHashSet<RawPtrWillBeMember<Document>>.

Done.

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
File Source/core/inspector/InspectorController.h (right):

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorController.h:157: Page* m_page;
On 2014/06/11 06:20:48, Mads Ager (chromium) wrote:
> We should make m_page a Member as well and trace it. This is a back pointer to
> the page and the InspectorController lives and dies with the page so we should
> trace in both directions.

Done.

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMDebuggerAgent.h (right):

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMDebuggerAgent.h:131: HashMap<Node*, uint32_t>
m_domBreakpoints;
On 2014/06/11 06:09:48, haraken wrote:
> 
> I think this Node pointer is strong. You can use
> WillBeHeapHashMap<RawPtrWillBeMember<Node>, uint32_t>.

Done.

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
File Source/core/inspector/InspectorInspectorAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/160001/Source/core/inspector/I...
Source/core/inspector/InspectorInspectorAgent.cpp:67:
m_instrumentingAgents->setInspectorInspectorAgent(0);
On 2014/06/11 06:20:48, Mads Ager (chromium) wrote:
> The lifetime is not completely clear to me here. m_instrumentingAgents is a
raw
> pointer to an InstrumentingAgents object which holds a raw pointer back to
this
> InspectorInspectorAgent. Are we guaranteed that the InspectorInspectorAgent
and
> the InstrumentingAgents will live and die together so that we do not have to
> clear the pointer here? It seems to me that we would have to move
> InstrumentingAgents to the heap as well and trace in both directions if they
are
> supposed to live and die together or use weak pointers to perform the clearing
> if needed?

I've confirmed that InstrumentingAgents does not outlive InspectorController
when running LayoutTests but I'm not sure we can guarantee that completely so
I'm moving InstrumentingAgents to the heap in this patch.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InjectedScriptHost.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptHost.h:101: ScriptDebugServer*
m_scriptDebugServer;
SAFE: Cleared in InjectedScriptHost::disconnect() which is called from
InjectedScriptManager::disconnect which is called in
InspectorController::willBeDestroyed.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorApplicationCacheAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorApplicationCacheAgent.h:78:
InspectorFrontend::ApplicationCache* m_frontend;
SAFE: Cleared in InspectorApplicationCacheAgent::clearFrontend which is called
from InspectorAgentRegistry::clearFrontend. InspectorFrontend::ApplicationCache
dies with InspectorFrontend and InspectorFrontend is owned by
InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorBaseAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorBaseAgent.h:90: InspectorCompositeState*
m_inspectorState;
SAFE: InspectorAgentRegistry and InspectorCompositeState are owned by
InspectorController so they die together.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.h:198: InspectorFrontend::CSS*
m_frontend;
SAFE: InspectorFrontend::CSS is owned by InspectorFrontend which is owned by
InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorCanvasAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCanvasAgent.h:101: InjectedScriptManager*
m_injectedScriptManager;
SAFE: InjectedScriptManager is owned by InspectorController

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCanvasAgent.h:102: InspectorFrontend::Canvas*
m_frontend;
SAFE: InspectorFrontend::Canvas is owned by InspectorFrontend, and
InspectorFrontend is owned by InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorConsoleAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorConsoleAgent.h:113: InjectedScriptManager*
m_injectedScriptManager;
SAFE: InjectedScriptManager and InspectorConsoleAgent are both owned by
InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorConsoleAgent.h:114: InspectorFrontend::Console*
m_frontend;
SAFE: InspectorFrontend::Console is owned by InspectorFrontend. And
InspectorFrontend is owned by InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorController.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorController.h:158: InspectorClient*
m_inspectorClient;
SAFE: InspectorClient is WebViewImpl . Cleared in
InspectorController::willBeDestroyed.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.h:259: InspectorOverlay* m_overlay;
SAFE: InspectorOverlay is owned by InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.h:260: InspectorFrontend::DOM*
m_frontend;
SAFE: InspectorFrontend::DOM is part of InspectorFrontend which is owned by
InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.h:261: DOMListener* m_domListener;
SAFE: DOMListener here is InspectorCSSAgent. Cleared by
InspectorCSSAgent::discardAgent which is called from
InspectorController::willBeDestroyed.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMStorageAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMStorageAgent.h:81: InspectorFrontend*
m_frontend;
SAFE: InspectorFrontend is owned by InspectorController

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorDebuggerAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDebuggerAgent.h:228: InjectedScriptManager*
m_injectedScriptManager;
SAFE: InjectedScriptManager is owned by InspectorController

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDebuggerAgent.h:229: InspectorFrontend::Debugger*
m_frontend;
SAFE: InspectorFrontend::Debugger is part of InspectorFrontend which is owned by
InspectorController

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorHeapProfilerAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorHeapProfilerAgent.h:86: InjectedScriptManager*
m_injectedScriptManager;
SAFE: InjectedScriptManager is owned by InspectorController

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorHeapProfilerAgent.h:87:
InspectorFrontend::HeapProfiler* m_frontend;
SAFE: InspectorFrontend::HeapProfiler is part of InspectorFrontend which is
owned by InspectorController

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorInspectorAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorInspectorAgent.h:88: InspectorFrontend*
m_frontend;
SAFE:  InspectorFrontend is owned by InspectorController

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorInspectorAgent.h:89: InjectedScriptManager*
m_injectedScriptManager;
SAFE:  InjectedScriptManager is owned by InspectorController

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorLayerTreeAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorLayerTreeAgent.h:101:
InspectorFrontend::LayerTree* m_frontend;
SAFE: InspectorFrontend::LayerTree is owned by InspectorFrontend which is owned
by InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorMemoryAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorMemoryAgent.h:61: InspectorFrontend::Memory*
m_frontend;
SAFE: InspectorFrontend::Memory is owned by InspectorFrontend which is owned
by InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorPageAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorPageAgent.h:181: InjectedScriptManager*
m_injectedScriptManager;
SAFE: InjectedScriptManager is owned by InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorPageAgent.h:182: InspectorClient* m_client;
SAFE: InspectorClient is WebViewImpl.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorPageAgent.h:183: InspectorFrontend::Page*
m_frontend;
SAFE: InspectorFrontend::Page is part of InspectorFrontend which is owned by
InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorPageAgent.h:184: InspectorOverlay* m_overlay;
SAFE: InspectorOverlay is owned by InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorProfilerAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorProfilerAgent.h:86: InjectedScriptManager*
m_injectedScriptManager;
SAFE: InjectedScriptManager is owned by InspectorController

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorProfilerAgent.h:87: InspectorFrontend::Profiler*
m_frontend;
SAFE: InspectorFrontend::Profiler is owned by InspectorFrontend which is owned
by InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorProfilerAgent.h:94: ProfileNameIdleTimeMap*
m_profileNameIdleTimeMap;
SAFE: satic allocated in ScriptProfiler::currentProfileNameIdleTimeMap

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorProfilerAgent.h:96: InspectorOverlay* m_overlay;
SAFE: InspectorOverlay is owned by InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorResourceAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorResourceAgent.h:154: InspectorFrontend::Network*
m_frontend;
SAFE: InspectorFrontend::Network is owned by InspectorFrontend which is owned
by InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorTimelineAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorTimelineAgent.h:292: InspectorFrontend::Timeline*
m_frontend;
SAFE: InspectorFrontend::Timeline is part of InspectorFrontend which is owned
by InspectorController.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorTimelineAgent.h:293: InspectorClient* m_client;
SAFE: InspectorClient is WebViewImpl

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorTimelineAgent.h:294: InspectorOverlay* m_overlay;
SAFE: InspectorOverlay is owned by InspectorController.

[haraken, 2014-06-11 16:03:28]

Thanks for the deep analysis! Let me take a close look tomorrow.

[haraken, 2014-06-12 05:53:01]

Thanks for the update!

I took a look at the first half of the files of this CL. I'm now looking at the
remaining ones.

https://codereview.chromium.org/307943002/diff/220001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8InjectedScriptManager.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8InjectedScriptManager.cpp:66:
V8DOMWrapper::setNativeInfoForHiddenWrapper(instanceTemplate,
&V8InjectedScriptHost::wrapperTypeInfo, host);

Should't this be setNativeInfoWithPersistentHandle ?

In my understanding, there is no hidden prototype chain about
InjectedScriptHost.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorBaseAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorBaseAgent.h:52: virtual void trace(Visitor*);

Add OVERRIDE.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorBaseAgent.h:68: InspectorState* m_state;

This raw pointer looks safe because InspectorCompositeState::m_inspectorStateMap
keeps the InspectorState alive while InspectorAgent holds the InspectorState.

However, the lifetime relationship is complicated, so it would be better to move
InspectorState to the heap.

Shall we add a FIXME and address in a follow-up?

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorBaseAgent.h:74: class InspectorAgentRegistry :
public NoBaseWillBeGarbageCollectedFinalized<InspectorAgentRegistry> {

Add FINAL.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorBaseAgent.h:90: InspectorCompositeState*
m_inspectorState;
On 2014/06/11 14:10:53, keishi wrote:
> SAFE: InspectorAgentRegistry and InspectorCompositeState are owned by
> InspectorController so they die together.

I agree that this is safe, but it would be better to move
InspectorCompositeState to the heap. If someone (other than InspectorController)
starts holding a Member to InspectorAgentRegistry in the future, there is no
guarantee that the InspectorAgentRegistry and the InspectorCompositeState die
together. Then m_inspectorState can be a stale pointer.

Shall we add a FIXME and address in a follow-up?

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:376: ASSERT(!m_domAgent);

You need to remove this in oilpan builds.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:377: reset();

This looks dangerous. reset() calls resetPseudoStates(), and resetPsedoStates()
touches m_nodeIdToForcedPseudoState, which is an on-heap object.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:548: Vector<CSSStyleSheet*>
newSheetsVector;

This should be WillBeHeapVector<RawPtrWillBeMember<CSSStyleSheet>>.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:553: void
InspectorCSSAgent::setActiveStyleSheets(Document* document, const
Vector<CSSStyleSheet*>& allSheetsVector, StyleSheetsUpdateType
styleSheetsUpdateType)

Ditto.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:1443: HashSet<Document*>
documentsToChange;

This should be WillBeHeapHashSet<RawPtrWillBeMember<Document>>.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.h:208:
WillBeHeapHashSet<RawPtrWillBeMember<Document> > m_invalidatedDocuments;

I haven't checked in details, but are you sure that the raw pointers in these
hash maps/sets are strong pointers, not weak pointers?

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorCanvasAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCanvasAgent.h:101: InjectedScriptManager*
m_injectedScriptManager;
On 2014/06/11 14:10:54, keishi wrote:
> SAFE: InjectedScriptManager is owned by InspectorController

In a follow-up, you can move InjectedScriptManager to the heap. Then you can
make this raw pointer definitely look safe and remove a persistent handle from
the InjectedScriptManager. FIXME in this CL?

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorConsoleAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorConsoleAgent.h:113: InjectedScriptManager*
m_injectedScriptManager;
On 2014/06/11 14:10:54, keishi wrote:
> SAFE: InjectedScriptManager and InspectorConsoleAgent are both owned by
> InspectorController.

Ditto. Moving InspectorScriptManager in a follow-up makes things clearer and
better.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.cpp:245: reset();

Do you need to call reset()? As far as I see, all the reset() does is just
clearing out on-heap collections in the InspectorDOMAgent. I don't think this is
needed in neither oilpan builds nor non-oilpan builds. (I wonder why we're
currently calling reset() in non-oilpan builds.)

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.h:106: virtual void trace(Visitor*);

Add OVERRIDE.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.h:259: InspectorOverlay* m_overlay;
On 2014/06/11 14:10:54, keishi wrote:
> SAFE: InspectorOverlay is owned by InspectorController.

Let's move this to the heap in a follow-up. FIXME in this CL?

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.h:261: DOMListener* m_domListener;
On 2014/06/11 14:10:54, keishi wrote:
> SAFE: DOMListener here is InspectorCSSAgent. Cleared by
> InspectorCSSAgent::discardAgent which is called from
> InspectorController::willBeDestroyed.

Given that InspectorCSSAgent is already on-heap, can we make the DOMListener a
GC-mixin and make this a Member?

[haraken, 2014-06-12 08:40:26]

I added comments on the remaining files.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMStorageAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMStorageAgent.h:74:
InspectorDOMStorageAgent(InspectorPageAgent*);

Add explicit.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorResourceAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorResourceAgent.cpp:289: if
(m_state->getBoolean(ResourceAgentState::resourceAgentEnabled)) {

I'm not quite sure if m_state is not a stale pointer at the point where
InspectorResourceAgent's destructor is called. (Probably it's safe but I'd like
to confirm.)

Once you move InspectorState & InspectorCompositeState to the heap and make the
InspectorState & InspectorCompositeState & Agents die together, things will
become clearer.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorResourceAgent.cpp:291: disable(&error);

This should be dangerous because disable() touches m_instrumentingAgents, which
is an on-heap object.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/P...
File Source/core/inspector/PageDebuggerAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/P...
Source/core/inspector/PageDebuggerAgent.h:81: PageScriptDebugServer*
m_pageScriptDebugServer;

You can move this to the heap in a follow-up.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/P...
File Source/core/inspector/PageRuntimeAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/P...
Source/core/inspector/PageRuntimeAgent.h:67: Page* m_inspectedPage;

Page is on-heap, so this should be a Member<Page>.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
Source/core/inspector/WorkerDebuggerAgent.cpp:50: typedef HashMap<WorkerThread*,
OwnPtr<CrossThreadPersistent<WorkerDebuggerAgent> > > WorkerDebuggerAgents;

Why does this need to be OwnPtr<CrossThreadPersistent<X>> instead of
CrossThreadPersistent<X>?

In the first place, I don't fully understand how the WorkerDebuggerAgents should
work. Would you elaborate on who adds/removes entries to/from the map when?

I guess this map can just be PersistentHashMap<WorkerThread*,
WeakMember<WorkerDebuggerAgent>>.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
File Source/core/inspector/WorkerInspectorController.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
Source/core/inspector/WorkerInspectorController.cpp:171:
m_debuggerAgent->discard();

We conventionally call this kind of method dispose().

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
File Source/core/inspector/WorkerInspectorController.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
Source/core/inspector/WorkerInspectorController.h:74: WorkerGlobalScope*
m_workerGlobalScope;

WorkerGlobalScope is on-heap, so you can use a Member<WorkerGlobalScope>.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
File Source/core/inspector/WorkerRuntimeAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
Source/core/inspector/WorkerRuntimeAgent.h:63: WorkerGlobalScope*
m_workerGlobalScope;

WorkerGlobalScope is on-heap, so this should be a Member<WorkerGlobalScope>.

https://codereview.chromium.org/307943002/diff/220001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:185: if (!m_workerInspectorController)

Is it possible that clearInspector() is called twice?

https://codereview.chromium.org/307943002/diff/220001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:187:
m_workerInspectorController->discard();

We conventionally call this kind of method dispose().

https://codereview.chromium.org/307943002/diff/220001/Source/modules/filesyst...
File Source/modules/filesystem/InspectorFileSystemAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/modules/filesyst...
Source/modules/filesystem/InspectorFileSystemAgent.h:63:
InspectorFileSystemAgent(Page*);

Add explicit.

https://codereview.chromium.org/307943002/diff/220001/Source/modules/geolocat...
File Source/modules/geolocation/GeolocationInspectorAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/modules/geolocat...
Source/modules/geolocation/GeolocationInspectorAgent.cpp:92: void
GeolocationInspectorAgent::AddController(GeolocationController* controller)

AddController => addController

https://codereview.chromium.org/307943002/diff/220001/Source/modules/geolocat...
Source/modules/geolocation/GeolocationInspectorAgent.cpp:97: void
GeolocationInspectorAgent::RemoveController(GeolocationController* controller)

RemoveController => removeController

https://codereview.chromium.org/307943002/diff/220001/Source/modules/indexedd...
File Source/modules/indexeddb/InspectorIndexedDBAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/modules/indexedd...
Source/modules/indexeddb/InspectorIndexedDBAgent.h:63:
InspectorIndexedDBAgent(Page*);

Add explicit.

[keishi, 2014-06-13 03:37:34]

https://codereview.chromium.org/307943002/diff/220001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8InjectedScriptManager.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8InjectedScriptManager.cpp:66:
V8DOMWrapper::setNativeInfoForHiddenWrapper(instanceTemplate,
&V8InjectedScriptHost::wrapperTypeInfo, host);
On 2014/06/12 05:53:00, haraken wrote:
> 
> Should't this be setNativeInfoWithPersistentHandle ?
> 
> In my understanding, there is no hidden prototype chain about
> InjectedScriptHost.

Done. Wasn't sure what to use for the PersistentNode argument.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorBaseAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorBaseAgent.h:52: virtual void trace(Visitor*);
On 2014/06/12 05:53:00, haraken wrote:
> 
> Add OVERRIDE.

This is the base class.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorBaseAgent.h:68: InspectorState* m_state;
On 2014/06/12 05:53:00, haraken wrote:
> 
> This raw pointer looks safe because
InspectorCompositeState::m_inspectorStateMap
> keeps the InspectorState alive while InspectorAgent holds the InspectorState.
> 
> However, the lifetime relationship is complicated, so it would be better to
move
> InspectorState to the heap.
> 
> Shall we add a FIXME and address in a follow-up?

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorBaseAgent.h:74: class InspectorAgentRegistry :
public NoBaseWillBeGarbageCollectedFinalized<InspectorAgentRegistry> {
On 2014/06/12 05:53:00, haraken wrote:
> 
> Add FINAL.

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorBaseAgent.h:90: InspectorCompositeState*
m_inspectorState;
On 2014/06/12 05:53:00, haraken wrote:
> On 2014/06/11 14:10:53, keishi wrote:
> > SAFE: InspectorAgentRegistry and InspectorCompositeState are owned by
> > InspectorController so they die together.
> 
> I agree that this is safe, but it would be better to move
> InspectorCompositeState to the heap. If someone (other than
InspectorController)
> starts holding a Member to InspectorAgentRegistry in the future, there is no
> guarantee that the InspectorAgentRegistry and the InspectorCompositeState die
> together. Then m_inspectorState can be a stale pointer.
> 
> Shall we add a FIXME and address in a follow-up?

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:376: ASSERT(!m_domAgent);
On 2014/06/12 05:53:00, haraken wrote:
> 
> You need to remove this in oilpan builds.

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:377: reset();
On 2014/06/12 05:53:01, haraken wrote:
> 
> This looks dangerous. reset() calls resetPseudoStates(), and
resetPsedoStates()
> touches m_nodeIdToForcedPseudoState, which is an on-heap object.

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:548: Vector<CSSStyleSheet*>
newSheetsVector;
On 2014/06/12 05:53:00, haraken wrote:
> 
> This should be WillBeHeapVector<RawPtrWillBeMember<CSSStyleSheet>>.

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:553: void
InspectorCSSAgent::setActiveStyleSheets(Document* document, const
Vector<CSSStyleSheet*>& allSheetsVector, StyleSheetsUpdateType
styleSheetsUpdateType)
On 2014/06/12 05:53:00, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:1443: HashSet<Document*>
documentsToChange;
On 2014/06/12 05:53:00, haraken wrote:
> 
> This should be WillBeHeapHashSet<RawPtrWillBeMember<Document>>.

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.h:208:
WillBeHeapHashSet<RawPtrWillBeMember<Document> > m_invalidatedDocuments;
On 2014/06/12 05:53:01, haraken wrote:
> 
> I haven't checked in details, but are you sure that the raw pointers in these
> hash maps/sets are strong pointers, not weak pointers?

m_cssStyleSheetToInspectorStyleSheet is in charge of keeping the
InspectorStyleSheets alive so it needs to be strong.
I thought it would be ok to make m_invalidatedDocuments weak but it made some
tests flaky timeout. I'm not sure what's going on.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorCanvasAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCanvasAgent.h:101: InjectedScriptManager*
m_injectedScriptManager;
On 2014/06/12 05:53:01, haraken wrote:
> On 2014/06/11 14:10:54, keishi wrote:
> > SAFE: InjectedScriptManager is owned by InspectorController
> 
> In a follow-up, you can move InjectedScriptManager to the heap. Then you can
> make this raw pointer definitely look safe and remove a persistent handle from
> the InjectedScriptManager. FIXME in this CL?

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorConsoleAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorConsoleAgent.h:113: InjectedScriptManager*
m_injectedScriptManager;
On 2014/06/12 05:53:01, haraken wrote:
> On 2014/06/11 14:10:54, keishi wrote:
> > SAFE: InjectedScriptManager and InspectorConsoleAgent are both owned by
> > InspectorController.
> 
> Ditto. Moving InspectorScriptManager in a follow-up makes things clearer and
> better.

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.cpp:245: reset();
On 2014/06/12 05:53:01, haraken wrote:
> 
> Do you need to call reset()? As far as I see, all the reset() does is just
> clearing out on-heap collections in the InspectorDOMAgent. I don't think this
is
> needed in neither oilpan builds nor non-oilpan builds. (I wonder why we're
> currently calling reset() in non-oilpan builds.)
> 

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.h:106: virtual void trace(Visitor*);
On 2014/06/12 05:53:01, haraken wrote:
> 
> Add OVERRIDE.

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.h:259: InspectorOverlay* m_overlay;
On 2014/06/12 05:53:01, haraken wrote:
> On 2014/06/11 14:10:54, keishi wrote:
> > SAFE: InspectorOverlay is owned by InspectorController.
> 
> Let's move this to the heap in a follow-up. FIXME in this CL?

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.h:261: DOMListener* m_domListener;
On 2014/06/12 05:53:01, haraken wrote:
> On 2014/06/11 14:10:54, keishi wrote:
> > SAFE: DOMListener here is InspectorCSSAgent. Cleared by
> > InspectorCSSAgent::discardAgent which is called from
> > InspectorController::willBeDestroyed.
> 
> Given that InspectorCSSAgent is already on-heap, can we make the DOMListener a
> GC-mixin and make this a Member?

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMStorageAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMStorageAgent.h:74:
InspectorDOMStorageAgent(InspectorPageAgent*);
On 2014/06/12 08:40:25, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorResourceAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorResourceAgent.cpp:291: disable(&error);
On 2014/06/12 08:40:25, haraken wrote:
> 
> This should be dangerous because disable() touches m_instrumentingAgents,
which
> is an on-heap object.

m_state, m_instrumentingAgents, m_resourcesData should all die together so I
don't think we need to do this in the destructor.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/P...
File Source/core/inspector/PageDebuggerAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/P...
Source/core/inspector/PageDebuggerAgent.h:81: PageScriptDebugServer*
m_pageScriptDebugServer;
On 2014/06/12 08:40:25, haraken wrote:
> 
> You can move this to the heap in a follow-up.

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/P...
File Source/core/inspector/PageRuntimeAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/P...
Source/core/inspector/PageRuntimeAgent.h:67: Page* m_inspectedPage;
On 2014/06/12 08:40:25, haraken wrote:
> 
> Page is on-heap, so this should be a Member<Page>.

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
Source/core/inspector/WorkerDebuggerAgent.cpp:50: typedef HashMap<WorkerThread*,
OwnPtr<CrossThreadPersistent<WorkerDebuggerAgent> > > WorkerDebuggerAgents;
On 2014/06/12 08:40:25, haraken wrote:
> 
> Why does this need to be OwnPtr<CrossThreadPersistent<X>> instead of
> CrossThreadPersistent<X>?

I get this error because CrossThreadPersistent doesn't have a default
constructor.
../../third_party/WebKit/Source/wtf/HashTraits.h:83:40: error: no matching
constructor for initialization of
'WebCore::CrossThreadPersistent<WebCore::WorkerDebuggerAgent>'
        static T emptyValue() { return T(); }

> In the first place, I don't fully understand how the WorkerDebuggerAgents
should
> work. Would you elaborate on who adds/removes entries to/from the map when?

WorkerDebuggerAgent is created when the WorkerThread starts.
When a WorkerDebuggerAgent is created it adds itself to the map.
We need to remove it from the map when the WorkerDebuggerAgent is destroyed,
which is when the WorkerThread is stopping.

The map is used when the user runs JavaScript in the worker using the debugger.
The task to run the debugger javascript is added to the worker run loop from the
main thread.
Then we use the map to lookup the WorkerDebuggerAgent for that worker in order
to find the WorkerScriptDebugServer.
WorkerScriptDebugServer executes the javascript.

Now that I said this it looks like this map could be removed so I removed it in
this next patch.

> I guess this map can just be PersistentHashMap<WorkerThread*,
> WeakMember<WorkerDebuggerAgent>>.
According to https://codereview.chromium.org/307943002/#msg10
We can't use PersistentHeapHashMap because the backing store might be owned by a
worker thread.
I tried using CrossThreadPersistent<HeapHashMap<WorkerThread*,
WeakMember<WorkerDebuggerAgent> > > but I still got a crash.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
File Source/core/inspector/WorkerInspectorController.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
Source/core/inspector/WorkerInspectorController.cpp:171:
m_debuggerAgent->discard();
On 2014/06/12 08:40:25, haraken wrote:
> 
> We conventionally call this kind of method dispose().

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
File Source/core/inspector/WorkerInspectorController.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
Source/core/inspector/WorkerInspectorController.h:74: WorkerGlobalScope*
m_workerGlobalScope;
On 2014/06/12 08:40:25, haraken wrote:
> 
> WorkerGlobalScope is on-heap, so you can use a Member<WorkerGlobalScope>.

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
File Source/core/inspector/WorkerRuntimeAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
Source/core/inspector/WorkerRuntimeAgent.h:63: WorkerGlobalScope*
m_workerGlobalScope;
On 2014/06/12 08:40:25, haraken wrote:
> 
> WorkerGlobalScope is on-heap, so this should be a Member<WorkerGlobalScope>.

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:185: if (!m_workerInspectorController)
On 2014/06/12 08:40:25, haraken wrote:
> 
> Is it possible that clearInspector() is called twice?

Seems like its called twice.

First from 
1   0x9e645cf WebCore::WorkerGlobalScope::clearInspector()
2   0x9e81e80
WebCore::WorkerThreadShutdownFinishTask::performTask(WebCore::ExecutionContext*)
3   0x9e793ca WebCore::WorkerRunLoopTask::run()
4   0x9e75d1a WebCore::WorkerRunLoop::runCleanupTasks()
5   0x9e753c0 WebCore::WorkerRunLoop::run()
6   0x9e7c881 WebCore::WorkerThread::runEventLoop()
7   0x9e55487 WebCore::DedicatedWorkerThread::runEventLoop()
8   0x9e7c775 WebCore::WorkerThread::workerThread()
9   0x9e7c3db WebCore::WorkerThread::workerThreadStart(void*)
10  0x17d82c77 WTF::threadEntryPoint(void*)
11  0x17d8350f WTF::wtfThreadEntryPoint(void*)
12  0x982c45fb _pthread_body
13  0x982c4485 _pthread_struct_init
14  0x982c9cf2 thread_start

And then from
1   0x9e3759e WebCore::WorkerGlobalScope::clearInspector()
2   0x9e37a13 WebCore::WorkerGlobalScope::dispose()
3   0x9e4f78f WebCore::WorkerThread::workerThread()
4   0x9e4f3bb WebCore::WorkerThread::workerThreadStart(void*)
5   0x17d5bc77 WTF::threadEntryPoint(void*)
6   0x17d5c50f WTF::wtfThreadEntryPoint(void*)
7   0x982c45fb _pthread_body
8   0x982c4485 _pthread_struct_init
9   0x982c9cf2 thread_start

https://codereview.chromium.org/307943002/diff/220001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:187:
m_workerInspectorController->discard();
On 2014/06/12 08:40:25, haraken wrote:
> 
> We conventionally call this kind of method dispose().

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/modules/filesyst...
File Source/modules/filesystem/InspectorFileSystemAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/modules/filesyst...
Source/modules/filesystem/InspectorFileSystemAgent.h:63:
InspectorFileSystemAgent(Page*);
On 2014/06/12 08:40:25, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/modules/geolocat...
File Source/modules/geolocation/GeolocationInspectorAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/modules/geolocat...
Source/modules/geolocation/GeolocationInspectorAgent.cpp:92: void
GeolocationInspectorAgent::AddController(GeolocationController* controller)
On 2014/06/12 08:40:25, haraken wrote:
> 
> AddController => addController

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/modules/geolocat...
Source/modules/geolocation/GeolocationInspectorAgent.cpp:97: void
GeolocationInspectorAgent::RemoveController(GeolocationController* controller)
On 2014/06/12 08:40:25, haraken wrote:
> 
> RemoveController => removeController

Done.

https://codereview.chromium.org/307943002/diff/220001/Source/modules/indexedd...
File Source/modules/indexeddb/InspectorIndexedDBAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/modules/indexedd...
Source/modules/indexeddb/InspectorIndexedDBAgent.h:63:
InspectorIndexedDBAgent(Page*);
On 2014/06/12 08:40:25, haraken wrote:
> 
> Add explicit.

Done.

[haraken, 2014-06-13 08:02:53]

Thanks for the update! I'll take a close look this evening.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.h:208:
WillBeHeapHashSet<RawPtrWillBeMember<Document> > m_invalidatedDocuments;
On 2014/06/13 03:37:33, keishi wrote:
> On 2014/06/12 05:53:01, haraken wrote:
> > 
> > I haven't checked in details, but are you sure that the raw pointers in
these
> > hash maps/sets are strong pointers, not weak pointers?
> 
> m_cssStyleSheetToInspectorStyleSheet is in charge of keeping the
> InspectorStyleSheets alive so it needs to be strong.
> I thought it would be ok to make m_invalidatedDocuments weak but it made some
> tests flaky timeout. I'm not sure what's going on.

We don't want to make things weak as much as possible, so we're completely fine
with strong members. However, we need to make sure that they are OK with being
strong.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
File Source/core/inspector/InspectorResourceAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/I...
Source/core/inspector/InspectorResourceAgent.cpp:291: disable(&error);
On 2014/06/13 03:37:34, keishi wrote:
> On 2014/06/12 08:40:25, haraken wrote:
> > 
> > This should be dangerous because disable() touches m_instrumentingAgents,
> which
> > is an on-heap object.
> 
> m_state, m_instrumentingAgents, m_resourcesData should all die together so I
> don't think we need to do this in the destructor.

Makes sense.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
Source/core/inspector/WorkerDebuggerAgent.cpp:50: typedef HashMap<WorkerThread*,
OwnPtr<CrossThreadPersistent<WorkerDebuggerAgent> > > WorkerDebuggerAgents;
On 2014/06/13 03:37:34, keishi wrote:
> On 2014/06/12 08:40:25, haraken wrote:
> > 
> > Why does this need to be OwnPtr<CrossThreadPersistent<X>> instead of
> > CrossThreadPersistent<X>?
> 
> I get this error because CrossThreadPersistent doesn't have a default
> constructor.
> ../../third_party/WebKit/Source/wtf/HashTraits.h:83:40: error: no matching
> constructor for initialization of
> 'WebCore::CrossThreadPersistent<WebCore::WorkerDebuggerAgent>'
>         static T emptyValue() { return T(); }
> 
> > In the first place, I don't fully understand how the WorkerDebuggerAgents
> should
> > work. Would you elaborate on who adds/removes entries to/from the map when?
> 
> WorkerDebuggerAgent is created when the WorkerThread starts.
> When a WorkerDebuggerAgent is created it adds itself to the map.
> We need to remove it from the map when the WorkerDebuggerAgent is destroyed,
> which is when the WorkerThread is stopping.
> 
> The map is used when the user runs JavaScript in the worker using the
debugger.
> The task to run the debugger javascript is added to the worker run loop from
the
> main thread.
> Then we use the map to lookup the WorkerDebuggerAgent for that worker in order
> to find the WorkerScriptDebugServer.
> WorkerScriptDebugServer executes the javascript.
> 
> Now that I said this it looks like this map could be removed so I removed it
in
> this next patch.
> 
> > I guess this map can just be PersistentHashMap<WorkerThread*,
> > WeakMember<WorkerDebuggerAgent>>.
> According to https://codereview.chromium.org/307943002/#msg10
> We can't use PersistentHeapHashMap because the backing store might be owned by
a
> worker thread.
> I tried using CrossThreadPersistent<HeapHashMap<WorkerThread*,
> WeakMember<WorkerDebuggerAgent> > > but I still got a crash.

I'm not quite sure what you're planning, but if you're planning to remove it,
that's great. Let's add a FIXME.

According to your analysis, it seems that we can just make the WorkerThread hold
a persistent handle to WorkerDebuggerAgent.

https://codereview.chromium.org/307943002/diff/220001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:185: if (!m_workerInspectorController)
On 2014/06/13 03:37:34, keishi wrote:
> On 2014/06/12 08:40:25, haraken wrote:
> > 
> > Is it possible that clearInspector() is called twice?
> 
> Seems like its called twice.
> 
> First from 
> 1   0x9e645cf WebCore::WorkerGlobalScope::clearInspector()
> 2   0x9e81e80
>
WebCore::WorkerThreadShutdownFinishTask::performTask(WebCore::ExecutionContext*)
> 3   0x9e793ca WebCore::WorkerRunLoopTask::run()
> 4   0x9e75d1a WebCore::WorkerRunLoop::runCleanupTasks()
> 5   0x9e753c0 WebCore::WorkerRunLoop::run()
> 6   0x9e7c881 WebCore::WorkerThread::runEventLoop()
> 7   0x9e55487 WebCore::DedicatedWorkerThread::runEventLoop()
> 8   0x9e7c775 WebCore::WorkerThread::workerThread()
> 9   0x9e7c3db WebCore::WorkerThread::workerThreadStart(void*)
> 10  0x17d82c77 WTF::threadEntryPoint(void*)
> 11  0x17d8350f WTF::wtfThreadEntryPoint(void*)
> 12  0x982c45fb _pthread_body
> 13  0x982c4485 _pthread_struct_init
> 14  0x982c9cf2 thread_start
> 
> And then from
> 1   0x9e3759e WebCore::WorkerGlobalScope::clearInspector()
> 2   0x9e37a13 WebCore::WorkerGlobalScope::dispose()
> 3   0x9e4f78f WebCore::WorkerThread::workerThread()
> 4   0x9e4f3bb WebCore::WorkerThread::workerThreadStart(void*)
> 5   0x17d5bc77 WTF::threadEntryPoint(void*)
> 6   0x17d5c50f WTF::wtfThreadEntryPoint(void*)
> 7   0x982c45fb _pthread_body
> 8   0x982c4485 _pthread_struct_init
> 9   0x982c9cf2 thread_start

Thanks, understood. It seems strange that clearInspector() is called twice, but
it's not related to your CL. I'll take a look.

[keishi, 2014-06-17 03:13:23]

PTAL.

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
Source/core/inspector/WorkerDebuggerAgent.cpp:50: typedef HashMap<WorkerThread*,
OwnPtr<CrossThreadPersistent<WorkerDebuggerAgent> > > WorkerDebuggerAgents;
On 2014/06/13 08:02:53, haraken wrote:
> On 2014/06/13 03:37:34, keishi wrote:
> > On 2014/06/12 08:40:25, haraken wrote:
> > > 
> > > Why does this need to be OwnPtr<CrossThreadPersistent<X>> instead of
> > > CrossThreadPersistent<X>?
> > 
> > I get this error because CrossThreadPersistent doesn't have a default
> > constructor.
> > ../../third_party/WebKit/Source/wtf/HashTraits.h:83:40: error: no matching
> > constructor for initialization of
> > 'WebCore::CrossThreadPersistent<WebCore::WorkerDebuggerAgent>'
> >         static T emptyValue() { return T(); }
> > 
> > > In the first place, I don't fully understand how the WorkerDebuggerAgents
> > should
> > > work. Would you elaborate on who adds/removes entries to/from the map
when?
> > 
> > WorkerDebuggerAgent is created when the WorkerThread starts.
> > When a WorkerDebuggerAgent is created it adds itself to the map.
> > We need to remove it from the map when the WorkerDebuggerAgent is destroyed,
> > which is when the WorkerThread is stopping.
> > 
> > The map is used when the user runs JavaScript in the worker using the
> debugger.
> > The task to run the debugger javascript is added to the worker run loop from
> the
> > main thread.
> > Then we use the map to lookup the WorkerDebuggerAgent for that worker in
order
> > to find the WorkerScriptDebugServer.
> > WorkerScriptDebugServer executes the javascript.
> > 
> > Now that I said this it looks like this map could be removed so I removed it
> in
> > this next patch.
> > 
> > > I guess this map can just be PersistentHashMap<WorkerThread*,
> > > WeakMember<WorkerDebuggerAgent>>.
> > According to https://codereview.chromium.org/307943002/#msg10
> > We can't use PersistentHeapHashMap because the backing store might be owned
by
> a
> > worker thread.
> > I tried using CrossThreadPersistent<HeapHashMap<WorkerThread*,
> > WeakMember<WorkerDebuggerAgent> > > but I still got a crash.
> 
> I'm not quite sure what you're planning, but if you're planning to remove it,
> that's great. Let's add a FIXME.
> 
> According to your analysis, it seems that we can just make the WorkerThread
hold
> a persistent handle to WorkerDebuggerAgent.

I've already removed the map in my CL.

https://codereview.chromium.org/307943002/diff/220001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/307943002/diff/220001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:185: if (!m_workerInspectorController)
On 2014/06/13 08:02:53, haraken wrote:
> On 2014/06/13 03:37:34, keishi wrote:
> > On 2014/06/12 08:40:25, haraken wrote:
> > > 
> > > Is it possible that clearInspector() is called twice?
> > 
> > Seems like its called twice.
> > 
> > First from 
> > 1   0x9e645cf WebCore::WorkerGlobalScope::clearInspector()
> > 2   0x9e81e80
> >
>
WebCore::WorkerThreadShutdownFinishTask::performTask(WebCore::ExecutionContext*)
> > 3   0x9e793ca WebCore::WorkerRunLoopTask::run()
> > 4   0x9e75d1a WebCore::WorkerRunLoop::runCleanupTasks()
> > 5   0x9e753c0 WebCore::WorkerRunLoop::run()
> > 6   0x9e7c881 WebCore::WorkerThread::runEventLoop()
> > 7   0x9e55487 WebCore::DedicatedWorkerThread::runEventLoop()
> > 8   0x9e7c775 WebCore::WorkerThread::workerThread()
> > 9   0x9e7c3db WebCore::WorkerThread::workerThreadStart(void*)
> > 10  0x17d82c77 WTF::threadEntryPoint(void*)
> > 11  0x17d8350f WTF::wtfThreadEntryPoint(void*)
> > 12  0x982c45fb _pthread_body
> > 13  0x982c4485 _pthread_struct_init
> > 14  0x982c9cf2 thread_start
> > 
> > And then from
> > 1   0x9e3759e WebCore::WorkerGlobalScope::clearInspector()
> > 2   0x9e37a13 WebCore::WorkerGlobalScope::dispose()
> > 3   0x9e4f78f WebCore::WorkerThread::workerThread()
> > 4   0x9e4f3bb WebCore::WorkerThread::workerThreadStart(void*)
> > 5   0x17d5bc77 WTF::threadEntryPoint(void*)
> > 6   0x17d5c50f WTF::wtfThreadEntryPoint(void*)
> > 7   0x982c45fb _pthread_body
> > 8   0x982c4485 _pthread_struct_init
> > 9   0x982c9cf2 thread_start
> 
> Thanks, understood. It seems strange that clearInspector() is called twice,
but
> it's not related to your CL. I'll take a look.

Yes I think we can remove the call to clearInspector in
WorkerThreadShutdownFinishTask but I won't do it in this CL.

[haraken, 2014-06-17 14:06:18]

I'm sorry about the review delay. Here are final rounds of comments.

https://codereview.chromium.org/307943002/diff/260001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8InjectedScriptManager.cpp (right):

https://codereview.chromium.org/307943002/diff/260001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8InjectedScriptManager.cpp:66:
V8DOMWrapper::setNativeInfoWithPersistentHandle(instanceTemplate,
&V8InjectedScriptHost::wrapperTypeInfo, host, new
Persistent<InjectedScriptHost>(host));

I think this will leak memory in non-oilpan builds. You'll need to use
setNativeInfo in non-oilpan builds and setNativeInfoWithPersistentHandle in
oilpan builds.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorBaseAgent.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorBaseAgent.h:107: InspectorBaseAgent(const String&
name) : InspectorAgent(name)
Add explicit.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:360: reset();

I'm not quite sure of this. Is it OK to not call resetPseudoStates() in the
destructor? I understand we cannot call it since it touches on-heap objects, but
I'm not convinced why it's OK to not call it.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.h (left):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.h:207: HashSet<Document*>
m_invalidatedDocuments;

I confirmed that these pointers are strong pointers. So these changes look
correct.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorController.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorController.h:159:
OwnPtrWillBeMember<InspectorAgentRegistry> m_agents;

I think you can keep InspectorAgentRegistry as a part of object. You can add
DISALLOW_ALLOCATION to InspectorAgentRegistry.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMAgent.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.h:280: Listener* m_listener;

I think you can make InspectorDOMAgent::Listener a GC-mixin, and use a Member
for m_listener.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorDebuggerAgent.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorDebuggerAgent.h:245: Listener* m_listener;

You can make InspectorDebuggerAgent::Listener a GC-mixin, and use a Member for
m_listener.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorResourceAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorResourceAgent.cpp:290: if
(m_state->getBoolean(ResourceAgentState::resourceAgentEnabled)) {

Removing the line 290 - 294 is fine in oilpan builds, since m_state and
m_instrumentingAgents die together with the InspectorResourceAgent.

However, I guess we can also remove the line 290 - 294 in non-oilpan builds as
well. Since clearFrontEnd() is already called, we won't need to call disable()
in the destructor. Not related to your CL though.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorResourceAgent.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorResourceAgent.h:149:
InspectorResourceAgent(InspectorPageAgent*);

Add explicit.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/W...
File Source/core/inspector/WorkerDebuggerAgent.cpp (left):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/W...
Source/core/inspector/WorkerDebuggerAgent.cpp:53:
DEFINE_STATIC_LOCAL(WorkerDebuggerAgents, agents, ());

This is a nice change!

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/W...
File Source/core/inspector/WorkerInspectorController.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/W...
Source/core/inspector/WorkerInspectorController.h:81:
OwnPtrWillBeMember<InspectorAgentRegistry> m_agents;

You can keep the InspectorAgentRegistry as a part of object.

https://codereview.chromium.org/307943002/diff/260001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:185: if (!m_workerInspectorController)

Would you add a FIXME and mention that clearInspector should not be called
twice?

https://codereview.chromium.org/307943002/diff/260001/Source/modules/geolocat...
File Source/modules/geolocation/GeolocationInspectorAgent.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/modules/geolocat...
Source/modules/geolocation/GeolocationInspectorAgent.h:66:
WillBeHeapHashSet<RawPtrWillBeMember<GeolocationController> > m_controllers;

You can use GeoLocationControllers.

[keishi, 2014-06-18 12:31:07]

https://codereview.chromium.org/307943002/diff/260001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8InjectedScriptManager.cpp (right):

https://codereview.chromium.org/307943002/diff/260001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8InjectedScriptManager.cpp:66:
V8DOMWrapper::setNativeInfoWithPersistentHandle(instanceTemplate,
&V8InjectedScriptHost::wrapperTypeInfo, host, new
Persistent<InjectedScriptHost>(host));
On 2014/06/17 14:06:17, haraken wrote:
> 
> I think this will leak memory in non-oilpan builds. You'll need to use
> setNativeInfo in non-oilpan builds and setNativeInfoWithPersistentHandle in
> oilpan builds.

Done.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorBaseAgent.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorBaseAgent.h:107: InspectorBaseAgent(const String&
name) : InspectorAgent(name)
On 2014/06/17 14:06:17, haraken wrote:
> Add explicit.

Done.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:360: reset();
On 2014/06/17 14:06:17, haraken wrote:
> 
> I'm not quite sure of this. Is it OK to not call resetPseudoStates() in the
> destructor? I understand we cannot call it since it touches on-heap objects,
but
> I'm not convinced why it's OK to not call it.
> 

InspectorController::willBeDestroyed calls InspectorCSSAgent::clearFrontend
which calls resetPseudoStates().

InspectorCSSAgent always dies after InspectorController, so I think
resetPseudoStates should be already done.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorController.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorController.h:159:
OwnPtrWillBeMember<InspectorAgentRegistry> m_agents;
On 2014/06/17 14:06:17, haraken wrote:
> 
> I think you can keep InspectorAgentRegistry as a part of object. You can add
> DISALLOW_ALLOCATION to InspectorAgentRegistry.

Done.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMAgent.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.h:280: Listener* m_listener;
On 2014/06/17 14:06:17, haraken wrote:
> 
> I think you can make InspectorDOMAgent::Listener a GC-mixin, and use a Member
> for m_listener.

Done.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorDebuggerAgent.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorDebuggerAgent.h:245: Listener* m_listener;
On 2014/06/17 14:06:17, haraken wrote:
> 
> You can make InspectorDebuggerAgent::Listener a GC-mixin, and use a Member for
> m_listener.

Done.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorResourceAgent.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorResourceAgent.h:149:
InspectorResourceAgent(InspectorPageAgent*);
On 2014/06/17 14:06:17, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/W...
File Source/core/inspector/WorkerInspectorController.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/W...
Source/core/inspector/WorkerInspectorController.h:81:
OwnPtrWillBeMember<InspectorAgentRegistry> m_agents;
On 2014/06/17 14:06:18, haraken wrote:
> 
> You can keep the InspectorAgentRegistry as a part of object.

Done.

https://codereview.chromium.org/307943002/diff/260001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:185: if (!m_workerInspectorController)
On 2014/06/17 14:06:18, haraken wrote:
> 
> Would you add a FIXME and mention that clearInspector should not be called
> twice?

Done.

https://codereview.chromium.org/307943002/diff/260001/Source/modules/geolocat...
File Source/modules/geolocation/GeolocationInspectorAgent.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/modules/geolocat...
Source/modules/geolocation/GeolocationInspectorAgent.h:66:
WillBeHeapHashSet<RawPtrWillBeMember<GeolocationController> > m_controllers;
On 2014/06/17 14:06:18, haraken wrote:
> 
> You can use GeoLocationControllers.

Done.

[haraken, 2014-06-18 12:45:10]

LGTM.

Let's move the following objects to the heap and remove raw pointers to these
objects in follow-up CLs:
- InjectedScriptManager
- InspectorState
- InspectorCompositeState
- InspectorOverlay

Let's wait for an approval from yuyrs or pfeldman.

[yurys, 2014-06-18 14:23:37]

https://codereview.chromium.org/307943002/diff/280001/Source/core/inspector/I...
File Source/core/inspector/InspectorController.cpp (right):

https://codereview.chromium.org/307943002/diff/280001/Source/core/inspector/I...
Source/core/inspector/InspectorController.cpp:92:
agents().append(pageAgentPtr.release());
What's the point in having agents() compared to using m_agents-> directly?

https://codereview.chromium.org/307943002/diff/280001/Source/core/inspector/I...
File Source/core/inspector/InspectorTimelineAgent.h (right):

https://codereview.chromium.org/307943002/diff/280001/Source/core/inspector/I...
Source/core/inspector/InspectorTimelineAgent.h:92: : public
InspectorBaseAgent<InspectorTimelineAgent>
Why did this change?

https://codereview.chromium.org/307943002/diff/280001/Source/core/inspector/W...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/280001/Source/core/inspector/W...
Source/core/inspector/WorkerDebuggerAgent.cpp:86: WorkerGlobalScope*
workerGlobalScope = thread->workerGlobalScope();
Why WorkerGlobalScope cannot be already null when this method is called? It
seems to me that it may happen if that worker thread have already started
terminating.

[keishi, 2014-06-18 14:47:09]

https://codereview.chromium.org/307943002/diff/280001/Source/core/inspector/I...
File Source/core/inspector/InspectorController.cpp (right):

https://codereview.chromium.org/307943002/diff/280001/Source/core/inspector/I...
Source/core/inspector/InspectorController.cpp:92:
agents().append(pageAgentPtr.release());
On 2014/06/18 14:23:37, yurys wrote:
> What's the point in having agents() compared to using m_agents-> directly?

I'm not sure. tkent@ could you explain?
https://codereview.chromium.org/307943002/#msg21

https://codereview.chromium.org/307943002/diff/280001/Source/core/inspector/I...
File Source/core/inspector/InspectorTimelineAgent.h (right):

https://codereview.chromium.org/307943002/diff/280001/Source/core/inspector/I...
Source/core/inspector/InspectorTimelineAgent.h:92: : public
InspectorBaseAgent<InspectorTimelineAgent>
On 2014/06/18 14:23:37, yurys wrote:
> Why did this change?

We need the GarbageCollected class needs to be left-most class.
When GC scans the heap, the GarbageCollected(Finalized) class treats the
beginning address of the GarbageCollected object as the head address of the
object.

https://codereview.chromium.org/307943002/diff/280001/Source/core/inspector/W...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/280001/Source/core/inspector/W...
Source/core/inspector/WorkerDebuggerAgent.cpp:86: WorkerGlobalScope*
workerGlobalScope = thread->workerGlobalScope();
On 2014/06/18 14:23:37, yurys wrote:
> Why WorkerGlobalScope cannot be already null when this method is called? It
> seems to me that it may happen if that worker thread have already started
> terminating.

You're right. That could happen. Done.

[tkent, 2014-06-18 23:21:29]

https://codereview.chromium.org/307943002/diff/280001/Source/core/inspector/I...
File Source/core/inspector/InspectorController.cpp (right):

https://codereview.chromium.org/307943002/diff/280001/Source/core/inspector/I...
Source/core/inspector/InspectorController.cpp:92:
agents().append(pageAgentPtr.release());
On 2014/06/18 14:47:09, keishi wrote:
> On 2014/06/18 14:23:37, yurys wrote:
> > What's the point in having agents() compared to using m_agents-> directly?
> 
> I'm not sure. tkent@ could you explain?
> https://codereview.chromium.org/307943002/#msg21

m_agents is always non-null, and we prefer using references for non-null objects
like the original code.

[yurys, 2014-06-19 07:37:03]

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorController.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorController.h:159:
OwnPtrWillBeMember<InspectorAgentRegistry> m_agents;
On 2014/06/18 12:31:06, keishi wrote:
> On 2014/06/17 14:06:17, haraken wrote:
> > 
> > I think you can keep InspectorAgentRegistry as a part of object. You can add
> > DISALLOW_ALLOCATION to InspectorAgentRegistry.
> 
> Done.

And now you don't need
InspectorAgentRegistry& agents() { return m_agents; }

https://codereview.chromium.org/307943002/diff/300001/Source/core/inspector/W...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/300001/Source/core/inspector/W...
Source/core/inspector/WorkerDebuggerAgent.cpp:87: if (!workerGlobalScope)
This method is called on the main thread while WorkerGlobalScope is created and
destroyed on the worker thread. It may well happen now that WorkerGlobalScope is
getting destroyed while we are using it in this method. Is there a reason we
cannot leave previous implementation that used a map guarded by a mutex?

[keishi, 2014-06-19 12:52:07]

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
File Source/core/inspector/InspectorController.h (right):

https://codereview.chromium.org/307943002/diff/260001/Source/core/inspector/I...
Source/core/inspector/InspectorController.h:159:
OwnPtrWillBeMember<InspectorAgentRegistry> m_agents;
On 2014/06/19 07:37:03, yurys wrote:
> On 2014/06/18 12:31:06, keishi wrote:
> > On 2014/06/17 14:06:17, haraken wrote:
> > > 
> > > I think you can keep InspectorAgentRegistry as a part of object. You can
add
> > > DISALLOW_ALLOCATION to InspectorAgentRegistry.
> > 
> > Done.
> 
> And now you don't need
> InspectorAgentRegistry& agents() { return m_agents; }

Sorry, escaped my mind.

https://codereview.chromium.org/307943002/diff/300001/Source/core/inspector/W...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/300001/Source/core/inspector/W...
Source/core/inspector/WorkerDebuggerAgent.cpp:87: if (!workerGlobalScope)
On 2014/06/19 07:37:03, yurys wrote:
> This method is called on the main thread while WorkerGlobalScope is created
and
> destroyed on the worker thread. It may well happen now that WorkerGlobalScope
is
> getting destroyed while we are using it in this method. Is there a reason we
> cannot leave previous implementation that used a map guarded by a mutex?

I was able to make the map solution work in oilpan in patch set 11.
https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...

There are limitations related to threads with oilpan's handles (discussed in
previous comments) and that made the type of the map quite complicated. I was
trying if there was a simpler way.

In the new patch I've added a pointer to WebThread and added a mutex so we don't
destruct and call this function at the same time. 

I could go back to the map if you'd like.

[yurys, 2014-06-19 13:23:55]

On 2014/06/19 12:52:07, keishi wrote:
> I was able to make the map solution work in oilpan in patch set 11.
>
https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
> 
> There are limitations related to threads with oilpan's handles (discussed in
> previous comments) and that made the type of the map quite complicated. I was
> trying if there was a simpler way.
> 
> In the new patch I've added a pointer to WebThread and added a mutex so we
don't
> destruct and call this function at the same time. 
> 
> I could go back to the map if you'd like.

I'd rather keep existing implementation that uses a static map as it makes it
clear that WorkerDebuggerAgent we access is alive. At first glance
WorkerDebuggerAgents map seems to be used in a way similar to ContextMap  in
DatabaseManager that Mads pointed out in
https://codereview.chromium.org/307943002/#msg10 so I believe we can change its
type to something similar.

https://codereview.chromium.org/307943002/diff/320001/Source/core/inspector/W...
File Source/core/inspector/WorkerDebuggerAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/320001/Source/core/inspector/W...
Source/core/inspector/WorkerDebuggerAgent.cpp:76: MutexLocker locker(m_mutex);
I don't think it does what you expect it to do. The locker will be destroyed
before field destructors are called.

[haraken, 2014-06-19 13:34:42]

>
https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
> > 
> > There are limitations related to threads with oilpan's handles (discussed in
> > previous comments) and that made the type of the map quite complicated. I
was
> > trying if there was a simpler way.
> > 
> > In the new patch I've added a pointer to WebThread and added a mutex so we
> don't
> > destruct and call this function at the same time. 
> > 
> > I could go back to the map if you'd like.
> 
> I'd rather keep existing implementation that uses a static map as it makes it
> clear that WorkerDebuggerAgent we access is alive. At first glance
> WorkerDebuggerAgents map seems to be used in a way similar to ContextMap  in
> DatabaseManager that Mads pointed out in
> https://codereview.chromium.org/307943002/#msg10 so I believe we can change
its
> type to something similar.

I'd prefer the current CL (i.e., not having the map).

As discussed in the previous comments, there are limitations related to threads
with oilpan handles and thus the map that involves oilpan handles touched by
multiple threads is complicated.

The map in the DatabaseManager.h is a hack and we actually want to avoid using
the map. However, in case of DatabaseManager.h, there is no alternative way to
make it workable, so we cannot avoid using the map. On the other hand, in case
of WorkerDebuugerAgent, there is a simpler way to avoid the map, so we want to
go the simpler way.

[yurys, 2014-06-19 14:14:47]

https://codereview.chromium.org/307943002/diff/320001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/307943002/diff/320001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:109:
OwnPtrWillBePersistent<WorkerDebuggerAgent> m_workerDebuggerAgent;
The agent should be owned by WorkerInspectorController as it is a part of
inspector. Also I don't see this field to be initialized.

[yurys, 2014-06-19 14:18:57]

On 2014/06/19 13:34:42, haraken wrote:
> >
>
https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
> > > 
> > > There are limitations related to threads with oilpan's handles (discussed
in
> > > previous comments) and that made the type of the map quite complicated. I
> was
> > > trying if there was a simpler way.
> > > 
> > > In the new patch I've added a pointer to WebThread and added a mutex so we
> > don't
> > > destruct and call this function at the same time. 
> > > 
> > > I could go back to the map if you'd like.
> > 
> > I'd rather keep existing implementation that uses a static map as it makes
it
> > clear that WorkerDebuggerAgent we access is alive. At first glance
> > WorkerDebuggerAgents map seems to be used in a way similar to ContextMap  in
> > DatabaseManager that Mads pointed out in
> > https://codereview.chromium.org/307943002/#msg10 so I believe we can change
> its
> > type to something similar.
> 
> I'd prefer the current CL (i.e., not having the map).
> 
> As discussed in the previous comments, there are limitations related to
threads
> with oilpan handles and thus the map that involves oilpan handles touched by
> multiple threads is complicated.
> 
> The map in the DatabaseManager.h is a hack and we actually want to avoid using
> the map. However, in case of DatabaseManager.h, there is no alternative way to
> make it workable, so we cannot avoid using the map. On the other hand, in case
> of WorkerDebuugerAgent, there is a simpler way to avoid the map, so we want to
> go the simpler way.

OK, if there is no other way to avoid this we could have pointer to the debugger
agent right on WorkerThread but all accesses to it should be guarded by a mutex
sored on WorkerThread as well so that its lifetime matches that of the thread.

[keishi, 2014-06-23 13:10:35]

On 2014/06/19 14:18:57, yurys wrote:
> On 2014/06/19 13:34:42, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/307943002/diff/220001/Source/core/inspector/W...
> > > > 
> > > > There are limitations related to threads with oilpan's handles
(discussed
> in
> > > > previous comments) and that made the type of the map quite complicated.
I
> > was
> > > > trying if there was a simpler way.
> > > > 
> > > > In the new patch I've added a pointer to WebThread and added a mutex so
we
> > > don't
> > > > destruct and call this function at the same time. 
> > > > 
> > > > I could go back to the map if you'd like.
> > > 
> > > I'd rather keep existing implementation that uses a static map as it makes
> it
> > > clear that WorkerDebuggerAgent we access is alive. At first glance
> > > WorkerDebuggerAgents map seems to be used in a way similar to ContextMap 
in
> > > DatabaseManager that Mads pointed out in
> > > https://codereview.chromium.org/307943002/#msg10 so I believe we can
change
> > its
> > > type to something similar.
> > 
> > I'd prefer the current CL (i.e., not having the map).
> > 
> > As discussed in the previous comments, there are limitations related to
> threads
> > with oilpan handles and thus the map that involves oilpan handles touched by
> > multiple threads is complicated.
> > 
> > The map in the DatabaseManager.h is a hack and we actually want to avoid
using
> > the map. However, in case of DatabaseManager.h, there is no alternative way
to
> > make it workable, so we cannot avoid using the map. On the other hand, in
case
> > of WorkerDebuugerAgent, there is a simpler way to avoid the map, so we want
to
> > go the simpler way.
> 
> OK, if there is no other way to avoid this we could have pointer to the
debugger
> agent right on WorkerThread but all accesses to it should be guarded by a
mutex
> sored on WorkerThread as well so that its lifetime matches that of the thread.

Will this do? WorkerDebuggerAgent::dispose is always called before destruction
of WorkerDebuggerAgent and WorkerGlobalScope so it sets a flag so that
interruptAndDispatchInspectorCommands will not access the worker thread.

[yurys, 2014-06-24 08:49:52]

On 2014/06/23 13:10:35, keishi wrote:
> Will this do? WorkerDebuggerAgent::dispose is always called before destruction
> of WorkerDebuggerAgent and WorkerGlobalScope so it sets a flag so that
> interruptAndDispatchInspectorCommands will not access the worker thread.

I don't think this will work. See my comments for changes in WorkerThread.cpp

https://codereview.chromium.org/307943002/diff/340001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/307943002/diff/340001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:109: if (m_workerDebuggerAgent)
m_workerDebuggerAgent is never initialized as I pointed out above, so this
method does nothing. I believe we will need a test for debug break functionality
when worker is in a tight loop to make sure it works after this change.

Also m_workerDebuggerAgent is modified on the worker thread while
interruptAndDispatchInspectorCommands is called on the main thread so this code
is not thread-safe, e.g. m_workerDebuggerAgent may well become null right after
the check.

[keishi, 2014-06-24 14:53:30]

https://codereview.chromium.org/307943002/diff/340001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/307943002/diff/340001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:109: if (m_workerDebuggerAgent)
On 2014/06/24 08:49:52, yurys wrote:
> m_workerDebuggerAgent is never initialized as I pointed out above, so this
> method does nothing. I believe we will need a test for debug break
functionality
> when worker is in a tight loop to make sure it works after this change.
> 
> Also m_workerDebuggerAgent is modified on the worker thread while
> interruptAndDispatchInspectorCommands is called on the main thread so this
code
> is not thread-safe, e.g. m_workerDebuggerAgent may well become null right
after
> the check.

I added a mutex to WorkerThread so that
WorkerThread::interruptAndDispatchInspectorCommands will not run while the
thread is being shutdown in the worker thread.

I'm not sure how to test this.

[yurys, 2014-06-25 05:51:57]

https://codereview.chromium.org/307943002/diff/340001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/307943002/diff/340001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:109: if (m_workerDebuggerAgent)
On 2014/06/24 14:53:29, keishi wrote:
> On 2014/06/24 08:49:52, yurys wrote:
> > m_workerDebuggerAgent is never initialized as I pointed out above, so this
> > method does nothing. I believe we will need a test for debug break
> functionality
> > when worker is in a tight loop to make sure it works after this change.
> > 
> > Also m_workerDebuggerAgent is modified on the worker thread while
> > interruptAndDispatchInspectorCommands is called on the main thread so this
> code
> > is not thread-safe, e.g. m_workerDebuggerAgent may well become null right
> after
> > the check.
> 
> I added a mutex to WorkerThread so that
> WorkerThread::interruptAndDispatchInspectorCommands will not run while the
> thread is being shutdown in the worker thread.
> 
> I'm not sure how to test this.

The simplest way would be to add a layout test based on remote debugging
protocol, see e.g.
LayoutTests/inspector-protocol/debugger/debugger-pause-dedicated-worker.html

[yurys, 2014-06-25 06:11:23]

https://codereview.chromium.org/307943002/diff/360001/Source/core/inspector/W...
File Source/core/inspector/WorkerInspectorController.cpp (right):

https://codereview.chromium.org/307943002/diff/360001/Source/core/inspector/W...
Source/core/inspector/WorkerInspectorController.cpp:171: m_workerDebuggerAgent =
nullptr;
You don't need to clear m_workerDebuggerAgent as you clear reference to
WorkerInspectorController owning it (without Oilpan this code would be called
from the destructor only).

https://codereview.chromium.org/307943002/diff/360001/Source/core/inspector/W...
File Source/core/inspector/WorkerInspectorController.h (right):

https://codereview.chromium.org/307943002/diff/360001/Source/core/inspector/W...
Source/core/inspector/WorkerInspectorController.h:68: WorkerDebuggerAgent*
workerDebuggerAgent() { return m_workerDebuggerAgent.get(); }
Let's not expose whole agent but instead have only
interruptAndDispatchInspectorCommands method on WorkerInspectorController that
would call the agent internally.

https://codereview.chromium.org/307943002/diff/360001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/307943002/diff/360001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:115: if (WorkerDebuggerAgent* debuggerAgent
= inspectorController->workerDebuggerAgent())
This check should always be true as the agent is cleared only when disposing of
WorkerInspectorController.

[yurys, 2014-06-25 07:50:27]

https://codereview.chromium.org/307943002/diff/340001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/307943002/diff/340001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:109: if (m_workerDebuggerAgent)
On 2014/06/24 14:53:29, keishi wrote:
> I'm not sure how to test this.

Never mind, I'm adding a new test for this:
https://codereview.chromium.org/352103002/

[keishi, 2014-06-26 01:44:26]

> Never mind, I'm adding a new test for this:
> https://codereview.chromium.org/352103002/

Thanks!!

https://codereview.chromium.org/307943002/diff/360001/Source/core/inspector/W...
File Source/core/inspector/WorkerInspectorController.cpp (right):

https://codereview.chromium.org/307943002/diff/360001/Source/core/inspector/W...
Source/core/inspector/WorkerInspectorController.cpp:171: m_workerDebuggerAgent =
nullptr;
On 2014/06/25 06:11:23, yurys wrote:
> You don't need to clear m_workerDebuggerAgent as you clear reference to
> WorkerInspectorController owning it (without Oilpan this code would be called
> from the destructor only).

Done.

https://codereview.chromium.org/307943002/diff/360001/Source/core/inspector/W...
File Source/core/inspector/WorkerInspectorController.h (right):

https://codereview.chromium.org/307943002/diff/360001/Source/core/inspector/W...
Source/core/inspector/WorkerInspectorController.h:68: WorkerDebuggerAgent*
workerDebuggerAgent() { return m_workerDebuggerAgent.get(); }
On 2014/06/25 06:11:23, yurys wrote:
> Let's not expose whole agent but instead have only
> interruptAndDispatchInspectorCommands method on WorkerInspectorController that
> would call the agent internally.

Done.

[yurys, 2014-06-26 11:04:30]

https://codereview.chromium.org/307943002/diff/380001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/307943002/diff/380001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:112: if (WorkerInspectorController*
inspectorController = m_workerGlobalScope->workerInspectorController())
WorkerGlobalScope::clearInspector may be executing on the worker thread in
parallel and you may end up with non-null inspectorController instance here
which will be destroyed immediately after that on the worker thread resulting in
use after free.

Having said that, I don't think this may happen with dedicated workers as
clearInspector is called in the following two places:
1) From WorkerGlobalScope::dispose, which is called with the mutex locked
2) From WorkerThreadShutdownFinishTask::performTask where you don't lock the
mutex but the task is always a result of WorkerThread::stop, which in turn is
only invoked from WorkerMessagingProxy when m_askedToTerminate is true. If
m_askedToTerminate is true we will bail out from
WorkerMessagingProxy::sendMessageToInspector before calling
interruptAndDispatchInspectorCommands. The latter is not true though when
interruptAndDispatchInspectorCommands is invoked from
WebEmbeddedWorkerImpl::dispatchDevToolsMessage without checking
m_askedToTerminate. 

All of that seems very fragile so I believe we need to guard
WorkerInspectorController destruction inside WorkerGlobalObject::clearInspector
with the same mutex as we do when disposing m_workerGlobalScope.


+horo who may have additional input regarding WebEmbeddedWorkerImpl

[horo, 2014-06-27 06:36:06]

https://codereview.chromium.org/307943002/diff/380001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/307943002/diff/380001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:112: if (WorkerInspectorController*
inspectorController = m_workerGlobalScope->workerInspectorController())
On 2014/06/26 11:04:30, yurys wrote:
> WorkerGlobalScope::clearInspector may be executing on the worker thread in
> parallel and you may end up with non-null inspectorController instance here
> which will be destroyed immediately after that on the worker thread resulting
in
> use after free.
> 
> Having said that, I don't think this may happen with dedicated workers as
> clearInspector is called in the following two places:
> 1) From WorkerGlobalScope::dispose, which is called with the mutex locked
> 2) From WorkerThreadShutdownFinishTask::performTask where you don't lock the
> mutex but the task is always a result of WorkerThread::stop, which in turn is
> only invoked from WorkerMessagingProxy when m_askedToTerminate is true. If
> m_askedToTerminate is true we will bail out from
> WorkerMessagingProxy::sendMessageToInspector before calling
> interruptAndDispatchInspectorCommands. The latter is not true though when
> interruptAndDispatchInspectorCommands is invoked from
> WebEmbeddedWorkerImpl::dispatchDevToolsMessage without checking
> m_askedToTerminate. 
> 
> All of that seems very fragile so I believe we need to guard
> WorkerInspectorController destruction inside
WorkerGlobalObject::clearInspector
> with the same mutex as we do when disposing m_workerGlobalScope.
> 
> 
> +horo who may have additional input regarding WebEmbeddedWorkerImpl

I agree with yurys.

But it may be good to add "if (m_askedToTerminate) return;" in
WebEmbeddedWorkerImpl::dispatchDevToolsMessage and
WebSharedWorkerImpl::dispatchDevToolsMessage.

[keishi, 2014-06-30 08:30:45]

On 2014/06/26 11:04:30, yurys_ooo wrote:
>
https://codereview.chromium.org/307943002/diff/380001/Source/core/workers/Wor...
> File Source/core/workers/WorkerThread.cpp (right):
> 
>
https://codereview.chromium.org/307943002/diff/380001/Source/core/workers/Wor...
> Source/core/workers/WorkerThread.cpp:112: if (WorkerInspectorController*
> inspectorController = m_workerGlobalScope->workerInspectorController())
> WorkerGlobalScope::clearInspector may be executing on the worker thread in
> parallel and you may end up with non-null inspectorController instance here
> which will be destroyed immediately after that on the worker thread resulting
in
> use after free.
> 
> Having said that, I don't think this may happen with dedicated workers as
> clearInspector is called in the following two places:
> 1) From WorkerGlobalScope::dispose, which is called with the mutex locked
> 2) From WorkerThreadShutdownFinishTask::performTask where you don't lock the
> mutex but the task is always a result of WorkerThread::stop, which in turn is
> only invoked from WorkerMessagingProxy when m_askedToTerminate is true. If
> m_askedToTerminate is true we will bail out from
> WorkerMessagingProxy::sendMessageToInspector before calling
> interruptAndDispatchInspectorCommands. The latter is not true though when
> interruptAndDispatchInspectorCommands is invoked from
> WebEmbeddedWorkerImpl::dispatchDevToolsMessage without checking
> m_askedToTerminate. 
> 
> All of that seems very fragile so I believe we need to guard
> WorkerInspectorController destruction inside
WorkerGlobalObject::clearInspector
> with the same mutex as we do when disposing m_workerGlobalScope.
> 
> 
> +horo who may have additional input regarding WebEmbeddedWorkerImpl

Moved mutex to WorkerGlobalScope and guarded clearInspector.

> But it may be good to add "if (m_askedToTerminate) return;" in
> WebEmbeddedWorkerImpl::dispatchDevToolsMessage and
> WebSharedWorkerImpl::dispatchDevToolsMessage.

Added checks.

[yurys, 2014-07-01 05:57:52]

On 2014/06/30 08:30:45, keishi wrote:
> 
> Moved mutex to WorkerGlobalScope and guarded clearInspector.
> 
But this returns us back to the problem that m_workerGlobalScope may be cleared
on the worker thread after it has been checked on the main thread in
WorkerThread::interruptAndDispatchInspectorCommands. Also exposing
workerInspectorControllerMutex() on WorkerGlobalScope looks a bit odd, I believe
we should leave it on WorkerThread and have it locked when there is a chance
that WorkerInspectorController may be destroyed. I still think that simplest way
to achieve this would be to have something like

WorkerThread::setWorkerInspectorController(WorkerInspectorController* c)
{
    MutexLocker locker(m_workerInspectorControllerMutex);
    m_workerInspectorController = c;
}

And call it from WorkerGlobalScope constructor and clearInspector or even from
WorkerInspectorController's constructor and dispose. Interrupt method would be
quite straightforward and easy to reason about its thread-safety:

void WorkerThread::interruptAndDispatchInspectorCommands()
{
     MutexLocker locker(m_workerInspectorControllerMutex);
     if (m_workerInspectorController)
          m_workerInspectorController->interruptAndDispatchInspectorCommands();
}

Does it makes?

[keishi, 2014-07-01 08:44:18]

On 2014/07/01 05:57:52, yurys_ooo wrote:
> On 2014/06/30 08:30:45, keishi wrote:
> > 
> > Moved mutex to WorkerGlobalScope and guarded clearInspector.
> > 
> But this returns us back to the problem that m_workerGlobalScope may be
cleared
> on the worker thread after it has been checked on the main thread in
> WorkerThread::interruptAndDispatchInspectorCommands. Also exposing
> workerInspectorControllerMutex() on WorkerGlobalScope looks a bit odd, I
believe
> we should leave it on WorkerThread and have it locked when there is a chance
> that WorkerInspectorController may be destroyed. I still think that simplest
way
> to achieve this would be to have something like
> 
> WorkerThread::setWorkerInspectorController(WorkerInspectorController* c)
> {
>     MutexLocker locker(m_workerInspectorControllerMutex);
>     m_workerInspectorController = c;
> }
> 
> And call it from WorkerGlobalScope constructor and clearInspector or even from
> WorkerInspectorController's constructor and dispose. Interrupt method would be
> quite straightforward and easy to reason about its thread-safety:
> 
> void WorkerThread::interruptAndDispatchInspectorCommands()
> {
>      MutexLocker locker(m_workerInspectorControllerMutex);
>      if (m_workerInspectorController)
>          
m_workerInspectorController->interruptAndDispatchInspectorCommands();
> }
> 
> Does it makes?

Done, added m_workerInspectorControllerMutex to WorkerThread. I changed the
WorkerInspectorController to RefCounted because we don't have
RawPtrWillBePersistent to avoid memory leaks.

[yurys, 2014-07-03 03:25:54]

On 2014/07/01 08:44:18, keishi wrote:
> Done, added m_workerInspectorControllerMutex to WorkerThread. I changed the
> WorkerInspectorController to RefCounted because we don't have
> RawPtrWillBePersistent to avoid memory leaks.

As far as I understand it should be RawPtrWillBeMember similar to
WorkerInspectorController::m_workerGlobalScope where we store a raw pointer to
the object owned by WorkerThread.

https://codereview.chromium.org/307943002/diff/420001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/307943002/diff/420001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:192:
thread()->setWorkerInspectorController(nullptr);
The reference stored in WorkerThread should be cleared before
WorkerInspectorController::dispose.

https://codereview.chromium.org/307943002/diff/420001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/307943002/diff/420001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:50: class WorkerDebuggerAgent;
You don't need this forward declaration anymore.

https://codereview.chromium.org/307943002/diff/420001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:80: void
setWorkerInspectorController(PassRefPtrWillBeRawPtr<WorkerInspectorController>);
The argument can already be a raw pointer if it works fine with Oilpan types.

[keishi, 2014-07-03 23:53:54]

> As far as I understand it should be RawPtrWillBeMember similar to
> WorkerInspectorController::m_workerGlobalScope where we store a raw pointer to
> the object owned by WorkerThread.

We can't use RawPtrWillBe_Member_ for WorkerThread::m_workerInspectorController
because WorkerThread is not GarbageCollected. WorkerThread and WorkerGlobalScope
are RefCounted when oilpan is enabled so using RawPtrWillBePersistent could
cause a leak.

In this patch set I moved ownership of the WorkerInspectorController to
WorkerThread.

My thinking was that if we can't make WorkerThread garbage collected and we need
to keep a pointer to WorkerInspectorController in WorkerThread, we have to use
RefPtrWillBePersistent and OwnPtrWillBePersistent. RefPtrWillBePersistent was
the previous patch set. OwnPtrWillBePersistent is this new one.

https://codereview.chromium.org/307943002/diff/420001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/307943002/diff/420001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:192:
thread()->setWorkerInspectorController(nullptr);
On 2014/07/03 03:25:54, yurys_ooo wrote:
> The reference stored in WorkerThread should be cleared before
> WorkerInspectorController::dispose.

Done.

https://codereview.chromium.org/307943002/diff/420001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/307943002/diff/420001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:50: class WorkerDebuggerAgent;
On 2014/07/03 03:25:54, yurys_ooo wrote:
> You don't need this forward declaration anymore.

Done.

[haraken, 2014-07-04 01:01:45]

On 2014/07/03 23:53:54, keishi wrote:
> > As far as I understand it should be RawPtrWillBeMember similar to
> > WorkerInspectorController::m_workerGlobalScope where we store a raw pointer
to
> > the object owned by WorkerThread.
> 
> We can't use RawPtrWillBe_Member_ for
WorkerThread::m_workerInspectorController
> because WorkerThread is not GarbageCollected. WorkerThread and
WorkerGlobalScope
> are RefCounted when oilpan is enabled so using RawPtrWillBePersistent could
> cause a leak.
> 
> In this patch set I moved ownership of the WorkerInspectorController to
> WorkerThread.
> 
> My thinking was that if we can't make WorkerThread garbage collected and we
need
> to keep a pointer to WorkerInspectorController in WorkerThread, we have to use
> RefPtrWillBePersistent and OwnPtrWillBePersistent. RefPtrWillBePersistent was
> the previous patch set. OwnPtrWillBePersistent is this new one.

yurys@:

Before considering oilpan, in general we want to use an OwnPtr only when the
ownership is very clear (i.e., it's clear that the owned object is owned by only
one owner). Otherwise, we want to use a RefPtr.

In this particular case, the current object relationship looks like this:

(a)
WorkerThread --(RefPtr)--> WorkerGlobalScope --(OwnPtr)-->
WorkerInspectorController

If we want to add a direct reference from WorkerThread to
WorkerInspectorController, it's possible to use a RawPtr for the reference (it's
correct), but we prefer using a RefPtr to make the lifetime relationship
clearer.

(b)
WorkerThread --(RefPtr)--> WorkerGlobalScope --(RefPtr)-->
WorkerInspectorController
|
-------------------------------(RefPtr)-----------------> (the above
WorkerInspectorController)

I'm fine with either of (a) and (b), but in case of (b), I prefer using a
RefPtr. Then the RefPtr will become RefPtrWillBePersistent in oilpan.

(Note: If we have to use RawPtrWillBePersistent, it's a signal that there is a
weird lifetime relationship in the non-oilpan mode. That's why we currently
don't have RawPtrWillBePersistent in the code base.)

[yurys, 2014-07-04 04:13:04]

On 2014/07/03 23:53:54, keishi wrote:
> > As far as I understand it should be RawPtrWillBeMember similar to
> > WorkerInspectorController::m_workerGlobalScope where we store a raw pointer
to
> > the object owned by WorkerThread.
> 
> We can't use RawPtrWillBe_Member_ for
WorkerThread::m_workerInspectorController
> because WorkerThread is not GarbageCollected. WorkerThread and
WorkerGlobalScope
> are RefCounted when oilpan is enabled so using RawPtrWillBePersistent could
> cause a leak.
> 
> In this patch set I moved ownership of the WorkerInspectorController to
> WorkerThread.
> 
> My thinking was that if we can't make WorkerThread garbage collected and we
need
> to keep a pointer to WorkerInspectorController in WorkerThread, we have to use
> RefPtrWillBePersistent and OwnPtrWillBePersistent. RefPtrWillBePersistent was
> the previous patch set. OwnPtrWillBePersistent is this new one.
> 
Thanks for explaining. I missed the fact that WorkerThread is not going to be
garbage collected and we cannot use RawPtrWillBeMember as its member types. In
that case I'm fine with making WorkerInspectorController ref-counted.  Patch set
#21 with the minor comments addressed - LGTM

[yurys, 2014-07-04 04:48:21]

On 2014/07/04 01:01:45, haraken wrote:
> 
> yurys@:
> 
> Before considering oilpan, in general we want to use an OwnPtr only when the
> ownership is very clear (i.e., it's clear that the owned object is owned by
only
> one owner). Otherwise, we want to use a RefPtr.
>
I believe ownership model of WorkerInspectorController is very clear: it exists
as long as containing WorkerGlobalScope exists and is owned by it. For better
debugging experience we need to trigger JS pause on the parallel thread which
could be accomplished by following
WorkerThread->WorkerGlobalScope->WorkerInspectorController->WorkerDebuggerAgent->WorkerScriptDebugServer
chain but that proves cumbersome and error-prone when thread-safety comes into
play. To overcome this we could store a pointer to WorkerInspectorController
right on WorkerThread and make sure that it is not null iff
WorkerScriptDebugServer is alive and usable. In C++ it would be a raw pointer
(which could become e.g. a weak handle). It turns out there is no matching
Oilpan transition type so we have to make some objects ref-counted just for the
sake of having appropriate transition types. When I see a raw pointer field it
it clear that the object is owned by someone else and I need to be careful about
keeping the pointer valid, on the other hand the ownership model is less clear
with RefPtrs.
 

> In this particular case, the current object relationship looks like this:
> 
> (a)
> WorkerThread --(RefPtr)--> WorkerGlobalScope --(OwnPtr)-->
> WorkerInspectorController
> 
> If we want to add a direct reference from WorkerThread to
> WorkerInspectorController, it's possible to use a RawPtr for the reference
(it's
> correct), but we prefer using a RefPtr to make the lifetime relationship
> clearer.
> 
> (b)
> WorkerThread --(RefPtr)--> WorkerGlobalScope --(RefPtr)-->
> WorkerInspectorController
> |
> -------------------------------(RefPtr)-----------------> (the above
> WorkerInspectorController)
> 
> I'm fine with either of (a) and (b), but in case of (b), I prefer using a
> RefPtr. Then the RefPtr will become RefPtrWillBePersistent in oilpan.
>
Yeah, if WorkerThread is not going to be garbage collected I'm fine with (b).

> (Note: If we have to use RawPtrWillBePersistent, it's a signal that there is a
> weird lifetime relationship in the non-oilpan mode. That's why we currently
> don't have RawPtrWillBePersistent in the code base.)
If the class with such member were to be garbage collected the member would
likely become RawPtrWillBeMember. Should it be RawPtrWillBeWeak or something?

[haraken, 2014-07-04 04:52:32]

On 2014/07/04 04:48:21, yurys_ooo wrote:
> On 2014/07/04 01:01:45, haraken wrote:
> > 
> > yurys@:
> > 
> > Before considering oilpan, in general we want to use an OwnPtr only when the
> > ownership is very clear (i.e., it's clear that the owned object is owned by
> only
> > one owner). Otherwise, we want to use a RefPtr.
> >
> I believe ownership model of WorkerInspectorController is very clear: it
exists
> as long as containing WorkerGlobalScope exists and is owned by it. For better
> debugging experience we need to trigger JS pause on the parallel thread which
> could be accomplished by following
>
WorkerThread->WorkerGlobalScope->WorkerInspectorController->WorkerDebuggerAgent->WorkerScriptDebugServer
> chain but that proves cumbersome and error-prone when thread-safety comes into
> play. To overcome this we could store a pointer to WorkerInspectorController
> right on WorkerThread and make sure that it is not null iff
> WorkerScriptDebugServer is alive and usable. In C++ it would be a raw pointer
> (which could become e.g. a weak handle). It turns out there is no matching
> Oilpan transition type so we have to make some objects ref-counted just for
the
> sake of having appropriate transition types. When I see a raw pointer field it
> it clear that the object is owned by someone else and I need to be careful
about
> keeping the pointer valid, on the other hand the ownership model is less clear
> with RefPtrs.
>  
> 
> > In this particular case, the current object relationship looks like this:
> > 
> > (a)
> > WorkerThread --(RefPtr)--> WorkerGlobalScope --(OwnPtr)-->
> > WorkerInspectorController
> > 
> > If we want to add a direct reference from WorkerThread to
> > WorkerInspectorController, it's possible to use a RawPtr for the reference
> (it's
> > correct), but we prefer using a RefPtr to make the lifetime relationship
> > clearer.
> > 
> > (b)
> > WorkerThread --(RefPtr)--> WorkerGlobalScope --(RefPtr)-->
> > WorkerInspectorController
> > |
> > -------------------------------(RefPtr)-----------------> (the above
> > WorkerInspectorController)
> > 
> > I'm fine with either of (a) and (b), but in case of (b), I prefer using a
> > RefPtr. Then the RefPtr will become RefPtrWillBePersistent in oilpan.
> >
> Yeah, if WorkerThread is not going to be garbage collected I'm fine with (b).
> 
> > (Note: If we have to use RawPtrWillBePersistent, it's a signal that there is
a
> > weird lifetime relationship in the non-oilpan mode. That's why we currently
> > don't have RawPtrWillBePersistent in the code base.)
> If the class with such member were to be garbage collected the member would
> likely become RawPtrWillBeMember. Should it be RawPtrWillBeWeak or something?

Yeah, if WorkerThread is on-heap, we can just use RawPtrWillBeMember.

[yurys, 2014-07-07 09:37:33]

On 2014/07/04 04:52:32, haraken wrote:
> > > (Note: If we have to use RawPtrWillBePersistent, it's a signal that there
is
> a
> > > weird lifetime relationship in the non-oilpan mode. That's why we
currently
> > > don't have RawPtrWillBePersistent in the code base.)
> > If the class with such member were to be garbage collected the member would
> > likely become RawPtrWillBeMember. Should it be RawPtrWillBeWeak or
something?
> 
> Yeah, if WorkerThread is on-heap, we can just use RawPtrWillBeMember.

Actually I meant that provided WorkerThread is _not_ on the Oilpan heap, the raw
pointer to WorkerInspectorController could be replaced with RawPtrWillBe_Weak_.
I.e. it should become a non-retaining handle to the object. Have you considered
this as an alternative to making an object ref-counted in order to allow using
RawPtrWillBePersistent?

[haraken, 2014-07-07 09:45:02]

On 2014/07/07 09:37:33, yurys wrote:
> On 2014/07/04 04:52:32, haraken wrote:
> > > > (Note: If we have to use RawPtrWillBePersistent, it's a signal that
there
> is
> > a
> > > > weird lifetime relationship in the non-oilpan mode. That's why we
> currently
> > > > don't have RawPtrWillBePersistent in the code base.)
> > > If the class with such member were to be garbage collected the member
would
> > > likely become RawPtrWillBeMember. Should it be RawPtrWillBeWeak or
> something?
> > 
> > Yeah, if WorkerThread is on-heap, we can just use RawPtrWillBeMember.
> 
> Actually I meant that provided WorkerThread is _not_ on the Oilpan heap, the
raw
> pointer to WorkerInspectorController could be replaced with
RawPtrWillBe_Weak_.
> I.e. it should become a non-retaining handle to the object. Have you
considered
> this as an alternative to making an object ref-counted in order to allow using
> RawPtrWillBePersistent?

Not really. In oilpan, we don't want to use weak pointers as much as possible
(because weak pointers are heavy in terms of both performance and memory). The
policy is that we use weak pointers only if we need weak semantics (i.e., the
owner object of the weak pointer can outlive the object pointed to by the weak
pointer). In this particular case, WorkerThread cannot outlive
WorkerInspectorController, so we don't want to use weak pointers here.

Also we anyway cannot use RawPtrWillBeWeakMember in WorkerThread until we move
WorkerThread to oilpan's heap, because only on-heap objects can have
WeakMembers.

[keishi, 2014-07-08 04:30:50]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-07-08 04:31:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/307943002/480001

[commit-bot: I haz the power, 2014-07-08 05:09:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  android_chromium_gn_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/9546)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/14544)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15815)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/25752)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/30202)

[commit-bot: I haz the power, 2014-07-08 05:10:50]

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

[commit-bot: I haz the power, 2014-07-08 05:10:54]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/25756)

[keishi, 2014-07-11 00:46:57]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-07-11 00:47:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/307943002/500001

[commit-bot: I haz the power, 2014-07-11 00:48:33]

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

[commit-bot: I haz the power, 2014-07-11 00:48:36]

Failed to apply patch for Source/core/workers/WorkerMessagingProxy.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/workers/WorkerMessagingProxy.cpp
  Hunk #1 FAILED at 250.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/workers/WorkerMessagingProxy.cpp.rej

Patch:       Source/core/workers/WorkerMessagingProxy.cpp
Index: Source/core/workers/WorkerMessagingProxy.cpp
diff --git a/Source/core/workers/WorkerMessagingProxy.cpp
b/Source/core/workers/WorkerMessagingProxy.cpp
index
6abc7aa627d4072fad61ada8b5a6fe9488e9e65b..e045bd194b3b297e3c2d0a2e595cad8a37b0558c
100644
--- a/Source/core/workers/WorkerMessagingProxy.cpp
+++ b/Source/core/workers/WorkerMessagingProxy.cpp
@@ -250,7 +250,7 @@ void WorkerMessagingProxy::sendMessageToInspector(const
String& message)
     if (m_askedToTerminate)
         return;
    
m_workerThread->runLoop().postDebuggerTask(createCallbackTask(dispatchOnInspectorBackendTask,
String(message)));
-   
WorkerDebuggerAgent::interruptAndDispatchInspectorCommands(m_workerThread.get());
+    m_workerThread->interruptAndDispatchInspectorCommands();
 }
 
 void WorkerMessagingProxy::workerGlobalScopeDestroyed()

[keishi, 2014-07-11 02:13:42]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-07-11 02:14:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/307943002/520001

[commit-bot: I haz the power, 2014-07-11 03:23:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16364)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/31781)

[commit-bot: I haz the power, 2014-07-11 03:39:02]

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

[commit-bot: I haz the power, 2014-07-11 03:39:05]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16374)

[keishi, 2014-07-14 12:08:24]

msvc gives the C4407 warning for the cast inside
TraceEventDispatcher::addListener because I had to change the inheritance order
of InspectorTimelineAgent
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...
http://msdn.microsoft.com/en-us/library/1s6193tt.aspx

I couldn't get method function pointer casting to work so I added the
TraceEventListener class that handles the polymorphism instead of casting the
method function pointer

yurys@ haraken@ PTAL.

[haraken, 2014-07-14 12:33:40]

LGTM

https://codereview.chromium.org/307943002/diff/550001/Source/core/inspector/I...
File Source/core/inspector/InspectorTimelineAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/550001/Source/core/inspector/I...
Source/core/inspector/InspectorTimelineAgent.cpp:361:
dispatcher->addListener(InstrumentationEvents::BeginFrame,
TRACE_EVENT_PHASE_INSTANT, adoptPtrWillBeNoop(new
InspectorTimelineAgent::TraceEventListener(this,
&InspectorTimelineAgent::onBeginImplSideFrame)), m_client);

Shall we add a create() method?

https://codereview.chromium.org/307943002/diff/550001/Source/core/inspector/I...
File Source/core/inspector/InspectorTimelineAgent.h (right):

https://codereview.chromium.org/307943002/diff/550001/Source/core/inspector/I...
Source/core/inspector/InspectorTimelineAgent.h:116: class TraceEventListener :
public TraceEventDispatcher::TraceEventListener {

Can we rename this to TimelineAgentTraceEventListener? It's a bit confusing to
have two classes that have the same name.

[keishi, 2014-07-15 05:52:53]

https://codereview.chromium.org/307943002/diff/550001/Source/core/inspector/I...
File Source/core/inspector/InspectorTimelineAgent.cpp (right):

https://codereview.chromium.org/307943002/diff/550001/Source/core/inspector/I...
Source/core/inspector/InspectorTimelineAgent.cpp:361:
dispatcher->addListener(InstrumentationEvents::BeginFrame,
TRACE_EVENT_PHASE_INSTANT, adoptPtrWillBeNoop(new
InspectorTimelineAgent::TraceEventListener(this,
&InspectorTimelineAgent::onBeginImplSideFrame)), m_client);
On 2014/07/14 12:33:39, haraken wrote:
> 
> Shall we add a create() method?

Done.

https://codereview.chromium.org/307943002/diff/550001/Source/core/inspector/I...
File Source/core/inspector/InspectorTimelineAgent.h (right):

https://codereview.chromium.org/307943002/diff/550001/Source/core/inspector/I...
Source/core/inspector/InspectorTimelineAgent.h:116: class TraceEventListener :
public TraceEventDispatcher::TraceEventListener {
On 2014/07/14 12:33:40, haraken wrote:
> 
> Can we rename this to TimelineAgentTraceEventListener? It's a bit confusing to
> have two classes that have the same name.

Done. Changed to InspectorTimelineAgentTraceEventListener

[keishi, 2014-07-17 02:00:01]

yurrys@ could you take a look and see if the change to TraceEventDispatcher is
alright? Thanks.

[haraken, 2014-07-24 02:03:04]

yurys@: friendly ping. We'd like to get the patch in :)

[yurys, 2014-07-24 07:11:39]

@caseq: you may want to take a look at TraceEventDispatcher changes.

[yurys, 2014-07-24 07:18:55]

TraceEventDispatcher.* changes in PS#32 - lgtm

https://codereview.chromium.org/307943002/diff/630001/Source/core/inspector/I...
File Source/core/inspector/InspectorTimelineAgent.h (right):

https://codereview.chromium.org/307943002/diff/630001/Source/core/inspector/I...
Source/core/inspector/InspectorTimelineAgent.h:324: class
InspectorTimelineAgentTraceEventListener : public
TraceEventDispatcher::TraceEventListener {
This class can be moved into InspectorTimelineAgent.cpp

[keishi, 2014-07-24 07:32:39]

Thanks!

https://codereview.chromium.org/307943002/diff/630001/Source/core/inspector/I...
File Source/core/inspector/InspectorTimelineAgent.h (right):

https://codereview.chromium.org/307943002/diff/630001/Source/core/inspector/I...
Source/core/inspector/InspectorTimelineAgent.h:324: class
InspectorTimelineAgentTraceEventListener : public
TraceEventDispatcher::TraceEventListener {
On 2014/07/24 07:18:55, yurys wrote:
> This class can be moved into InspectorTimelineAgent.cpp

Done.

[keishi, 2014-07-25 00:34:44]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-07-25 00:35:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/307943002/650001

[commit-bot: I haz the power, 2014-07-25 01:24:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/17015)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/18846)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/33077)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/38111)

[commit-bot: I haz the power, 2014-07-25 01:30:43]

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

[commit-bot: I haz the power, 2014-07-25 01:30:46]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[keishi, 2014-07-25 02:21:02]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-07-25 02:21:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/307943002/670001

[commit-bot: I haz the power, 2014-07-25 03:32:25]

Change committed as 178895