Oilpan: move LiveNodeList collections to the heap.

Oilpan: move LiveNodeList collections to the heap.

Move LiveNodeListBase and all collection objects based on it (NodeLists
and HTMLCollections) to the heap.

R=haraken@chromium.org,tkent@chromium.org,ager@chromium.org,erik.corry@gmail.com
BUG=357163
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174377

[sof, 2014-05-11 16:09:23]

Whenever there's next time, please take a look.

A larger-than-wished-for CL, but I don't see a good way to split it up. (Once
you move LiveNodeListBase, the collection objects based on it do follow.)

Points of interest:

 - Maintenance of live collections on Document to handle invalidations. The
Document is now 'in control' (by having weak references) rather than the
collections explicitly unregistering during finalization/destruction.
 - Similarly, the handling of registered NodeLists by NodeListsNodeData using
weak refs. Identical to how the experimental branch did it.

(I'll be away at BlinkOn2 until Thu, btw.)

[haraken, 2014-05-12 07:28:53]

I'll take a look at this CL by the end of today.

[haraken, 2014-05-12 12:04:11]

Here is a first round of comments!

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3717:
m_nodeLists[list->invalidationType()].remove(WeakMember<LiveNodeListBase>(list));

Isn't this weak processing automatically done by oilpan's GC?

In the first place, you're commenting out unregisterNodeList() from
LiveNodeListBase.h (line 62), so this call path shouldn't run when the
LiveNodeListBase is destructed.

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3721: if (list->isRootedAtDocument()) {

I'm just curious but why is this check necessary in non-oilpan builds? Maybe the
fact that this check is necessary implies that simple weak processing is not
enough for m_listsInvalidatedAtDocument? (I'm not sure.)

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3723: m_listsInvalidatedAtDocument.remove(list);

I think you can remove this line from oilpan-builds. The weak processing is
automatically done by oilpan's GC.

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3733: void
Document::decrementNodeListWithIdNameCacheCount(const LiveNodeListBase* list)

I'd rename these methods to {register,unregister}NodeListWithIdNameCache().

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.h:1317: // is currently alive or not for the different
types.

is is => is

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/NodeRare...
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/NodeRare...
Source/core/dom/NodeRareData.h:316: NodeRareData(RenderObject* renderer)

Add explicit.

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/NodeRare...
Source/core/dom/NodeRareData.h:341: ownerNode.clearNodeLists();

When the last cached list is removed from the Node, we need to discard the
NodeListsNodeData (to save memory). How is this logic implemented in oilpan?

I guess that some weak processing is necessary to implement the logic.

https://codereview.chromium.org/280123002/diff/20001/Source/core/html/Collect...
File Source/core/html/CollectionIndexCache.h (right):

https://codereview.chromium.org/280123002/diff/20001/Source/core/html/Collect...
Source/core/html/CollectionIndexCache.h:41: ALLOW_ONLY_INLINE_ALLOCATION();

Shouldn't this be DISALLOW_ALLOCATION()? CollectionIndexCache is used as a part
of object.

https://codereview.chromium.org/280123002/diff/20001/Source/core/html/Collect...
Source/core/html/CollectionIndexCache.h:67: void traceAfterDispatch(Visitor*
visitor)

Why isn't this trace()?

https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
File Source/web/tests/TouchActionTest.cpp (right):

https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
Source/web/tests/TouchActionTest.cpp:192:
WebCore::Heap::collectGarbage(WebCore::ThreadState::HeapPointersOnStack);

Would you elaborate on this?

- Why can PendingGCRunner run during this test?
- Even if it runs, why does it run assuming no pointers on stack even though
there is pointers on stack?

https://codereview.chromium.org/280123002/diff/20001/public/web/WebElementCol...
File public/web/WebElementCollection.h (right):

https://codereview.chromium.org/280123002/diff/20001/public/web/WebElementCol...
public/web/WebElementCollection.h:60: bool isNull() const { return
m_private.isNull(); }

Nit: If we forget to make this change, do we hit a compile error? I'm afraid
that it's easy to forget to make this change.

[sof, 2014-05-12 12:24:35]

Thanks; first round of answers :)

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3717:
m_nodeLists[list->invalidationType()].remove(WeakMember<LiveNodeListBase>(list));
On 2014/05/12 12:04:13, haraken wrote:
> 
> Isn't this weak processing automatically done by oilpan's GC?
> 
> In the first place, you're commenting out unregisterNodeList() from
> LiveNodeListBase.h (line 62), so this call path shouldn't run when the
> LiveNodeListBase is destructed.

It is done automatically most times but for the case of moving a NodeList to
another document, LiveNodeListBase::didMoveToDocument(), where the switchover
needs to happen explicitly.

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3723: m_listsInvalidatedAtDocument.remove(list);
On 2014/05/12 12:04:13, haraken wrote:
> 
> I think you can remove this line from oilpan-builds. The weak processing is
> automatically done by oilpan's GC.

I think it needs to stay; see above.

https://codereview.chromium.org/280123002/diff/20001/Source/core/html/Collect...
File Source/core/html/CollectionIndexCache.h (right):

https://codereview.chromium.org/280123002/diff/20001/Source/core/html/Collect...
Source/core/html/CollectionIndexCache.h:41: ALLOW_ONLY_INLINE_ALLOCATION();
On 2014/05/12 12:04:13, haraken wrote:
> 
> Shouldn't this be DISALLOW_ALLOCATION()? CollectionIndexCache is used as a
part
> of object.

I suppose it is a value object in collections here ;), but you're right.

https://codereview.chromium.org/280123002/diff/20001/Source/core/html/Collect...
Source/core/html/CollectionIndexCache.h:67: void traceAfterDispatch(Visitor*
visitor)
On 2014/05/12 12:04:13, haraken wrote:
> 
> Why isn't this trace()?

With DISALLOW_ALLOCATION() used instead, it should be renamed.

https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
File Source/web/tests/TouchActionTest.cpp (right):

https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
Source/web/tests/TouchActionTest.cpp:192:
WebCore::Heap::collectGarbage(WebCore::ThreadState::HeapPointersOnStack);
On 2014/05/12 12:04:13, haraken wrote:
> 
> Would you elaborate on this?
> 
> - Why can PendingGCRunner run during this test?
> - Even if it runs, why does it run assuming no pointers on stack even though
> there is pointers on stack?

The parser thread creates a pending GC runner. I don't know the answer to the
2nd, hence the FIXME, other than I assume the GC runner is meant to be run at a
safe point.

https://codereview.chromium.org/280123002/diff/20001/public/web/WebElementCol...
File public/web/WebElementCollection.h (right):

https://codereview.chromium.org/280123002/diff/20001/public/web/WebElementCol...
public/web/WebElementCollection.h:60: bool isNull() const { return
m_private.isNull(); }
On 2014/05/12 12:04:13, haraken wrote:
> 
> Nit: If we forget to make this change, do we hit a compile error? I'm afraid
> that it's easy to forget to make this change.

I seem to remember answering this question in some other CL some time ago :) I
do it be reflex by now, but it does fail to compile.

[haraken, 2014-05-12 12:36:54]

https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
File Source/web/tests/TouchActionTest.cpp (right):

https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
Source/web/tests/TouchActionTest.cpp:192:
WebCore::Heap::collectGarbage(WebCore::ThreadState::HeapPointersOnStack);
On 2014/05/12 12:24:36, sof wrote:
> On 2014/05/12 12:04:13, haraken wrote:
> > 
> > Would you elaborate on this?
> > 
> > - Why can PendingGCRunner run during this test?
> > - Even if it runs, why does it run assuming no pointers on stack even though
> > there is pointers on stack?
> 
> The parser thread creates a pending GC runner. I don't know the answer to the
> 2nd, hence the FIXME, other than I assume the GC runner is meant to be run at
a
> safe point.

hmm, your current solution looks a bit fragile. Even if you force
Heap::collectGarbage, it's possible that the pending GC runner comes into a
play.

Probably is the condition under which the PendingGCRunner uses
HeapPointersOnStack wrong?

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

[sof, 2014-05-12 12:52:45]

https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
File Source/web/tests/TouchActionTest.cpp (right):

https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
Source/web/tests/TouchActionTest.cpp:192:
WebCore::Heap::collectGarbage(WebCore::ThreadState::HeapPointersOnStack);
On 2014/05/12 12:36:55, haraken wrote:
> On 2014/05/12 12:24:36, sof wrote:
> > On 2014/05/12 12:04:13, haraken wrote:
> > > 
> > > Would you elaborate on this?
> > > 
> > > - Why can PendingGCRunner run during this test?
> > > - Even if it runs, why does it run assuming no pointers on stack even
though
> > > there is pointers on stack?
> > 
> > The parser thread creates a pending GC runner. I don't know the answer to
the
> > 2nd, hence the FIXME, other than I assume the GC runner is meant to be run
at
> a
> > safe point.
> 
> hmm, your current solution looks a bit fragile. Even if you force
> Heap::collectGarbage, it's possible that the pending GC runner comes into a
> play.
> 
> Probably is the condition under which the PendingGCRunner uses
> HeapPointersOnStack wrong?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Indeed so; I'm not proposing this as the final solution (cf FIXME).

Need a different setup for tests, perhaps? Seeking input on where best to
address this.

[haraken, 2014-05-12 13:06:48]

On 2014/05/12 12:52:45, sof wrote:
>
https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
> File Source/web/tests/TouchActionTest.cpp (right):
> 
>
https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
> Source/web/tests/TouchActionTest.cpp:192:
> WebCore::Heap::collectGarbage(WebCore::ThreadState::HeapPointersOnStack);
> On 2014/05/12 12:36:55, haraken wrote:
> > On 2014/05/12 12:24:36, sof wrote:
> > > On 2014/05/12 12:04:13, haraken wrote:
> > > > 
> > > > Would you elaborate on this?
> > > > 
> > > > - Why can PendingGCRunner run during this test?
> > > > - Even if it runs, why does it run assuming no pointers on stack even
> though
> > > > there is pointers on stack?
> > > 
> > > The parser thread creates a pending GC runner. I don't know the answer to
> the
> > > 2nd, hence the FIXME, other than I assume the GC runner is meant to be run
> at
> > a
> > > safe point.
> > 
> > hmm, your current solution looks a bit fragile. Even if you force
> > Heap::collectGarbage, it's possible that the pending GC runner comes into a
> > play.
> > 
> > Probably is the condition under which the PendingGCRunner uses
> > HeapPointersOnStack wrong?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Indeed so; I'm not proposing this as the final solution (cf FIXME).
> 
> Need a different setup for tests, perhaps? Seeking input on where best to
> address this.

OK, let's address it in a follow-up CL. However, the FIXME indicates that
something wrong might be going on in the PendingGCRunner which can affect real
products as well.

[tkent, 2014-05-13 11:20:35]

https://codereview.chromium.org/280123002/diff/20001/Source/web/WebFormElemen...
File Source/web/WebFormElement.cpp (right):

https://codereview.chromium.org/280123002/diff/20001/Source/web/WebFormElemen...
Source/web/WebFormElement.cpp:84: // FIXME: The second argument of
HTMLFormElement::getNamedElements should be
We can resolve this FIXME.

https://codereview.chromium.org/280123002/diff/20001/public/web/WebElementCol...
File public/web/WebElementCollection.h (right):

https://codereview.chromium.org/280123002/diff/20001/public/web/WebElementCol...
public/web/WebElementCollection.h:35: #include "public/platform/WebCommon.h"
Please keep ../.  This change will break Chromium build.

[sof, 2014-05-15 09:23:22]

https://codereview.chromium.org/280123002/diff/20001/Source/web/WebFormElemen...
File Source/web/WebFormElement.cpp (right):

https://codereview.chromium.org/280123002/diff/20001/Source/web/WebFormElemen...
Source/web/WebFormElement.cpp:84: // FIXME: The second argument of
HTMLFormElement::getNamedElements should be
On 2014/05/13 11:20:37, tkent (ooo until May 18) wrote:
> We can resolve this FIXME.

Done, but the type conversions needed to be able to allow such a direct
assign()ment was a bit fiddly to get right.

Ended up adding a WebNode constructor for Oilpan to handle this, please let me
know what you think. The cleanest solution I was able to come up with.

(As the FIXME in the header file says, the reference arg could be given the more
precise type WebVector<WebElement>, but changing a public API is best done
separately.)

https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
File Source/web/tests/TouchActionTest.cpp (right):

https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
Source/web/tests/TouchActionTest.cpp:192:
WebCore::Heap::collectGarbage(WebCore::ThreadState::HeapPointersOnStack);
On 2014/05/12 12:52:46, sof wrote:
> On 2014/05/12 12:36:55, haraken wrote:
> > On 2014/05/12 12:24:36, sof wrote:
> > > On 2014/05/12 12:04:13, haraken wrote:
> > > > 
> > > > Would you elaborate on this?
> > > > 
> > > > - Why can PendingGCRunner run during this test?
> > > > - Even if it runs, why does it run assuming no pointers on stack even
> though
> > > > there is pointers on stack?
> > > 
> > > The parser thread creates a pending GC runner. I don't know the answer to
> the
> > > 2nd, hence the FIXME, other than I assume the GC runner is meant to be run
> at
> > a
> > > safe point.
> > 
> > hmm, your current solution looks a bit fragile. Even if you force
> > Heap::collectGarbage, it's possible that the pending GC runner comes into a
> > play.
> > 
> > Probably is the condition under which the PendingGCRunner uses
> > HeapPointersOnStack wrong?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Indeed so; I'm not proposing this as the final solution (cf FIXME).
> 
> Need a different setup for tests, perhaps? Seeking input on where best to
> address this.

Tried to make the FIXME comment clearer; i.e., tests either need to be set up to
run with HeapPointersOnStack, or we need to adjust the running of
PendingGCRunner.

https://codereview.chromium.org/280123002/diff/20001/public/web/WebElementCol...
File public/web/WebElementCollection.h (right):

https://codereview.chromium.org/280123002/diff/20001/public/web/WebElementCol...
public/web/WebElementCollection.h:35: #include "public/platform/WebCommon.h"
On 2014/05/13 11:20:37, tkent (ooo until May 18) wrote:
> Please keep ../.  This change will break Chromium build.

Reverted, but I notice there are both styles (i.e., relative paths and
"public/platform") of #includes of WebCommon.h and other headers in this
directory. (And I thought we wanted to move to the latter.)

[Mads Ager (chromium), 2014-05-15 10:54:22]

Lots of stuff going on here. Thanks for taking this on Sigbjørn!

The set of NodeLists instead of a count is annoying but I don't immediately see
a better way of doing it.

I'm happy with this change once we have figured out why the PendingGCRunner gets
into play in the test. Can you collect a stack trace and post it so we can have
a look?

https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
File Source/web/tests/TouchActionTest.cpp (right):

https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
Source/web/tests/TouchActionTest.cpp:192:
WebCore::Heap::collectGarbage(WebCore::ThreadState::HeapPointersOnStack);
On 2014/05/15 09:23:24, sof wrote:
> On 2014/05/12 12:52:46, sof wrote:
> > On 2014/05/12 12:36:55, haraken wrote:
> > > On 2014/05/12 12:24:36, sof wrote:
> > > > On 2014/05/12 12:04:13, haraken wrote:
> > > > > 
> > > > > Would you elaborate on this?
> > > > > 
> > > > > - Why can PendingGCRunner run during this test?
> > > > > - Even if it runs, why does it run assuming no pointers on stack even
> > though
> > > > > there is pointers on stack?
> > > > 
> > > > The parser thread creates a pending GC runner. I don't know the answer
to
> > the
> > > > 2nd, hence the FIXME, other than I assume the GC runner is meant to be
run
> > at
> > > a
> > > > safe point.
> > > 
> > > hmm, your current solution looks a bit fragile. Even if you force
> > > Heap::collectGarbage, it's possible that the pending GC runner comes into
a
> > > play.
> > > 
> > > Probably is the condition under which the PendingGCRunner uses
> > > HeapPointersOnStack wrong?
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Indeed so; I'm not proposing this as the final solution (cf FIXME).
> > 
> > Need a different setup for tests, perhaps? Seeking input on where best to
> > address this.
> 
> Tried to make the FIXME comment clearer; i.e., tests either need to be set up
to
> run with HeapPointersOnStack, or we need to adjust the running of
> PendingGCRunner.

The PendingGCRunner should only run when we are back at the event loop and
therefore know that there are no pointers on the stack. Are these tests creating
a separate event loop where they pump tasks through on this thread? Can you post
a stack trace Sigbjorn? We need to understand this. Something is completely
wrong here. If the test is doing something nasty like that which cannot happen
in real code it seems to me that the solution is to use Persistents instead of
RawPtr for the pointers in this test method. However, I would like to understand
exactly how and when we end up getting the PendingGCRunner executed when we are
actually not back at an event loop.

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/ChildNod...
File Source/core/dom/ChildNodeList.cpp (right):

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/ChildNod...
Source/core/dom/ChildNodeList.cpp:74:
m_collectionIndexCache.traceAfterDispatch(visitor);
This should just be visitor->trace(m_collectionIndexCache)?

There are more occurences.

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:594: #if !ENABLE(OILPAN)
Remove this line and line 592? :)

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.h:1317: // is currently alive or not for the different
types.
is is -> is

https://codereview.chromium.org/280123002/diff/60001/Source/core/html/Collect...
File Source/core/html/CollectionIndexCache.h (right):

https://codereview.chromium.org/280123002/diff/60001/Source/core/html/Collect...
Source/core/html/CollectionIndexCache.h:67: void traceAfterDispatch(Visitor*
visitor)
This is just a non-virtual trace method, so just call it trace and use
visitor->trace(collectionIndexCache) when you have one?

https://codereview.chromium.org/280123002/diff/60001/Source/core/html/HTMLAll...
File Source/core/html/HTMLAllCollection.cpp (right):

https://codereview.chromium.org/280123002/diff/60001/Source/core/html/HTMLAll...
Source/core/html/HTMLAllCollection.cpp:80: returnValue1 =
PassRefPtr<Element>(namedItems.at(0).get());
Does the following work in both builds?

// FIXME: Oilpan: Remove the call to |get| when Element becomes
[GarbageCollected].
returnValue1 = namedItems.at(0).get();

There are multiple occurrences if it does.

[sof, 2014-05-15 11:39:40]

https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
File Source/web/tests/TouchActionTest.cpp (right):

https://codereview.chromium.org/280123002/diff/20001/Source/web/tests/TouchAc...
Source/web/tests/TouchActionTest.cpp:192:
WebCore::Heap::collectGarbage(WebCore::ThreadState::HeapPointersOnStack);
On 2014/05/15 10:54:24, Mads Ager (chromium) wrote:
> On 2014/05/15 09:23:24, sof wrote:
> > On 2014/05/12 12:52:46, sof wrote:
> > > On 2014/05/12 12:36:55, haraken wrote:
> > > > On 2014/05/12 12:24:36, sof wrote:
> > > > > On 2014/05/12 12:04:13, haraken wrote:
> > > > > > 
> > > > > > Would you elaborate on this?
> > > > > > 
> > > > > > - Why can PendingGCRunner run during this test?
> > > > > > - Even if it runs, why does it run assuming no pointers on stack
even
> > > though
> > > > > > there is pointers on stack?
> > > > > 
> > > > > The parser thread creates a pending GC runner. I don't know the answer
> to
> > > the
> > > > > 2nd, hence the FIXME, other than I assume the GC runner is meant to be
> run
> > > at
> > > > a
> > > > > safe point.
> > > > 
> > > > hmm, your current solution looks a bit fragile. Even if you force
> > > > Heap::collectGarbage, it's possible that the pending GC runner comes
into
> a
> > > > play.
> > > > 
> > > > Probably is the condition under which the PendingGCRunner uses
> > > > HeapPointersOnStack wrong?
> > > > 
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > > Indeed so; I'm not proposing this as the final solution (cf FIXME).
> > > 
> > > Need a different setup for tests, perhaps? Seeking input on where best to
> > > address this.
> > 
> > Tried to make the FIXME comment clearer; i.e., tests either need to be set
up
> to
> > run with HeapPointersOnStack, or we need to adjust the running of
> > PendingGCRunner.
> 
> The PendingGCRunner should only run when we are back at the event loop and
> therefore know that there are no pointers on the stack. Are these tests
creating
> a separate event loop where they pump tasks through on this thread? Can you
post
> a stack trace Sigbjorn? We need to understand this. Something is completely
> wrong here. If the test is doing something nasty like that which cannot happen
> in real code it seems to me that the solution is to use Persistents instead of
> RawPtr for the pointers in this test method. However, I would like to
understand
> exactly how and when we end up getting the PendingGCRunner executed when we
are
> actually not back at an event loop.

A nested message loop, it seems. The PendingGCRunner that triggers the GC is
initially created from:

#0  WebCore::PendingGCRunner::PendingGCRunner (this=0x6cd6440) at
../../third_party/WebKit/Source/platform/heap/glue/PendingGCRunner.h:41
#1  0x000000000146b45f in blink::initializeWithoutV8 (platform=0x6c5f0e8) at
../../third_party/WebKit/Source/web/WebKit.cpp:163
#2  0x000000000146b0cf in blink::initialize (platform=0x6c5f0e8) at
../../third_party/WebKit/Source/web/WebKit.cpp:98
#3  0x000000000080f42c in
content::TestWebKitPlatformSupport::TestWebKitPlatformSupport (this=0x6c5f0e0)
at ../../content/test/test_webkit_platform_support.cc:48
#4  0x000000000080e460 in content::(anonymous
namespace)::TestEnvironment::TestEnvironment (this=0x6c5e120) at
../../content/test/webkit_support.cc:63
#5  0x000000000080e4e0 in content::SetUpTestEnvironmentForUnitTests () at
../../content/test/webkit_support.cc:111
#6  0x000000000080e111 in content::SetUpTestEnvironmentForWebKitUnitTests () at
../../content/test/webkit_unit_test_support.cc:12
#7  0x0000000000417c67 in main (argc=1, argv=0x7fffffffdea8) at
../../third_party/WebKit/Source/web/tests/RunAllTests.cpp:63

with a "no heap pointers on stack" GC then happening later on,

#0  WebCore::Heap::collectGarbage
(stackState=WebCore::ThreadState::NoHeapPointersOnStack) at
../../third_party/WebKit/Source/platform/heap/Heap.cpp:1631
#1  0x00000000009511c3 in WebCore::ThreadState::performPendingGC (this=0x6b7c900
<WebCore::ThreadState::s_mainThreadStateStorage>,
stackState=WebCore::ThreadState::NoHeapPointersOnStack)
    at ../../third_party/WebKit/Source/platform/heap/ThreadState.cpp:638
#2  0x0000000000951796 in WebCore::ThreadState::safePoint (this=0x6b7c900
<WebCore::ThreadState::s_mainThreadStateStorage>,
stackState=WebCore::ThreadState::NoHeapPointersOnStack)
    at ../../third_party/WebKit/Source/platform/heap/ThreadState.cpp:738
#3  0x000000000146bcfa in WebCore::PendingGCRunner::didProcessTask
(this=0x6cd6440) at
../../third_party/WebKit/Source/platform/heap/glue/PendingGCRunner.h:64
#4  0x0000000002fb31b1 in
content::WebThreadBase::TaskObserverAdapter::DidProcessTask (this=0x6cd64a0,
pending_task=...) at ../../content/child/webthread_impl.cc:32
#5  0x00000000030d2a46 in base::MessageLoop::RunTask (this=0x6c5e530,
pending_task=...) at ../../base/message_loop/message_loop.cc:451
#6  0x00000000030d2b12 in base::MessageLoop::DeferOrRunPendingTask
(this=0x6c5e530, pending_task=...) at
../../base/message_loop/message_loop.cc:462
#7  0x00000000030d3055 in base::MessageLoop::DoWork (this=0x6c5e530) at
../../base/message_loop/message_loop.cc:576
#8  0x000000000318aa81 in base::MessagePumpGlib::HandleDispatch (this=0x6c5e7f0)
at ../../base/message_loop/message_pump_glib.cc:219
#9  0x000000000318a4af in base::(anonymous namespace)::WorkSourceDispatch
(source=0x6c5e8a0, unused_func=0x0, unused_data=0x0) at
../../base/message_loop/message_pump_glib.cc:107
#10 0x00007ffff6ca4f05 in g_main_dispatch (context=0x6c5ed00) at
/build/buildd/glib2.0-2.36.0/./glib/gmain.c:3054
#11 g_main_context_dispatch (context=context@entry=0x6c5ed00) at
/build/buildd/glib2.0-2.36.0/./glib/gmain.c:3630
#12 0x00007ffff6ca5248 in g_main_context_iterate
(context=context@entry=0x6c5ed00, block=block@entry=0,
dispatch=dispatch@entry=1, self=<optimized out>)
    at /build/buildd/glib2.0-2.36.0/./glib/gmain.c:3701
#13 0x00007ffff6ca5304 in g_main_context_iteration (context=0x6c5ed00,
may_block=0) at /build/buildd/glib2.0-2.36.0/./glib/gmain.c:3762
#14 0x000000000318ac67 in base::MessagePumpGlib::Run (this=0x6c5e7f0,
delegate=0x6c5e530) at ../../base/message_loop/message_pump_glib.cc:266
#15 0x00000000030d24e6 in base::MessageLoop::RunHandler (this=0x6c5e530) at
../../base/message_loop/message_loop.cc:400
#16 0x00000000031212de in base::RunLoop::Run (this=0x7fffffffcb00) at
../../base/run_loop.cc:49
#17 0x00000000030d1b9a in base::MessageLoop::Run (this=0x6c5e530) at
../../base/message_loop/message_loop.cc:293
#18 0x0000000002fb2e12 in content::WebThreadImplForMessageLoop::enterRunLoop
(this=0x6cd63e0) at ../../content/child/webthread_impl.cc:116
#19 0x000000000060e3e4 in blink::FrameTestHelpers::runPendingTasks () at
../../third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp:252
#20 0x000000000066250d in (anonymous namespace)::TouchActionTest::sendTouchEvent
(this=0x6d2a460, webView=0x109621e48010, type=blink::WebInputEvent::TouchStart,
clientPoint=...)
    at ../../third_party/WebKit/Source/web/tests/TouchActionTest.cpp:321
#21 0x00000000006615db in (anonymous namespace)::TouchActionTest::runTestOnTree
(this=0x6d2a460, root=0x7ffff7f85598, webView=0x109621e48010, client=...)
    at ../../third_party/WebKit/Source/web/tests/TouchActionTest.cpp:265
#22 0x000000000065ff8a in (anonymous
namespace)::TouchActionTest::runTouchActionTest (this=0x6d2a460, file=...) at
../../third_party/WebKit/Source/web/tests/TouchActionTest.cpp:137
#23 0x0000000000662630 in (anonymous
namespace)::TouchActionTest_Simple_Test::TestBody (this=0x6d2a460) at
../../third_party/WebKit/Source/web/tests/TouchActionTest.cpp:326
#24 0x00000000007e42cd in
testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>
(object=0x6d2a460, method=&virtual testing::Test::TestBody(), 
    location=0x3ae08a3 "the test body") at ../../testing/gtest/src/gtest.cc:1989
#25 0x00000000007df809 in
testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>
(object=0x6d2a460, method=&virtual testing::Test::TestBody(), 
    location=0x3ae08a3 "the test body") at ../../testing/gtest/src/gtest.cc:2042
#26 0x00000000007d3ca9 in testing::Test::Run (this=0x6d2a460) at
../../testing/gtest/src/gtest.cc:2062
#27 0x00000000007d42d2 in testing::TestInfo::Run (this=0x6c20470) at
../../testing/gtest/src/gtest.cc:2237
#28 0x00000000007d482d in testing::TestCase::Run (this=0x6c205a0) at
../../testing/gtest/src/gtest.cc:2344
#29 0x00000000007d9290 in testing::internal::UnitTestImpl::RunAllTests
(this=0x6bc7920) at ../../testing/gtest/src/gtest.cc:4065
#30 0x00000000007e56f8 in
testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,
bool> (object=0x6bc7920, 
    method=(bool
(testing::internal::UnitTestImpl::*)(testing::internal::UnitTestImpl * const))
0x7d8f9e <testing::internal::UnitTestImpl::RunAllTests()>, 
    location=0x3ae1110 "auxiliary test code (environments or event listeners)")
at ../../testing/gtest/src/gtest.cc:1989
#31 0x00000000007e0961 in
testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,
bool> (object=0x6bc7920, 
    method=(bool
(testing::internal::UnitTestImpl::*)(testing::internal::UnitTestImpl * const))
0x7d8f9e <testing::internal::UnitTestImpl::RunAllTests()>, 
    location=0x3ae1110 "auxiliary test code (environments or event listeners)")
at ../../testing/gtest/src/gtest.cc:2042
#32 0x00000000007d823f in testing::UnitTest::Run (this=0x6b7c060
<testing::UnitTest::GetInstance()::instance>) at
../../testing/gtest/src/gtest.cc:3700
#33 0x0000000000793627 in RUN_ALL_TESTS () at
../../testing/gtest/include/gtest/gtest.h:2231
#34 0x0000000000792f86 in base::TestSuite::Run (this=0x7fffffffdd90) at
../../base/test/test_suite.cc:206
#35 0x0000000000417c73 in main (argc=1, argv=0x7fffffffdea8) at
../../third_party/WebKit/Source/web/tests/RunAllTests.cpp:64

[Mads Ager (chromium), 2014-05-15 11:41:04]

Here is the relevant part of the stack trace when we get a GC with no pointers
on the stack from here. The issue is indeed that the test manually runs the
event loop from the test and therefore, there are pointers on the stack below
the event loop. I think the best way to fix this is to make the
RefPtrWillBeRawPtr on the stack in this test RefPtrWillBePersistent with a
comment that these pointers are below the event loop because the test explicitly
controls the event loop. This will never happen in the shipping system and we
should just make sure that the GC knows about these pointers that are on the
stack below the event loop.

#17 0x00007fffec58230a in content::WebThreadImplForMessageLoop::enterRunLoop
(this=0x7fffe10fa410) at ../../content/child/webthread_impl.cc:116
#18 0x00007fffef4c2e2f in blink::FrameTestHelpers::runPendingTasks () at
../../third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp:252
#19 0x00007fffef510dab in (anonymous namespace)::TouchActionTest::sendTouchEvent
(this=0x133d6928f440, webView=0x208534048010,
type=blink::WebInputEvent::TouchStart, clientPoint=...)
    at ../../third_party/WebKit/Source/web/tests/TouchActionTest.cpp:321
#20 0x00007fffef50feea in (anonymous namespace)::TouchActionTest::runTestOnTree
(this=0x133d6928f440, root=0x7fffdec85598, webView=0x208534048010, client=...)
    at ../../third_party/WebKit/Source/web/tests/TouchActionTest.cpp:265
#21 0x00007fffef50ebda in (anonymous
namespace)::TouchActionTest::runTouchActionTest (this=0x133d6928f440, file=...)
at ../../third_party/WebKit/Source/web/tests/TouchActionTest.cpp:137

[sof, 2014-05-15 11:52:34]

On 2014/05/15 11:41:04, Mads Ager (chromium) wrote:
> Here is the relevant part of the stack trace when we get a GC with no pointers
> on the stack from here. The issue is indeed that the test manually runs the
> event loop from the test and therefore, there are pointers on the stack below
> the event loop. I think the best way to fix this is to make the
> RefPtrWillBeRawPtr on the stack in this test RefPtrWillBePersistent with a
> comment that these pointers are below the event loop because the test
explicitly
> controls the event loop. This will never happen in the shipping system and we
> should just make sure that the GC knows about these pointers that are on the
> stack below the event loop.
> 
> #17 0x00007fffec58230a in content::WebThreadImplForMessageLoop::enterRunLoop
> (this=0x7fffe10fa410) at ../../content/child/webthread_impl.cc:116
> #18 0x00007fffef4c2e2f in blink::FrameTestHelpers::runPendingTasks () at
> ../../third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp:252
> #19 0x00007fffef510dab in (anonymous
namespace)::TouchActionTest::sendTouchEvent
> (this=0x133d6928f440, webView=0x208534048010,
> type=blink::WebInputEvent::TouchStart, clientPoint=...)
>     at ../../third_party/WebKit/Source/web/tests/TouchActionTest.cpp:321
> #20 0x00007fffef50feea in (anonymous
namespace)::TouchActionTest::runTestOnTree
> (this=0x133d6928f440, root=0x7fffdec85598, webView=0x208534048010, client=...)
>     at ../../third_party/WebKit/Source/web/tests/TouchActionTest.cpp:265
> #21 0x00007fffef50ebda in (anonymous
> namespace)::TouchActionTest::runTouchActionTest (this=0x133d6928f440,
file=...)
> at ../../third_party/WebKit/Source/web/tests/TouchActionTest.cpp:137

good, let's try going with that.

[Erik Corry, 2014-05-15 12:34:50]

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.h:670: void incrementNodeListWithIdNameCacheCount();
Consider having the same API and ignoring the list pre-oilpan.

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3716:
ASSERT(m_nodeLists[list->invalidationType()].contains(WeakMember<LiveNodeListBase>(list)));
I think you can write contains(list) instead of
contains(WeakMember<LiveNodeListBase>(list))
At least it works in clang.  Is there some compiler that won't eat that?

There are 5 instances of this pattern.

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5711: return
shouldInvalidateNodeListCachesForAttr<DoNotInvalidateOnAttributeChanges +
1>(m_nodeLists, *attrName);
Looks like all these templated functions are just a wildly complicated way to
write

for (unsigned i = DoNotInvalidateOnAttributeChanges + 1; i <
numNodeListInvalidationTypes; ++i) {
    if (!nodeLists[i].isEmpty() && shouldInvalidateTypeOnAttributeChange(i,
attrName))
        return true;
}
return false;

Probably you need to add some casts.  The templated version causes us to
generate a function per NodeListInvalidationType and recurse instead of
iterating.

[sof, 2014-05-15 22:15:56]

Followups on the Source/core/ parts; thanks for the on-target feedback
throughout.

Been debugging WeakMember<const T> usage in hash maps, some unexpected behavior
encountered. See comments.

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3721: if (list->isRootedAtDocument()) {
On 2014/05/12 12:04:13, haraken wrote:
> 
> I'm just curious but why is this check necessary in non-oilpan builds? Maybe
the
> fact that this check is necessary implies that simple weak processing is not
> enough for m_listsInvalidatedAtDocument? (I'm not sure.)

It is used to handle fast invalidation of certain document collections, and is
orthogonal to the NodeLists we separately&weakly keep.

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3733: void
Document::decrementNodeListWithIdNameCacheCount(const LiveNodeListBase* list)
On 2014/05/12 12:04:13, haraken wrote:
> 
> I'd rename these methods to {register,unregister}NodeListWithIdNameCache().

Done.

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.h:670: void incrementNodeListWithIdNameCacheCount();
On 2014/05/15 12:34:52, Erik Corry wrote:
> Consider having the same API and ignoring the list pre-oilpan.

ok, i resisted the temptation initially as the non-Oilpan methods can be
provided as inlines. But I don't think there is much to worry about perf wise
here really, so let's try unifying.

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.h:1317: // is currently alive or not for the different
types.
On 2014/05/12 12:04:13, haraken wrote:
> 
> is is => is

Done.

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/NodeRare...
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/280123002/diff/20001/Source/core/dom/NodeRare...
Source/core/dom/NodeRareData.h:316: NodeRareData(RenderObject* renderer)
On 2014/05/12 12:04:13, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/ChildNod...
File Source/core/dom/ChildNodeList.cpp (right):

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/ChildNod...
Source/core/dom/ChildNodeList.cpp:74:
m_collectionIndexCache.traceAfterDispatch(visitor);
On 2014/05/15 10:54:24, Mads Ager (chromium) wrote:
> This should just be visitor->trace(m_collectionIndexCache)?
> 
> There are more occurences.

Thanks, switched to trace().

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3716:
ASSERT(m_nodeLists[list->invalidationType()].contains(WeakMember<LiveNodeListBase>(list)));
On 2014/05/15 12:34:52, Erik Corry wrote:
> I think you can write contains(list) instead of
> contains(WeakMember<LiveNodeListBase>(list))
> At least it works in clang.  Is there some compiler that won't eat that?
> 
> There are 5 instances of this pattern.

Yes, the explicit WeakMember<>s aren't needed, removed them.

The const_cast<>s are needed though, but I'd like to avoid using them. However,
if I change m_nodeLists[i]'s key to be WeakMember<const LiveNodeListBase>, the
isAlive() method in effect isn't adjusting the object pointer as required for
this mixin & it crashes when determining if the weak reference is alive.

I haven't pinned down if this is due to the 'const' getting in the way when
deriving what ObjectAliveTrait to use; will resume the chase tomorrow.

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5711: return
shouldInvalidateNodeListCachesForAttr<DoNotInvalidateOnAttributeChanges +
1>(m_nodeLists, *attrName);
On 2014/05/15 12:34:52, Erik Corry wrote:
> Looks like all these templated functions are just a wildly complicated way to
> write
> 
> for (unsigned i = DoNotInvalidateOnAttributeChanges + 1; i <
> numNodeListInvalidationTypes; ++i) {
>     if (!nodeLists[i].isEmpty() && shouldInvalidateTypeOnAttributeChange(i,
> attrName))
>         return true;
> }
> return false;
> 
> Probably you need to add some casts.  The templated version causes us to
> generate a function per NodeListInvalidationType and recurse instead of
> iterating.

Alright, took that on here as well, removing the template expansion over the
enum for a simpler loop.

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.h:1317: // is currently alive or not for the different
types.
On 2014/05/15 10:54:24, Mads Ager (chromium) wrote:
> is is -> is

Done.

https://codereview.chromium.org/280123002/diff/60001/Source/core/html/Collect...
File Source/core/html/CollectionIndexCache.h (right):

https://codereview.chromium.org/280123002/diff/60001/Source/core/html/Collect...
Source/core/html/CollectionIndexCache.h:67: void traceAfterDispatch(Visitor*
visitor)
On 2014/05/15 10:54:24, Mads Ager (chromium) wrote:
> This is just a non-virtual trace method, so just call it trace and use
> visitor->trace(collectionIndexCache) when you have one?

Yes, now done + switched to DISALLOW_ALLOCATION() for this class.

https://codereview.chromium.org/280123002/diff/60001/Source/core/html/HTMLAll...
File Source/core/html/HTMLAllCollection.cpp (right):

https://codereview.chromium.org/280123002/diff/60001/Source/core/html/HTMLAll...
Source/core/html/HTMLAllCollection.cpp:80: returnValue1 =
PassRefPtr<Element>(namedItems.at(0).get());
On 2014/05/15 10:54:24, Mads Ager (chromium) wrote:
> Does the following work in both builds?
> 
> // FIXME: Oilpan: Remove the call to |get| when Element becomes
> [GarbageCollected].
> returnValue1 = namedItems.at(0).get();
> 
> There are multiple occurrences if it does.

It works to drop down to a bare pointer, the FIXME will help explain why it
looks unusual in a non-Oilpan light.

[sof, 2014-05-16 06:12:27]

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3716:
ASSERT(m_nodeLists[list->invalidationType()].contains(WeakMember<LiveNodeListBase>(list)));
On 2014/05/15 22:15:57, sof wrote:
> On 2014/05/15 12:34:52, Erik Corry wrote:
> > I think you can write contains(list) instead of
> > contains(WeakMember<LiveNodeListBase>(list))
> > At least it works in clang.  Is there some compiler that won't eat that?
> > 
> > There are 5 instances of this pattern.
> 
> Yes, the explicit WeakMember<>s aren't needed, removed them.
> 
> The const_cast<>s are needed though, but I'd like to avoid using them.
However,
> if I change m_nodeLists[i]'s key to be WeakMember<const LiveNodeListBase>, the
> isAlive() method in effect isn't adjusting the object pointer as required for
> this mixin & it crashes when determining if the weak reference is alive.
> 
> I haven't pinned down if this is due to the 'const' getting in the way when
> deriving what ObjectAliveTrait to use; will resume the chase tomorrow.

That is the problem,

  NeedsAdjustAndMark<const LiveNodeListBase>::value == false
  NeedsAdjustAndMark<LiveNodeListBase>::value == true

By redundantly adding "const"s to the underlying type conversion check +
virtually inheriting GarbageCollectedMixin throughout almost fixes it -- 2-3
instances of inaccessible bases compilation errors to contend with.

Don't have a solution for handling those wrt WTF::IsSubclass<> yet (short of
opening up the inheritance to public.)

[sof, 2014-05-16 08:15:22]

On 2014/05/16 06:12:27, sof wrote:
>
https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document...
> File Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/280123002/diff/60001/Source/core/dom/Document...
> Source/core/dom/Document.cpp:3716:
>
ASSERT(m_nodeLists[list->invalidationType()].contains(WeakMember<LiveNodeListBase>(list)));
> On 2014/05/15 22:15:57, sof wrote:
> > On 2014/05/15 12:34:52, Erik Corry wrote:
> > > I think you can write contains(list) instead of
> > > contains(WeakMember<LiveNodeListBase>(list))
> > > At least it works in clang.  Is there some compiler that won't eat that?
> > > 
> > > There are 5 instances of this pattern.
> > 
> > Yes, the explicit WeakMember<>s aren't needed, removed them.
> > 
> > The const_cast<>s are needed though, but I'd like to avoid using them.
> However,
> > if I change m_nodeLists[i]'s key to be WeakMember<const LiveNodeListBase>,
the
> > isAlive() method in effect isn't adjusting the object pointer as required
for
> > this mixin & it crashes when determining if the weak reference is alive.
> > 
> > I haven't pinned down if this is due to the 'const' getting in the way when
> > deriving what ObjectAliveTrait to use; will resume the chase tomorrow.
> 
> That is the problem,
> 
>   NeedsAdjustAndMark<const LiveNodeListBase>::value == false
>   NeedsAdjustAndMark<LiveNodeListBase>::value == true
> 
> By redundantly adding "const"s to the underlying type conversion check +
> virtually inheriting GarbageCollectedMixin throughout almost fixes it -- 2-3
> instances of inaccessible bases compilation errors to contend with.
> 
> Don't have a solution for handling those wrt WTF::IsSubclass<> yet (short of
> opening up the inheritance to public.)

That last part is due to another conversion problem wrt NeedsAdjustAndMark<T>;
it looks like NeedsAdjustAndMark<> usage has worked out by accident so far..?

Anyway, split this out as https://codereview.chromium.org/282263006/

[sof, 2014-05-16 16:06:41]

On 2014/05/16 08:15:22, sof wrote:
...
> > 
> > That is the problem,
> > 
> >   NeedsAdjustAndMark<const LiveNodeListBase>::value == false
> >   NeedsAdjustAndMark<LiveNodeListBase>::value == true
> > 
> > By redundantly adding "const"s to the underlying type conversion check +
> > virtually inheriting GarbageCollectedMixin throughout almost fixes it -- 2-3
> > instances of inaccessible bases compilation errors to contend with.
> > 
> > Don't have a solution for handling those wrt WTF::IsSubclass<> yet (short of
> > opening up the inheritance to public.)
> 
> That last part is due to another conversion problem wrt NeedsAdjustAndMark<T>;
> it looks like NeedsAdjustAndMark<> usage has worked out by accident so far..?
> 
> Anyway, split this out as https://codereview.chromium.org/282263006/

Landed in r174175. Rebased + removed the three egregious const_cast<>s over
LiveNodeListBase* as a result.

[haraken, 2014-05-18 22:08:56]

Sorry for the review delay, I'll take a look at this tomorrow morning.

[tkent, 2014-05-19 07:23:39]

lgtm.  Approved except bindings/.

https://codereview.chromium.org/280123002/diff/120001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/280123002/diff/120001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5750: for (int i = 0; i <
numNodeListInvalidationTypes; i++)
nit: We prefer ++i.

https://codereview.chromium.org/280123002/diff/120001/Source/core/dom/StaticN...
File Source/core/dom/StaticNodeList.cpp (right):

https://codereview.chromium.org/280123002/diff/120001/Source/core/dom/StaticN...
Source/core/dom/StaticNodeList.cpp:41: for (size_t i = 0; i < nodes.size(); i++)
nit: We prefer ++i.

https://codereview.chromium.org/280123002/diff/120001/public/web/WebNode.h
File public/web/WebNode.h (right):

https://codereview.chromium.org/280123002/diff/120001/public/web/WebNode.h#ne...
public/web/WebNode.h:148: // This constructor enables creation of WebNodes from
Members
Sounds reasonable.

[sof, 2014-05-19 07:59:00]

https://codereview.chromium.org/280123002/diff/120001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/280123002/diff/120001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5750: for (int i = 0; i <
numNodeListInvalidationTypes; i++)
On 2014/05/19 07:23:40, tkent wrote:
> nit: We prefer ++i.

Thanks, canonicalized to pre-increment (here & elsewhere.)

[haraken, 2014-05-19 09:19:49]

https://codereview.chromium.org/280123002/diff/120001/Source/core/dom/NodeRar...
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/280123002/diff/120001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:232: RawPtrWillBeMember<NodeList>
m_childNodeList;

Shouldn't this be a weak member? I think NodeRareData can outlive the NodeList.

https://codereview.chromium.org/280123002/diff/120001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:341: ownerNode.clearNodeLists();

I still have a question on this. (Sorry if you've answered somewhere.)

When the last cached list is removed from the Node, we need to discard the
NodeListsNodeData (to save memory). How is this logic implemented in oilpan?

https://codereview.chromium.org/280123002/diff/120001/public/web/WebNode.h
File public/web/WebNode.h (right):

https://codereview.chromium.org/280123002/diff/120001/public/web/WebNode.h#ne...
public/web/WebNode.h:152: // won't be usable.

Yeah, probably sounds reasonable.

It might be possible to provide a RawPtr<T> constructor from a Member<U>, but it
might confuse overload resolution for toV8() etc. (I'm not sure.)

[sof, 2014-05-19 21:36:44]

https://codereview.chromium.org/280123002/diff/120001/Source/core/dom/NodeRar...
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/280123002/diff/120001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:232: RawPtrWillBeMember<NodeList>
m_childNodeList;
On 2014/05/19 09:19:50, haraken wrote:
> 
> Shouldn't this be a weak member? I think NodeRareData can outlive the
NodeList.

Good catch, do want to refer to the NodeList weakly for that reason.

https://codereview.chromium.org/280123002/diff/120001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:341: ownerNode.clearNodeLists();
On 2014/05/19 09:19:50, haraken wrote:
> 
> I still have a question on this. (Sorry if you've answered somewhere.)
> 
> When the last cached list is removed from the Node, we need to discard the
> NodeListsNodeData (to save memory). How is this logic implemented in oilpan?

Thanks for persisting in getting an answer :) I hadn't taken on this
optimization. I have now, at the cost of adding some Oilpan-only code to
NodeListsNodeData.

[sof, 2014-05-19 21:40:47]

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:225: visitor->trace(m_owner);
Not sure about this trace() call (and the one for the other weak member,
m_childNodeList) -- is there a way to signal to the GC plugin that the weak
members will be handled manually within the weak callback?

[Erik Corry, 2014-05-19 21:44:59]

https://codereview.chromium.org/280123002/diff/60001/Source/web/WebNodeList.cpp
File Source/web/WebNodeList.cpp (right):

https://codereview.chromium.org/280123002/diff/60001/Source/web/WebNodeList.c...
Source/web/WebNodeList.cpp:53: WebNodeList::WebNodeList(const
PassRefPtrWillBeRawPtr<NodeList>& list)
Passing a reference to a pointer.  Should this not just be the pointer itself?

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:225: visitor->trace(m_owner);
On 2014/05/19 21:40:49, sof wrote:
> Not sure about this trace() call (and the one for the other weak member,
> m_childNodeList) -- is there a way to signal to the GC plugin that the weak
> members will be handled manually within the weak callback? 

I think you just have to mention the fields in the trace method to shut it up. 
Just having them as an expression statement should be enough:

m_childNodeList;

[zerny-chromium, 2014-05-20 06:07:42]

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:225: visitor->trace(m_owner);
On 2014/05/19 21:40:49, sof wrote:
> Not sure about this trace() call (and the one for the other weak member,
> m_childNodeList) -- is there a way to signal to the GC plugin that the weak
> members will be handled manually within the weak callback? 

This is fine as is. The plugin will/should identify the registered callback
function and since both m_owner and m_childNodeList appear there it will
consider them traced.

One small nit: we might wan't to move this trace to the cpp file. I'm not sure
if all compilation units will have access to the definition of clearWeakMembers
since it occurs in the cpp file but trace is in the header.

[sof, 2014-05-20 06:23:58]

https://codereview.chromium.org/280123002/diff/60001/Source/web/WebNodeList.cpp
File Source/web/WebNodeList.cpp (right):

https://codereview.chromium.org/280123002/diff/60001/Source/web/WebNodeList.c...
Source/web/WebNodeList.cpp:53: WebNodeList::WebNodeList(const
PassRefPtrWillBeRawPtr<NodeList>& list)
On 2014/05/19 21:45:00, Erik Corry wrote:
> Passing a reference to a pointer.  Should this not just be the pointer itself?

Passing a const reference to a wrapped pointer. Other than stating that this
pointer representation is not to be mutated, I don't know the real reason why
this is the style for the Blink-internal pointer constructors for web/.

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:225: visitor->trace(m_owner);
On 2014/05/20 06:07:43, zerny-chromium wrote:
> On 2014/05/19 21:40:49, sof wrote:
> > Not sure about this trace() call (and the one for the other weak member,
> > m_childNodeList) -- is there a way to signal to the GC plugin that the weak
> > members will be handled manually within the weak callback? 
> 
> This is fine as is. The plugin will/should identify the registered callback
> function and since both m_owner and m_childNodeList appear there it will
> consider them traced.
> 

Oh, that sounds encouraging. I used isAlive() over those two members in
clearWeakMembers() initially but the GC checker insisted on there being trace()s
for them.

> One small nit: we might wan't to move this trace to the cpp file. I'm not sure
> if all compilation units will have access to the definition of
clearWeakMembers
> since it occurs in the cpp file but trace is in the header.

Perhaps that is the reason why the GC plugin wasn't able to spot it? :) Will
move it there regardless.

[Mads Ager (chromium), 2014-05-20 06:58:55]

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
File Source/core/dom/NodeRareData.cpp (right):

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.cpp:64: if (m_owner && (m_childNodeList ? 1 : 0) +
m_atomicNameCaches.size() + m_tagCollectionCacheNS.size() == 0) {
When we make m_owner strong again we can simplify and not check for m_owner and
not clear m_owner. We just need to check if everything became empty and clear
the Member from the owner to this object. Then this object will die in the next
GC round.

This could delay things a bit but the cleanup will happen eventually. During the
GC that makes this empty, we might get this callback before the sets have been
cleared by weak processing. That would delay the cleanup by one GC cycle. Then
we clear out the pointer from the owner to this object. This object will die in
the next round of GC, so we could delay the removal by two GCs but it will
happen in the end. :)

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:175: static
PassOwnPtrWillBeRawPtr<NodeListsNodeData> create(NodeRareData* owner)
Since we have the #if around the entire definition we should just use the real
oilpan/non-oilpan types in the two versions.

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:215: #if ENABLE(OILPAN)
Nit: For Document and friends I have not used #if for clearWeakMembers. We could
move the #if to the body of clearWeakMembers to reduce the #if spaghetti. I
don't feel strongly about it though. :)

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:224: #if ENABLE(OILPAN)
Nit: Maybe we should just #if out the entire body instead of just parts of it?

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:232: NodeListsNodeData(NodeRareData* owner)
explicit

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:258: WeakMember<NodeRareData> m_owner;
The NodeRareData owns the NodeListNodeData and therefore I don't see any reason
for this to be a WeakMember. The NodeListNodeData will be unreachable when
removed from the NodeRareData and at that point the Member will not be traced
and it will not keep the NodeRareData alive. When they are connected they should
live and die together. So there is no need for weakness here. Let's change this
back to a strong pointer?

I'm sorry for the back and forth here. In general, if we think that something
might need to be weak we should make sure that it cannot just be strong. We
should always prefer strong pointers.

[zerny-chromium, 2014-05-20 07:57:58]

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:225: visitor->trace(m_owner);
On 2014/05/20 06:23:59, sof wrote:
> On 2014/05/20 06:07:43, zerny-chromium wrote:
> > On 2014/05/19 21:40:49, sof wrote:
> > > Not sure about this trace() call (and the one for the other weak member,
> > > m_childNodeList) -- is there a way to signal to the GC plugin that the
weak
> > > members will be handled manually within the weak callback? 
> > 
> > This is fine as is. The plugin will/should identify the registered callback
> > function and since both m_owner and m_childNodeList appear there it will
> > consider them traced.
> > 
> 
> Oh, that sounds encouraging. I used isAlive() over those two members in
> clearWeakMembers() initially but the GC checker insisted on there being
trace()s
> for them.
> 
> > One small nit: we might wan't to move this trace to the cpp file. I'm not
sure
> > if all compilation units will have access to the definition of
> clearWeakMembers
> > since it occurs in the cpp file but trace is in the header.
> 
> Perhaps that is the reason why the GC plugin wasn't able to spot it? :) Will
> move it there regardless.

I think it is. The plugin checks trace methods every time it encounters its
definition, so that will cause this trace to be checked in compilation units
that do not know about the clearWeakMembers callback. Better to keep them
together. Maybe I should find a way to warn about such undefined callbacks when
checking trace.

[sof, 2014-05-20 08:27:47]

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:175: static
PassOwnPtrWillBeRawPtr<NodeListsNodeData> create(NodeRareData* owner)
On 2014/05/20 06:58:57, Mads Ager (chromium) wrote:
> Since we have the #if around the entire definition we should just use the real
> oilpan/non-oilpan types in the two versions.

Done.

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:215: #if ENABLE(OILPAN)
On 2014/05/20 06:58:57, Mads Ager (chromium) wrote:
> Nit: For Document and friends I have not used #if for clearWeakMembers. We
could
> move the #if to the body of clearWeakMembers to reduce the #if spaghetti. I
> don't feel strongly about it though. :)

I will leave it out for non-Oilpan for now, unpretty as it is. We've already
#if-ed out the non-Oilpan version of this optimization, so by symmetry.. :)

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:224: #if ENABLE(OILPAN)
On 2014/05/20 06:58:57, Mads Ager (chromium) wrote:
> Nit: Maybe we should just #if out the entire body instead of just parts of it?

Done.

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:225: visitor->trace(m_owner);
On 2014/05/20 07:58:00, zerny-chromium wrote:
> On 2014/05/20 06:23:59, sof wrote:
> > On 2014/05/20 06:07:43, zerny-chromium wrote:
> > > On 2014/05/19 21:40:49, sof wrote:
> > > > Not sure about this trace() call (and the one for the other weak member,
> > > > m_childNodeList) -- is there a way to signal to the GC plugin that the
> weak
> > > > members will be handled manually within the weak callback? 
> > > 
> > > This is fine as is. The plugin will/should identify the registered
callback
> > > function and since both m_owner and m_childNodeList appear there it will
> > > consider them traced.
> > > 
> > 
> > Oh, that sounds encouraging. I used isAlive() over those two members in
> > clearWeakMembers() initially but the GC checker insisted on there being
> trace()s
> > for them.
> > 
> > > One small nit: we might wan't to move this trace to the cpp file. I'm not
> sure
> > > if all compilation units will have access to the definition of
> > clearWeakMembers
> > > since it occurs in the cpp file but trace is in the header.
> > 
> > Perhaps that is the reason why the GC plugin wasn't able to spot it? :) Will
> > move it there regardless.
> 
> I think it is. The plugin checks trace methods every time it encounters its
> definition, so that will cause this trace to be checked in compilation units
> that do not know about the clearWeakMembers callback. Better to keep them
> together. Maybe I should find a way to warn about such undefined callbacks
when
> checking trace.

You're quite right, not reported as trace() problem with the method moved into
the .cpp file.

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:232: NodeListsNodeData(NodeRareData* owner)
On 2014/05/20 06:58:57, Mads Ager (chromium) wrote:
> explicit

Done.

https://codereview.chromium.org/280123002/diff/160001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.h:258: WeakMember<NodeRareData> m_owner;
On 2014/05/20 06:58:57, Mads Ager (chromium) wrote:
> The NodeRareData owns the NodeListNodeData and therefore I don't see any
reason
> for this to be a WeakMember. The NodeListNodeData will be unreachable when
> removed from the NodeRareData and at that point the Member will not be traced
> and it will not keep the NodeRareData alive. When they are connected they
should
> live and die together. So there is no need for weakness here. Let's change
this
> back to a strong pointer?
> 
> I'm sorry for the back and forth here. In general, if we think that something
> might need to be weak we should make sure that it cannot just be strong. We
> should always prefer strong pointers.

thanks for bringing clarity; that makes good sense.

The back reference to NodeRareData will not prolong its lifetime, which is
really controlled by its Node. Hence, a strong reference will do just fine.
Switched and simplified.

[Mads Ager (chromium), 2014-05-20 08:45:08]

LGTM

[sof, 2014-05-20 09:07:29]

haraken: will wait on your input before doing anything here, should there be
further issues that you want to see addressed.

(I've gone through previous comments to check that nothing hasn't been attended
to.)

[Erik Corry, 2014-05-20 09:09:46]

LGTM

https://codereview.chromium.org/280123002/diff/180001/Source/core/dom/NodeRar...
File Source/core/dom/NodeRareData.cpp (right):

https://codereview.chromium.org/280123002/diff/180001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.cpp:73: #if ENABLE(OILPAN)
An alternative (I think simpler) way to do this would be to just have
m_childNodeList be a weak pointer and call trace on in normally.  In the trace
method of the NodeRareData you check whether the NodeListsNodeData is empty and
if it is you zero the pointer instead of tracing it.  This way there are no
explicitly registered weak callbacks.  The NodeListsNodeData is deleted one GC
later than the rest, but I don't think that's a big deal.

[haraken, 2014-05-20 09:27:31]

Double-checked. LGTM.

https://codereview.chromium.org/280123002/diff/180001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/280123002/diff/180001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5685: for (int type = 1; type <
numNodeListInvalidationTypes; ++type) {

Not related to your CL, why does this iteration start from 1 not from 0?

https://codereview.chromium.org/280123002/diff/180001/Source/core/dom/NodeRar...
File Source/core/dom/NodeRareData.cpp (right):

https://codereview.chromium.org/280123002/diff/180001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.cpp:64: if (m_childNodeList &&
!visitor->isAlive(m_childNodeList))

Nit: You don't need to check m_childNodeList.

[sof, 2014-05-20 09:54:55]

https://codereview.chromium.org/280123002/diff/180001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/280123002/diff/180001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:5685: for (int type = 1; type <
numNodeListInvalidationTypes; ++type) {
On 2014/05/20 09:27:33, haraken wrote:
> 
> Not related to your CL, why does this iteration start from 1 not from 0?

It seems like a very minor optimization; it will behave the same if you start at
0 (the predicate called upon in this loop returns false for
0/DoNotInvalidateOnAttributeChanges.)

https://codereview.chromium.org/280123002/diff/180001/Source/core/dom/NodeRar...
File Source/core/dom/NodeRareData.cpp (right):

https://codereview.chromium.org/280123002/diff/180001/Source/core/dom/NodeRar...
Source/core/dom/NodeRareData.cpp:73: #if ENABLE(OILPAN)
On 2014/05/20 09:09:48, Erik Corry wrote:
> An alternative (I think simpler) way to do this would be to just have
> m_childNodeList be a weak pointer and call trace on in normally.  In the trace
> method of the NodeRareData you check whether the NodeListsNodeData is empty
and
> if it is you zero the pointer instead of tracing it.  This way there are no
> explicitly registered weak callbacks.  The NodeListsNodeData is deleted one GC
> later than the rest, but I don't think that's a big deal.

I like this,it just fits better all around to move the is-empty checking out of
NodeListsNodeData to the object holding the reference. Done.

[sof, 2014-05-20 11:48:55]

Thanks for the reviews, sending this one along.

[sof, 2014-05-20 11:49:13]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-05-20 11:49:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/280123002/200001

[commit-bot: I haz the power, 2014-05-20 13:07:23]

Change committed as 174377