Oilpan: Remove RefPtrs to Node and its subclasses in core/inspector/ with Oilpan transition types.

Oilpan: Remove RefPtrs to Node and its subclasses in core/inspector/ with Oilpan transition types.

InspectorDOMAgent::m_documentNodeToIdMap needs to be switched from a part object
to OwnPtr/Persistent because we have some addres comparisons of
NodeToIdMap(HeapHashMap) and we have no ways to get a HeapHashMap pointer from a
PersistentHeapHashMap.

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

[tkent, 2014-06-06 06:56:57]

Please review this.

[haraken, 2014-06-06 07:17:04]

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDOMAgent.cpp:1044:
WillBeHeapVector<RefPtrWillBeMember<Node> >* resultsIt =
&m_searchResults.add(*searchId, WillBeHeapVector<RefPtrWillBeMember<Node>
>()).storedValue->value;

Doesn't nullptr work?

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorDOMAgent.h (right):

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDOMAgent.h:261:
OwnPtrWillBePersistent<NodeToIdMap> m_documentNodeToIdMap;

I'm curious why you need to change this from NodeToIdMap to
OwnPtrWillBePersistent<NodeToIdMap> ?

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDOMAgent.h:264: WillBePersistentHeapHashMap<int,
RawPtrWillBeMember<Node> > m_idToNode;

I don't fully understand this map. Are you sure that this is not a weak member
but a member?

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDOMAgent.h:265: WillBePersistentHeapHashMap<int,
RawPtrWillBeMember<NodeToIdMap> > m_idToNodesMap;

Ditto. I couldn't find code to remove entries from m_idToNodesMap. Not related
to your CL, I wonder why we don't need to remove the entries from m_idToNodesMap
when the InspectorDOMAgent gets unbound (i.e., InspectorDOMAgent::unbind).

[tkent, 2014-06-06 07:59:45]

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDOMAgent.cpp:1044:
WillBeHeapVector<RefPtrWillBeMember<Node> >* resultsIt =
&m_searchResults.add(*searchId, WillBeHeapVector<RefPtrWillBeMember<Node>
>()).storedValue->value;
On 2014/06/06 07:17:04, haraken wrote:
> 
> Doesn't nullptr work?

It's not a pointer.

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorDOMAgent.h (right):

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDOMAgent.h:261:
OwnPtrWillBePersistent<NodeToIdMap> m_documentNodeToIdMap;
On 2014/06/06 07:17:04, haraken wrote:
> 
> I'm curious why you need to change this from NodeToIdMap to
> OwnPtrWillBePersistent<NodeToIdMap> ?

I already explained it in the CL description.

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDOMAgent.h:264: WillBePersistentHeapHashMap<int,
RawPtrWillBeMember<Node> > m_idToNode;
On 2014/06/06 07:17:04, haraken wrote:
> 
> I don't fully understand this map. Are you sure that this is not a weak member
> but a member?

This mapped Node is always in m_documentNodeToIdMap or m_danglingNodeToIdMaps. 
So, Nodes lifetime should not be changed.

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDOMAgent.h:265: WillBePersistentHeapHashMap<int,
RawPtrWillBeMember<NodeToIdMap> > m_idToNodesMap;
On 2014/06/06 07:17:04, haraken wrote:
> 
> Ditto. I couldn't find code to remove entries from m_idToNodesMap. Not related
> to your CL, I wonder why we don't need to remove the entries from
m_idToNodesMap
> when the InspectorDOMAgent gets unbound (i.e., InspectorDOMAgent::unbind).

Hmm, it seems we should clear m_idToNodesMap in
InspectorDOMAgent::discardFrontendBindings.

[yurys, 2014-06-06 08:26:01]

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorDOMAgent.h (right):

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDOMAgent.h:265: WillBePersistentHeapHashMap<int,
RawPtrWillBeMember<NodeToIdMap> > m_idToNodesMap;
On 2014/06/06 07:59:45, tkent wrote:
> On 2014/06/06 07:17:04, haraken wrote:
> > 
> > Ditto. I couldn't find code to remove entries from m_idToNodesMap. Not
related
> > to your CL, I wonder why we don't need to remove the entries from
> m_idToNodesMap
> > when the InspectorDOMAgent gets unbound (i.e., InspectorDOMAgent::unbind).
> 
> Hmm, it seems we should clear m_idToNodesMap in
> InspectorDOMAgent::discardFrontendBindings.

Looks like a bug to me. We should remove corresponding entry in unbind and clear
the map in discardFrontendBindings

[tkent, 2014-06-06 08:26:07]

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorDOMAgent.h (right):

https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDOMAgent.h:265: WillBePersistentHeapHashMap<int,
RawPtrWillBeMember<NodeToIdMap> > m_idToNodesMap;
On 2014/06/06 07:59:45, tkent wrote:
> On 2014/06/06 07:17:04, haraken wrote:
> > 
> > Ditto. I couldn't find code to remove entries from m_idToNodesMap. Not
related
> > to your CL, I wonder why we don't need to remove the entries from
> m_idToNodesMap
> > when the InspectorDOMAgent gets unbound (i.e., InspectorDOMAgent::unbind).
> 
> Hmm, it seems we should clear m_idToNodesMap in
> InspectorDOMAgent::discardFrontendBindings.

Done.

[yurys, 2014-06-06 08:31:12]

On 2014/06/06 08:26:07, tkent wrote:
>
https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
> File Source/core/inspector/InspectorDOMAgent.h (right):
> 
>
https://codereview.chromium.org/315213006/diff/1/Source/core/inspector/Inspec...
> Source/core/inspector/InspectorDOMAgent.h:265:
WillBePersistentHeapHashMap<int,
> RawPtrWillBeMember<NodeToIdMap> > m_idToNodesMap;
> On 2014/06/06 07:59:45, tkent wrote:
> > On 2014/06/06 07:17:04, haraken wrote:
> > > 
> > > Ditto. I couldn't find code to remove entries from m_idToNodesMap. Not
> related
> > > to your CL, I wonder why we don't need to remove the entries from
> > m_idToNodesMap
> > > when the InspectorDOMAgent gets unbound (i.e., InspectorDOMAgent::unbind).
> > 
> > Hmm, it seems we should clear m_idToNodesMap in
> > InspectorDOMAgent::discardFrontendBindings.
> 
> Done.

Can we do this in a separate change please?

[tkent, 2014-06-06 08:31:48]

> Can we do this in a separate change please?

It makes sense.  Will do.

[tkent, 2014-06-06 08:37:13]

On 2014/06/06 08:31:48, tkent wrote:
> > Can we do this in a separate change please?
> 
> It makes sense.  Will do.

https://codereview.chromium.org/319043004


I'll delete Patch Set 2.

[tkent, 2014-06-06 08:43:12]

Now I'm confident that this CL doesn't change Node lifetime.

[haraken, 2014-06-06 08:46:36]

LGTM in terms of oilpan.

[yurys, 2014-06-06 08:51:35]

LGTM. DOM agent will keep Node alive as long as it is bound.

[Mads Ager (chromium), 2014-06-06 08:53:23]

lgtm2

[tkent, 2014-06-06 12:29:47]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-06 12:30:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/315213006/1

[commit-bot: I haz the power, 2014-06-06 12:33:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7140)

[commit-bot: I haz the power, 2014-06-06 12:35:35]

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

[commit-bot: I haz the power, 2014-06-06 12:35:37]

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

[tkent, 2014-06-06 12:46:12]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-06 12:46:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/315213006/1

[commit-bot: I haz the power, 2014-06-06 12:47:21]

Change committed as 175662