Oilpan: Make the Node hierarchy RefCountedGarbageCollected instead of TreeShared.

Oilpan: Make the Node hierarchy RefCountedGarbageCollected instead of TreeShared.

The entire tree structure is now traced. If one node in the tree is
alive the entire tree is kept alive. This makes the Node hierarchy
independent of reference counts and it is therefore possible to use
Members instead of RefPtrs to Nodes.

This change is very likely to introduce new Oilpan leaks because
any RefPtr from a Node to another Node in the same tree will leak
the entire tree. Once this change lands we should eliminate RefPtrs
to Nodes to avoid these leaks.

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

[Mads Ager (chromium), 2014-05-05 10:18:35]

This change is passing tests and I would like to get this landed ASAP and
iterate on it from there. Once this change lands we no longer have to worry
about replacing RefPtrs to Nodes with Members. That is now safe.

Kouhei, could you have a look at the SVG part of this change? That is the part
that I have spent the least amount of time on and the part of the code that I
understand the least. I would like to get some input on that part.

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

https://codereview.chromium.org/262093006/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:603: void Document::dispose()
The Document::dispose method breaks a lot of reference cycles that would
otherwise leak. With Oilpan all of this reference cycle breaking is unnecessary.
The form controller is still cleared (it contains RefPtrs) but that should be
removed as well in a follow-up change.

[Mads Ager (chromium), 2014-05-05 10:47:41]

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
File Source/core/dom/ContainerNodeAlgorithms.h (left):

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNodeAlgorithms.h:79: inline void
removeDetachedChildrenInContainer(GenericNodeContainer& container)
I had to move this code around so the Private namespace parts are declared
before use. The changes here are much smaller than they appear.

[zerny-chromium, 2014-05-05 11:07:52]

lgtm with some minor comments/questions.

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
File Source/core/dom/ContainerNodeAlgorithms.h (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNodeAlgorithms.h:144: // Helper functions for
TreeShared-derived classes, which have a 'Node' style interface
Nit: reference to "TreeShared-derived classes" is not accurate for the two
builds anymore.

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

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:2239: dispose();
Why the conditional call to dispose?

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/UserActi...
File Source/core/dom/UserActionElementSet.h (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/UserActi...
Source/core/dom/UserActionElementSet.h:40: class UserActionElementSet {
Nit: FINAL (since we use a non-virtual trace)

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/shadow/E...
File Source/core/dom/shadow/ElementShadow.h (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/shadow/E...
Source/core/dom/shadow/ElementShadow.h:42: class ElementShadow {
ditto

https://codereview.chromium.org/262093006/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/TextControlInnerElements.cpp (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/TextControlInnerElements.cpp:459: 
Nit: unneeded edit

[sof, 2014-05-05 11:27:47]

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/shadow/E...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/shadow/E...
Source/core/dom/shadow/ElementShadow.cpp:357:
visitor->mark(m_shadowRoots.head());
nit: you prefer to use mark() on raw pointers instead of trace()? Or is there
something else going on?

https://codereview.chromium.org/262093006/diff/20001/Source/core/svg/SVGEleme...
File Source/core/svg/SVGElementInstance.cpp (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/svg/SVGEleme...
Source/core/svg/SVGElementInstance.cpp:92: , m_element(originalElement.get()) //
FIXME: Oilpan: Remove the .get() call.
Add m_shadowTreeElement(nullptr) for tidiness?

[Erik Corry, 2014-05-05 11:35:47]

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:83: removeDetachedChildrenInContainer<Node,
ContainerNode>(*this);
This doesn't need to be called in the oilpan build?

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
File Source/core/dom/ContainerNodeAlgorithms.h (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNodeAlgorithms.h:150: // Always dispatch node removal
notifications when nodes are
This looks relevant for regular container nodes, but this function is not called
for them in the oilpan build any more(?)

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNodeAlgorithms.h:152: // been removed from the ducument
and their removedFrom method has
ducument -> duckument!

[Mads Ager (chromium), 2014-05-05 11:35:53]

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
File Source/core/dom/ContainerNodeAlgorithms.h (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNodeAlgorithms.h:144: // Helper functions for
TreeShared-derived classes, which have a 'Node' style interface
On 2014/05/05 11:07:52, zerny-chromium wrote:
> Nit: reference to "TreeShared-derived classes" is not accurate for the two
> builds anymore.

Yes, I would prefer to not change the comment at this point, but you are right
that it doesn't make sense for the oilpan build.

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

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:2239: dispose();
On 2014/05/05 11:07:52, zerny-chromium wrote:
> Why the conditional call to dispose?

At this point the Document has been detached from the DOMWindow. There is no way
to reattach a Document once it has been detached. dispose clears a number of
RefPtrs that would otherwise leak the entire tree. I think we can get rid of
dispose entirely, but at this point we need a call to dispose in order to clear
out some RefPtrs in the FormController.

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/UserActi...
File Source/core/dom/UserActionElementSet.h (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/UserActi...
Source/core/dom/UserActionElementSet.h:40: class UserActionElementSet {
On 2014/05/05 11:07:52, zerny-chromium wrote:
> Nit: FINAL (since we use a non-virtual trace)

Done.

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/shadow/E...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/shadow/E...
Source/core/dom/shadow/ElementShadow.cpp:357:
visitor->mark(m_shadowRoots.head());
On 2014/05/05 11:27:48, sof wrote:
> nit: you prefer to use mark() on raw pointers instead of trace()? Or is there
> something else going on?

Oops. Nothing special going on here. In the old days trace was called mark and
old habits die hard. ;-)

Changed to trace.

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/shadow/E...
File Source/core/dom/shadow/ElementShadow.h (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/shadow/E...
Source/core/dom/shadow/ElementShadow.h:42: class ElementShadow {
On 2014/05/05 11:07:52, zerny-chromium wrote:
> ditto

Done.

https://codereview.chromium.org/262093006/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/TextControlInnerElements.cpp (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/TextControlInnerElements.cpp:459: 
On 2014/05/05 11:07:52, zerny-chromium wrote:
> Nit: unneeded edit

Yeah, I'm going to leave it here. There should be an empty line before the
#endif. :)

https://codereview.chromium.org/262093006/diff/20001/Source/core/svg/SVGEleme...
File Source/core/svg/SVGElementInstance.cpp (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/svg/SVGEleme...
Source/core/svg/SVGElementInstance.cpp:92: , m_element(originalElement.get()) //
FIXME: Oilpan: Remove the .get() call.
On 2014/05/05 11:27:48, sof wrote:
> Add m_shadowTreeElement(nullptr) for tidiness?

You have good eyes! Done. :)

[Mads Ager (chromium), 2014-05-05 11:49:16]

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:83: removeDetachedChildrenInContainer<Node,
ContainerNode>(*this);
On 2014/05/05 11:35:48, Erik Corry wrote:
> This doesn't need to be called in the oilpan build?

No, this particular one is the one that is called when the reference count
reaches zero and is responsible for the weird behavior where parent nodes can
mysteriously disappear even though there is a pointer to one of the children. We
don't want that with Oilpan. There is a separate call to dispatch child removal
notifications when a node is explicitly removed from its container and that one
we keep.

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
File Source/core/dom/ContainerNodeAlgorithms.h (right):

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNodeAlgorithms.h:150: // Always dispatch node removal
notifications when nodes are
On 2014/05/05 11:35:48, Erik Corry wrote:
> This looks relevant for regular container nodes, but this function is not
called
> for them in the oilpan build any more(?)

Yeah, its complicated...

This is still called for SVGElementInstances on detach. This was never called
for explicit removal of nodes from a container node. See
ContainerNode::removeChild/removeBetween which does this work for the explicit
removal case.

https://codereview.chromium.org/262093006/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNodeAlgorithms.h:152: // been removed from the ducument
and their removedFrom method has
On 2014/05/05 11:35:48, Erik Corry wrote:
> ducument -> duckument!

Yeah, the ducument -> their container.

[sof, 2014-05-05 12:11:49]

lgtm to try

[Erik Corry, 2014-05-05 12:47:00]

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.h:215: Document& m_document;
This doesn't keep the document alive?

[Mads Ager (chromium), 2014-05-05 12:52:38]

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.h:215: Document& m_document;
On 2014/05/05 12:47:01, Erik Corry wrote:
> This doesn't keep the document alive?

The StyleEngine is owned (OwnPtr) by the document. When we move the StyleEngine
to the heap we should make this a Member.

[Mads Ager (chromium), 2014-05-05 12:54:55]

On 2014/05/05 12:52:38, Mads Ager (chromium) wrote:
>
https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
> File Source/core/dom/StyleEngine.h (right):
> 
>
https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
> Source/core/dom/StyleEngine.h:215: Document& m_document;
> On 2014/05/05 12:47:01, Erik Corry wrote:
> > This doesn't keep the document alive?
> 
> The StyleEngine is owned (OwnPtr) by the document. When we move the
StyleEngine
> to the heap we should make this a Member.

Yeah, we already did. However, the only reference to the StyleEngine is from the
Document and therefore the reference is safe. I'm not changing it here. It is
annoying to attempt to change it since it has turned into a reference now on
trunk and replacing references with pointers is a pain. However, it is a pain
that we will have to deal with soon...

[Mads Ager (chromium), 2014-05-05 13:06:39]

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:93: m_fontSelector->clearDocument();
Erik points out that removing this is probably unsafe. He is right, however,
Gustav has a change ready that will make it safe and we will see if we can land
our changes together tomorrow morning. :)

[haraken, 2014-05-05 16:57:22]

Let me take a close look tomorrow morning.

[haraken, 2014-05-06 04:20:15]

I'm sorry for a lot of clarifying questions -- I'd be happy if you could help me
understand. I think I can LG in the next round of comments (probably tomorrow).

I haven't yet checked the SVG part in this CL.

https://codereview.chromium.org/262093006/diff/60001/LayoutTests/OilpanExpect...
File LayoutTests/OilpanExpectations (right):

https://codereview.chromium.org/262093006/diff/60001/LayoutTests/OilpanExpect...
LayoutTests/OilpanExpectations:28: # These tests pass but the throw a different
exception than before. We need to figure out if

the throw => throw

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:101: #endif

Shall we add ASSERT(needsAttach()) to oilpan builds as well?

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:587: if (newChild.get() == m_lastChild) //
nothing to do

Shall we override == so that we can write this as 'newChild == m_lastChild' ?

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Containe...
File Source/core/dom/ContainerNodeAlgorithms.h (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNodeAlgorithms.h:145: // This applies to
'ContainerNode' and 'SVGElementInstance'

FYI, SVGElementInstance is going to be removed soon :)

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNodeAlgorithms.h:147: inline void
removeDetachedChildrenInContainer(GenericNodeContainer& container)

Just help me understand (very fundamental question): Who calls
removeDetachedChildrenInContainer() in oilpan builds? removeDetachedChildren()
is removed in oilpan builds, and I cannot find any caller of
removeDetachedChildrenInContainer() for the Node hierarchy, which made me
confused.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNodeAlgorithms.h:153: // been called, or the nodes die
with the their container.

the their container => their container

Just to confirm: If the nodes dies with their container,
removeDetachedChildrenInContainer() is not called and thus removedFrom() is not
called on the nodes. Right?

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

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:545: removeAllEventListenersRecursively();

Removing this line is great, but we'll still need to call
InspectorInstrumentation::didRemoveAllEventListeners(this) for each node.

Also probably do we still need to clear firingEventIterators of each Node?
Otherwise, firingEventIterators can retain Events that are targeting
already-dead Nodes. (Actually I don't fully understand how the
firingEventIterators works. I might be wrong.)

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:603: void Document::dispose()

Just to confirm: In oilpan, Document::dispose() is called when the entire tree
of the Document becomes unreachable. Thus it's not possible to reach the
Document after Document::dispose() is called. Is my understanding correct?

(In non-oilpan, it's possible to reach the Document after Document::dispose() is
called because of the existence of guardRef().)

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:616: m_contextFeatures =
ContextFeatures::defaultSwitch();

I'm curious why you can remove this before moving m_contextFeatures to the heap.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:620: detachParser();

Ditto. I'm curious why you can remove this before moving m_parser to the heap.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:622: m_registrationContext.clear();

Ditto. I'm curious why you can remove this before moving m_registrationContext
to the heap.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:625: m_importsController->wasDetachedFrom(*this);

I guess we still need this to call HTMLImportsController::clear() when the
Document is disposed. For example, we need to notify the import loader that the
import is going to be cancelled.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:631: destroyTreeScopeData();

As far as I read the comment in line 629 and 630, we need to clear the
TreeScopeData promptly when the Document is detached. If so, why can we remove
this line?

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:640: m_markers->clear();

Just to confirm: There is no problem in calling m_markers->clear(), but you
removed this because it's not needed (because the MarkerMap in the
DocumentMarkerController is cleared by weak processing), right?

(Once we move the DocumentMarkerController to the heap, the complexity will be
gone anyway.)

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:654:
m_lifecycle.advanceTo(DocumentLifecycle::Disposed);

- I wonder why you need to remove this line.

- If you need to remove this line for some reason, did you update all customers
of DocumentLifecycle::Disposed so that they don't rely on the state?

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:2239: dispose();

Just help me understand (very fundamental question): Before this CL, dispose()
was called when the last reference to the TreeScope is gone. After this CL,
dispose() is called when the Document is detached. Why is this change OK? (I
think you're doing a right thing, but I want to understand how the timing of
detach(), dispose() and removeDetachedChildren() is going to be reorganized.)

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4896: RefPtrWillBeMember<HTMLCanvasElement>&
element = m_cssCanvasElements.add(name, nullptr).storedValue->value;

I forgot the previous discussion, but what was the conclusion about the get/set
pattern v.s. the add/new pattern? Can we keep using the add/new pattern, or is
it safer to rewrite it with the get/set pattern?

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5692: visitor->trace(m_mediaQueryMatcher);

You're tracing m_mediaQueryMatcher twice :)

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/ElementR...
File Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/ElementR...
Source/core/dom/ElementRareData.h:129: visitor->trace(m_shadow);

It's confusing that ElementShadow is traced but ElementRareData::m_shadow is
kept as an OwnPtr. Can we move ElementShadow to the heap at the same time in
this CL? (I guess you already implemented ElementShadow::trace() in this CL, so
it won't that hard to change ElementShadow to
NoBaseWillBeGarbageCollectedFinalized.)

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:93: m_fontSelector->clearDocument();
On 2014/05/05 13:06:40, Mads Ager (chromium) wrote:
> Erik points out that removing this is probably unsafe. He is right, however,
> Gustav has a change ready that will make it safe and we will see if we can
land
> our changes together tomorrow morning. :)

Yeah, this looks unsafe. Want to look at the change from Gustav :)

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:101: m_styleSheetCollectionMap.clear();

Just to confirm: The line 99 - 101 was not needed in oilpan builds even before
this CL, right?

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.h:215: Document& m_document;
On 2014/05/05 12:52:39, Mads Ager (chromium) wrote:
> On 2014/05/05 12:47:01, Erik Corry wrote:
> > This doesn't keep the document alive?
> 
> The StyleEngine is owned (OwnPtr) by the document. When we move the
StyleEngine
> to the heap we should make this a Member.

The StyleEngine is already on the heap :)

Before this CL, Document::m_styleEngine was a persistent handle, and thus this
Document& was no problem. However, after this CL, Document::m_styleEngine
becomes a Member and thus Document& becomes a problem. So I think you need to
change this to a Member in this CL.

Probably we should seriously consider replacing a reference back to a raw
pointer, and replace it with RawPtrWillBeMember.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/TreeScop...
File Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/TreeScop...
Source/core/dom/TreeScope.cpp:90: m_rootNode.setTreeScope(0);

You can remove this code because the root Node is guaranteed to exist in this
TreeScope and thus die together. Changing m_rootNode to a Member will make the
lifetime relationship clearer.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/TreeScop...
Source/core/dom/TreeScope.cpp:540: visitor->trace(&m_rootNode);

This looks weird. Shall we change the reference back to a raw pointer and use
RawPtrWillBeMember in this CL?

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/UserActi...
File Source/core/dom/UserActionElementSet.h (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/UserActi...
Source/core/dom/UserActionElementSet.h:56: void documentDidRemoveLastRef();

You can add #if !ENABLE(OILPAN) to this method.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/shadow/E...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/shadow/E...
Source/core/dom/shadow/ElementShadow.cpp:173:
InspectorInstrumentation::willPopShadowRoot(shadowHost, oldRoot.get());

I guess you'll still need to call this when the ElementShadow is destructed.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/shadow/E...
Source/core/dom/shadow/ElementShadow.cpp:357:
visitor->trace(m_shadowRoots.head());

Let's add a comment to mention why we just need to trace the head element.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/shadow/S...
File Source/core/dom/shadow/ShadowRoot.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/shadow/S...
Source/core/dom/shadow/ShadowRoot.cpp:98: removeDetachedChildren();

The same question as before, but I got confused about when
removeDetachedChildrenInContainer is called in the Node hierarchy... :)

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/FormAss...
File Source/core/html/FormAssociatedElement.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/FormAss...
Source/core/html/FormAssociatedElement.cpp:93:
setFormAttributeTargetObserver(nullptr);

The observer is unregistered when FormAssociatedElement::removedFrom is called,
but how is the observer unregistered when FormAssociatedElement is destructed?

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:250: hideVisibleValidationMessage();

I'm curious why the previous code was working without this line.

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/forms/V...
File Source/core/html/forms/ValidationMessage.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/forms/V...
Source/core/html/forms/ValidationMessage.cpp:51:
client->hideValidationMessage(*m_element);

Now you're calling hideValidationMessage() in
HTMLFormControllerElement::removedFrom(). So, for consistency, I'd remove
hideValidationMessage() from here and instead add:

~HTMLFormControllerElement() {
#if !ENABLE(OILPAN)
  hideValidationMessage();
#endif
}

https://codereview.chromium.org/262093006/diff/60001/Source/core/svg/SVGEleme...
File Source/core/svg/SVGElementInstance.h (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/svg/SVGEleme...
Source/core/svg/SVGElementInstance.h:40: class SVGElementInstance FINAL : public
TreeSharedWillBeRefCountedGarbageCollected<SVGElementInstance>, public
EventTarget, public ScriptWrappable {

A good news is that SVGElementInstance is going to be removed soon :)

[Mads Ager (chromium), 2014-05-06 08:25:59]

Thanks for your comments Haraken.

Please do have another look and give more comments. I am going to go ahead and
land this change so we can iterate from here. At this point most code reviews
run into the issue that RefPtrs to nodes cannot be replaced by Members. We need
to get that fixed.

https://codereview.chromium.org/262093006/diff/60001/LayoutTests/OilpanExpect...
File LayoutTests/OilpanExpectations (right):

https://codereview.chromium.org/262093006/diff/60001/LayoutTests/OilpanExpect...
LayoutTests/OilpanExpectations:28: # These tests pass but the throw a different
exception than before. We need to figure out if
On 2014/05/06 04:20:16, haraken wrote:
> 
> the throw => throw

Thanks, changed to 'they throw'.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:101: #endif
On 2014/05/06 04:20:16, haraken wrote:
> 
> Shall we add ASSERT(needsAttach()) to oilpan builds as well?

We can probably do that, yes. Done.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:587: if (newChild.get() == m_lastChild) //
nothing to do
On 2014/05/06 04:20:16, haraken wrote:
> 
> Shall we override == so that we can write this as 'newChild == m_lastChild' ?

Yes, thanks. Added those too. It is much easier to remove the operators when we
are done than having to go through the source base and remove calls to get.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Containe...
File Source/core/dom/ContainerNodeAlgorithms.h (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNodeAlgorithms.h:145: // This applies to
'ContainerNode' and 'SVGElementInstance'
On 2014/05/06 04:20:16, haraken wrote:
> 
> FYI, SVGElementInstance is going to be removed soon :)

That is great! Right now this code is only used for SVGElementInstance in the
oilpan build and can therefore be removed at that point.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNodeAlgorithms.h:147: inline void
removeDetachedChildrenInContainer(GenericNodeContainer& container)
On 2014/05/06 04:20:16, haraken wrote:
> 
> Just help me understand (very fundamental question): Who calls
> removeDetachedChildrenInContainer() in oilpan builds? removeDetachedChildren()
> is removed in oilpan builds, and I cannot find any caller of
> removeDetachedChildrenInContainer() for the Node hierarchy, which made me
> confused.

Yes, it is very confusing. This is still called by the SVGElementInstance detach
method. However, it is actually not doing anything. The only reason for doing
this is to dispatch removedFrom notifications. However, those notifications are
only dispatched for Node and not for SVGElementInstance. I can completely remove
this from the Oilpan build. It is not doing anything at this point.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNodeAlgorithms.h:153: // been called, or the nodes die
with the their container.
On 2014/05/06 04:20:16, haraken wrote:
> 
> the their container => their container
> 
> Just to confirm: If the nodes dies with their container,
> removeDetachedChildrenInContainer() is not called and thus removedFrom() is
not
> called on the nodes. Right?

That is correct. This is only called from SVGElementInstance destructor and I am
going to remove that since the removedFrom events are not dispatched for that
hierarchy anyway so this code just sets a few pointers to 0 which isn't needed.

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

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:545: removeAllEventListenersRecursively();
On 2014/05/06 04:20:16, haraken wrote:
> 
> Removing this line is great, but we'll still need to call
> InspectorInstrumentation::didRemoveAllEventListeners(this) for each node.

Yeah, we need to figure out how to deal with InspectorInstrumentation. We cannot
iterate the nodes here and since the document is dead I'm guessing that it
doesn't matter. If it does matter we will have to use weak processing instead.
I'll add a comment here and we should talk to the devtools team about this. They
will be a BlinkOn so we can talk to them there.

> Also probably do we still need to clear firingEventIterators of each Node?
> Otherwise, firingEventIterators can retain Events that are targeting
> already-dead Nodes. (Actually I don't fully understand how the
> firingEventIterators works. I might be wrong.)

The events keep their target nodes alive, so the Document cannot be dead if
there are events for the nodes.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:603: void Document::dispose()
On 2014/05/06 04:20:16, haraken wrote:
> 
> Just to confirm: In oilpan, Document::dispose() is called when the entire tree
> of the Document becomes unreachable. Thus it's not possible to reach the
> Document after Document::dispose() is called. Is my understanding correct?
> 
> (In non-oilpan, it's possible to reach the Document after Document::dispose()
is
> called because of the existence of guardRef().)

With Oilpan, dispose is called at the point where the document reaches the
Stopped state in its lifetime cycle. I don't think there is anything stopping
people from reaching the document from JavaScript at that point, however, the
document can never be attached again and therefore we can clear out the state.

The end result should be that dispose is not called at all with oilpan because
the entire document struture will just die together and there is no reason to
explicitly pull it apart.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:616: m_contextFeatures =
ContextFeatures::defaultSwitch();
On 2014/05/06 04:20:16, haraken wrote:
> 
> I'm curious why you can remove this before moving m_contextFeatures to the
heap.

m_contextFeatures *is* on the heap and is traced, so I don't need to clear
anything here. If the Document is still reachable, so is the context features.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:620: detachParser();
On 2014/05/06 04:20:16, haraken wrote:
> 
> Ditto. I'm curious why you can remove this before moving m_parser to the heap.

You are right. I was using the fact that detachParser is called from the
destructor as well. However, based on the comment in the destructor it really
should be called here for the time being. Restored.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:622: m_registrationContext.clear();
On 2014/05/06 04:20:16, haraken wrote:
> 
> Ditto. I'm curious why you can remove this before moving m_registrationContext
> to the heap.

Let me restore it. Looking at it now I'm not entirely sure so I should leave it
in. Thanks.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:625: m_importsController->wasDetachedFrom(*this);
On 2014/05/06 04:20:16, haraken wrote:
> 
> I guess we still need this to call HTMLImportsController::clear() when the
> Document is disposed. For example, we need to notify the import loader that
the
> import is going to be cancelled.

Restored as well.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:631: destroyTreeScopeData();
On 2014/05/06 04:20:16, haraken wrote:
> 
> As far as I read the comment in line 629 and 630, we need to clear the
> TreeScopeData promptly when the Document is detached. If so, why can we remove
> this line?

We are not calling removeDetachedChildren. The children will still be alive and
reachable unless they have been explicitly removed. Therefore, the mappings are
still safe and do not have to be explicitly cleared.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:640: m_markers->clear();
On 2014/05/06 04:20:16, haraken wrote:
> 
> Just to confirm: There is no problem in calling m_markers->clear(), but you
> removed this because it's not needed (because the MarkerMap in the
> DocumentMarkerController is cleared by weak processing), right?
> 
> (Once we move the DocumentMarkerController to the heap, the complexity will be
> gone anyway.)

That's right. As long as the Document stays alive the markers will be cleared by
weak processing.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:654:
m_lifecycle.advanceTo(DocumentLifecycle::Disposed);
On 2014/05/06 04:20:16, haraken wrote:
> 
> - I wonder why you need to remove this line.
> 
> - If you need to remove this line for some reason, did you update all
customers
> of DocumentLifecycle::Disposed so that they don't rely on the state?

I removed this because we have collapsed Stopped and Disposed. There is an
assert in the DOMWindow destructor that the Document is Stopped. If I leave this
line that assert will fail. The alternative is to leave this line in and change
the assert for the oilpan build. That might be cleaner since we have been
disposed at this point. Let me do that instead.

There are no checks for the Disposed state in the source base that I can find.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:2239: dispose();
On 2014/05/06 04:20:16, haraken wrote:
> 
> Just help me understand (very fundamental question): Before this CL, dispose()
> was called when the last reference to the TreeScope is gone. After this CL,
> dispose() is called when the Document is detached. Why is this change OK? (I
> think you're doing a right thing, but I want to understand how the timing of
> detach(), dispose() and removeDetachedChildren() is going to be reorganized.)

When you detach the document it cannot be attached again and therefore some of
the work that dispose is doing can happen now. There is no reason to continue to
load imports for example. In the end dispose should do very little work. In
particular, we are not clearing any of the pointers to other Nodes because they
should be traced and stay alive as long as the Document does. So, detach and
dispose will go hand in hand. removeDetachedChildren will not be used. It was
used with reference counting to delete parts of a tree and we no longer do that.
Either a node is explicitly removed from the tree or the entire tree dies
together.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4896: RefPtrWillBeMember<HTMLCanvasElement>&
element = m_cssCanvasElements.add(name, nullptr).storedValue->value;
On 2014/05/06 04:20:16, haraken wrote:
> 
> I forgot the previous discussion, but what was the conclusion about the
get/set
> pattern v.s. the add/new pattern? Can we keep using the add/new pattern, or is
> it safer to rewrite it with the get/set pattern?

This one is safe. The only way the add/new pattern can be unsafe is if a
destructor explicitly changes the set being used. m_cssCanvasElements is used
very locally and is never modified in a destructor so this is safe.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5692: visitor->trace(m_mediaQueryMatcher);
On 2014/05/06 04:20:16, haraken wrote:
> 
> You're tracing m_mediaQueryMatcher twice :)

I REALLY don't want it to go away!!! ;-)

Thanks Haraken, you have good eyes. Merge conflict. Fixed!

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/ElementR...
File Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/ElementR...
Source/core/dom/ElementRareData.h:129: visitor->trace(m_shadow);
On 2014/05/06 04:20:16, haraken wrote:
> 
> It's confusing that ElementShadow is traced but ElementRareData::m_shadow is
> kept as an OwnPtr. Can we move ElementShadow to the heap at the same time in
> this CL? (I guess you already implemented ElementShadow::trace() in this CL,
so
> it won't that hard to change ElementShadow to
> NoBaseWillBeGarbageCollectedFinalized.)

I would prefer to do that as a followup. Moving more to the oilpan heap in this
change can lead to more finalization order issues. We need this to land soon so
it is safe to replace RefPtr with Member to the Node hierarchy.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:101: m_styleSheetCollectionMap.clear();
On 2014/05/06 04:20:16, haraken wrote:
> 
> Just to confirm: The line 99 - 101 was not needed in oilpan builds even before
> this CL, right?

I think that is right. In any case Gustav has a change out that makes sure that
this method is not needed with Oilpan.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.h:215: Document& m_document;
On 2014/05/06 04:20:16, haraken wrote:
> On 2014/05/05 12:52:39, Mads Ager (chromium) wrote:
> > On 2014/05/05 12:47:01, Erik Corry wrote:
> > > This doesn't keep the document alive?
> > 
> > The StyleEngine is owned (OwnPtr) by the document. When we move the
> StyleEngine
> > to the heap we should make this a Member.
> 
> The StyleEngine is already on the heap :)
> 
> Before this CL, Document::m_styleEngine was a persistent handle, and thus this
> Document& was no problem. However, after this CL, Document::m_styleEngine
> becomes a Member and thus Document& becomes a problem. So I think you need to
> change this to a Member in this CL.
> 
> Probably we should seriously consider replacing a reference back to a raw
> pointer, and replace it with RawPtrWillBeMember.

Yes, I'll do that. I think we will end up replacing a lot of references with
pointers again. References are really hard for us to deal with. You cannot make
good transition types for them.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/TreeScop...
File Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/TreeScop...
Source/core/dom/TreeScope.cpp:90: m_rootNode.setTreeScope(0);
On 2014/05/06 04:20:16, haraken wrote:
> 
> You can remove this code because the root Node is guaranteed to exist in this
> TreeScope and thus die together. Changing m_rootNode to a Member will make the
> lifetime relationship clearer.

Yes, it would. m_rootNode is a reference at this point but I'm tracing it. Let
me just make the reference a RawPtrWillBeMember. These references are not buying
us much and we end up taking their address explicitly a lot anyway.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/TreeScop...
Source/core/dom/TreeScope.cpp:540: visitor->trace(&m_rootNode);
On 2014/05/06 04:20:16, haraken wrote:
> 
> This looks weird. Shall we change the reference back to a raw pointer and use
> RawPtrWillBeMember in this CL?

Yes, done.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/UserActi...
File Source/core/dom/UserActionElementSet.h (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/UserActi...
Source/core/dom/UserActionElementSet.h:56: void documentDidRemoveLastRef();
On 2014/05/06 04:20:16, haraken wrote:
> 
> You can add #if !ENABLE(OILPAN) to this method.

Done.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/shadow/E...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/shadow/E...
Source/core/dom/shadow/ElementShadow.cpp:173:
InspectorInstrumentation::willPopShadowRoot(shadowHost, oldRoot.get());
On 2014/05/06 04:20:16, haraken wrote:
> 
> I guess you'll still need to call this when the ElementShadow is destructed.

We should ask the dev tools team next week. If this still needs to happen we
need to use weak processing instead.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/shadow/E...
Source/core/dom/shadow/ElementShadow.cpp:357:
visitor->trace(m_shadowRoots.head());
On 2014/05/06 04:20:16, haraken wrote:
> 
> Let's add a comment to mention why we just need to trace the head element.

Done.

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/shadow/S...
File Source/core/dom/shadow/ShadowRoot.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/shadow/S...
Source/core/dom/shadow/ShadowRoot.cpp:98: removeDetachedChildren();
On 2014/05/06 04:20:16, haraken wrote:
> 
> The same question as before, but I got confused about when
> removeDetachedChildrenInContainer is called in the Node hierarchy... :)

removeDetachedChildren was only needed with RefCounting. Nothing to be done
here.

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/FormAss...
File Source/core/html/FormAssociatedElement.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/FormAss...
Source/core/html/FormAssociatedElement.cpp:93:
setFormAttributeTargetObserver(nullptr);
On 2014/05/06 04:20:16, haraken wrote:
> 
> The observer is unregistered when FormAssociatedElement::removedFrom is
called,
> but how is the observer unregistered when FormAssociatedElement is destructed?

m_formAttributeTargetObserver is only non-null in the destructor when the
TreeScope is also dead. In that case it doesn't have to unregister itself
because the registry is part of the TreeScope and is therefore also dead.

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:250: hideVisibleValidationMessage();
On 2014/05/06 04:20:16, haraken wrote:
> 
> I'm curious why the previous code was working without this line.

Because the ValidationMessage destructor did this instead. We cannot let the
destructor do this because we cannot when the document is also dead. If the
document is also dead we don't have to do anything.

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/forms/V...
File Source/core/html/forms/ValidationMessage.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/forms/V...
Source/core/html/forms/ValidationMessage.cpp:51:
client->hideValidationMessage(*m_element);
On 2014/05/06 04:20:16, haraken wrote:
> 
> Now you're calling hideValidationMessage() in
> HTMLFormControllerElement::removedFrom(). So, for consistency, I'd remove
> hideValidationMessage() from here and instead add:
> 
> ~HTMLFormControllerElement() {
> #if !ENABLE(OILPAN)
>   hideValidationMessage();
> #endif
> }

Yeah, let me do that. There is a comment in the ValidationMessage.h file that
ValidationMessage should be removed in any case. Doing this will move a bit in
that direction.

https://codereview.chromium.org/262093006/diff/60001/Source/core/svg/SVGEleme...
File Source/core/svg/SVGElementInstance.h (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/svg/SVGEleme...
Source/core/svg/SVGElementInstance.h:40: class SVGElementInstance FINAL : public
TreeSharedWillBeRefCountedGarbageCollected<SVGElementInstance>, public
EventTarget, public ScriptWrappable {
On 2014/05/06 04:20:16, haraken wrote:
> 
> A good news is that SVGElementInstance is going to be removed soon :)

:)

[Erik Corry, 2014-05-06 08:29:39]

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:93: m_fontSelector->clearDocument();
As discussed offline this is still needed somehow.  Gustav says his change will
fix this.

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/FormAss...
File Source/core/html/FormAssociatedElement.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/FormAss...
Source/core/html/FormAssociatedElement.cpp:93:
setFormAttributeTargetObserver(nullptr);
On 2014/05/06 04:20:16, haraken wrote:
> 
> The observer is unregistered when FormAssociatedElement::removedFrom is
called,
> but how is the observer unregistered when FormAssociatedElement is destructed?

My guess is that the observer is in the form, which is in the document, and so
they all die together if the observed object is in the document tree.

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/forms/T...
File Source/core/html/forms/TextFieldInputType.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/forms/T...
Source/core/html/forms/TextFieldInputType.cpp:120:
spinButton->removeSpinButtonOwner();
m_spinButtonOwner is a raw pointer, and this CL does not change that

[Erik Corry, 2014-05-06 09:02:24]

LGTM, let's land this

https://codereview.chromium.org/262093006/diff/70001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/262093006/diff/70001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2356: #if !ENABLE(OILPAN)
Maybe just remove this assert completely since it's implied by node being in
scope.

[haraken, 2014-05-06 09:02:30]

> Please do have another look and give more comments. I am going to go ahead and
> land this change so we can iterate from here. At this point most code reviews
> run into the issue that RefPtrs to nodes cannot be replaced by Members. We
need
> to get that fixed.

Thanks for the quick update. I'll take another look in hours. This CL is
blocking a bunch of work, so let's get the patch in by the end of this week.

[Mads Ager (chromium), 2014-05-06 09:16:50]

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/forms/T...
File Source/core/html/forms/TextFieldInputType.cpp (right):

https://codereview.chromium.org/262093006/diff/60001/Source/core/html/forms/T...
Source/core/html/forms/TextFieldInputType.cpp:120:
spinButton->removeSpinButtonOwner();
On 2014/05/06 08:29:40, Erik Corry wrote:
> m_spinButtonOwner is a raw pointer, and this CL does not change that

This is safe because these nodes are in the same shadow tree and are therefore
live and die together (the owner is implicitly traced because they are in the
same tree). However, it is nasty and we should make the SpinButtonOwner traced.
We cannot do that at this point because one subclass of SpinButtonOwner is not
on the heap.

https://codereview.chromium.org/262093006/diff/70001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/262093006/diff/70001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2356: #if !ENABLE(OILPAN)
On 2014/05/06 09:02:25, Erik Corry wrote:
> Maybe just remove this assert completely since it's implied by node being in
> scope.

Done.

[Mads Ager (chromium), 2014-05-06 09:27:48]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-05-06 09:28:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ager@chromium.org/262093006/110001

[Mads Ager (chromium), 2014-05-06 10:03:23]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-05-06 10:03:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ager@chromium.org/262093006/130001

[Mads Ager (chromium), 2014-05-06 10:27:12]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-05-06 10:27:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ager@chromium.org/262093006/150001

[commit-bot: I haz the power, 2014-05-06 11:37:55]

Change committed as 173378

[haraken, 2014-05-06 15:59:40]

LGTM with comments. Most of the below comments are minor and the CL looks fine
to me.

(That being said, it seems you're a bit too rush. It wouldn't be a good idea to
land a CL without reaching a full consensus for this kind of complicated CL that
can affect non-oilpan code. We should also give kouhei@ a chance to comment on
the SVG part before landing. I understand that we want to avoid spending a
couple of reviewing cycles but we need to be careful when touching code that
affects other people :-)

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

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2251: dispose();

I do agree with you that Document::dispose() should be completely removed from
Oilpan. The fact that Document::dispose() is doing something implies that we're
changing a state of Document (which is sometimes observable from JavaScript)
when the Document is detached. When the Document is detached, we should do only
things that are necessary to detach the Document. All other things should die
together when the entire tree dies.

So shall we add a FIXME to mention that Document::dispose() should be completely
removed?

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

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/Documen...
Source/core/dom/Document.h:1323: WeakPtr<Document> m_contextDocument;

Nit: I haven't yet investigated this pointer, but WeakPtr tends to be tricky
because it does weak processing when the Document is destructed. You might want
to take care of this WeakPtr in a follow-up.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/Documen...
Source/core/dom/Document.h:1374: RawPtrWillBeMember<Document>
m_templateDocumentHost; // Manually managed weakref (backpointer from
m_templateDocument).

Nit: "weakref" is confusing. This is just a back, strong pointer.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/IdTarge...
File Source/core/dom/IdTargetObserver.h (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/IdTarge...
Source/core/dom/IdTargetObserver.h:39: virtual void unregister();

Thanks to your comments, I understand how unregister() works but it's
complicated. Shall we add a FIXME and mention that IdTargetObserver should be
moved to the heap and remove the explicit unregistration?

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (left):

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:2382: dispose();

I agree that we're going toward a right direction, but might need to keep in
mind that this change can cause some web-exposed behavior change. Before this
CL, Document was disposed when the last ref to the TreeScope is gone. After this
CL, Document is disposed when the Document is detached. This means that the
point where the Document is disposed becomes earlier than before, which can
cause web-exposed behavior change.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:57: : m_document(&document)

Probably we should announce to blink-dev@ that Oilpan is going to revert the
reference back to a raw pointer.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:93: m_fontSelector->clearDocument();

This is problematic but will be fixed in Gustav's CL shortly.

Nit: It would be great to have a FIXME for this kind of thing even if it is
going to be fixed in the next CL. We're doing complicated things and thus it's
hard to keep track of what we're doing.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/TreeSco...
File Source/core/dom/TreeScope.h (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.h:147: void destroyTreeScopeData();

You can add #if !ENABLE(OILPAN) to this method.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.h:158: virtual void dispose() { }

You can add #if !ENABLE(OILPAN) to this method and ShadowRoot::dispose(). In
oilpan, we only need Document::dispose().

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.h:164: #if SECURITY_ASSERT_ENABLED && !ENABLE(OILPAN)

This should be:

#if !ENABLE(OILPAN)
int refCount() const;
#if SECURITY_ASSERT_ENABLED
...
#else
...
#endif
#endif

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/shadow/...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/shadow/...
Source/core/dom/shadow/ElementShadow.cpp:173:
InspectorInstrumentation::willPopShadowRoot(shadowHost, oldRoot.get());

Add a FIXME and mention if this is OK to remove or not.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/shadow/...
File Source/core/dom/shadow/ElementShadow.h (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/shadow/...
Source/core/dom/shadow/ElementShadow.h:77: void removeDetachedShadowRoots();

You can add !ENABLE(OILPAN) to this method.

https://codereview.chromium.org/262093006/diff/150001/Source/core/html/FormAs...
File Source/core/html/FormAssociatedElement.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/html/FormAs...
Source/core/html/FormAssociatedElement.cpp:256: void
FormAssociatedElement::setFormAttributeTargetObserver(PassOwnPtr<FormAttributeTargetObserver>
newObserver)

This complexity will be removed once we move IdTargetObserver to the heap and
implement weak processing for the observer set.

https://codereview.chromium.org/262093006/diff/150001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:68: hideVisibleValidationMessage();

tkent-san: Would you confirm that this change is OK (as well as the
corresponding change in ValidationMessage)?

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGAnim...
File Source/core/svg/SVGAnimateElement.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGAnim...
Source/core/svg/SVGAnimateElement.cpp:54: clearAnimatedType(targetElement());

clearAnimatedType is doing a bunch of non-trivial stuff and I'm not sure if we
can simply remove all of them from the destructor. kouhei-san would know better.

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElementInstance.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
Source/core/svg/SVGElementInstance.cpp:142:
m_element->removeInstanceMapping(this);

In order to remove this, I think you need to:

- Make SVGElementRareData::m_elementInstances a hash set of WeakMembers.

- Add !ENABLE(OILPAN) to SVGElement::removeInstanceMapping.

What do you think?

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
Source/core/svg/SVGElementInstance.cpp:143: // DO NOT clear ref to m_element
because JavaScriptCore uses it for garbage collection

This comment is obsolete :)

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
Source/core/svg/SVGElementInstance.cpp:150: #if !ENABLE(OILPAN)

Can detach() be called in oilpan bulids? If no, you can remove the
!ENABLE(OILPAN) and instead add !ENABLE(OILPAN) to detach().

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElementInstance.h (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
Source/core/svg/SVGElementInstance.h:64: void detach();

You can add !ENABLE(OILPAN) to this.

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
Source/core/svg/SVGElementInstance.h:129: void removedLastRef();

Can we add !ENABLE(OILPAN) to this?

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
Source/core/svg/SVGElementInstance.h:141: friend void
removeDetachedChildrenInContainer(GenericNodeContainer&);

You can add !ENABLE(OILPAN) to this (but it won't be that helpful).

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/animati...
File Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.cpp:205:
m_timeContainer->unschedule(this, m_targetElement, m_attributeName);

It's not clear to me if we can remove this in oilpan builds. kouhei-san, do you
have any idea on this?

[Mads Ager (chromium), 2014-05-06 17:07:27]

On 2014/05/06 15:59:40, haraken wrote:
> LGTM with comments. Most of the below comments are minor and the CL looks fine
> to me.
> 
> (That being said, it seems you're a bit too rush. It wouldn't be a good idea
to
> land a CL without reaching a full consensus for this kind of complicated CL
that
> can affect non-oilpan code. We should also give kouhei@ a chance to comment on
> the SVG part before landing. I understand that we want to avoid spending a
> couple of reviewing cycles but we need to be careful when touching code that
> affects other people :-)

Thanks for your comments Haraken. I appreciate them and I will take action on
them over the next days. :-)

I'm sorry if you think this was too rushed. There are multiple changelists
blocked on getting the Node hierarchy traced and we cannot parallelize the work
until this change is in. So I made the call to land it now to unblock us. We
should continue discussions here on this change and I (and the rest of the team
in Aarhus now that we can parallelize) will continue working on it.

[tkent, 2014-05-07 00:41:18]

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/Contain...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:489: if (m_firstChild == &oldChild)
We should add operator==(const Member<Node>, const Node&) and so on like we have
them for RefPtr<Node>.  See Node.h.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:109: HTMLImportsController* import =
m_document->importsController();
We should have |Document& StyleEngine::document()|, and replace m_document usage
with it.

https://codereview.chromium.org/262093006/diff/150001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:68: hideVisibleValidationMessage();
On 2014/05/06 15:59:42, haraken wrote:
> 
> tkent-san: Would you confirm that this change is OK (as well as the
> corresponding change in ValidationMessage)?

This line is unnecessary.  One in removedFrom is enough because detached
elements can't have validation message.

[kouhei (in TOK), 2014-05-07 01:43:12]

svg/ reviewed.

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGAnim...
File Source/core/svg/SVGAnimateElement.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGAnim...
Source/core/svg/SVGAnimateElement.cpp:54: clearAnimatedType(targetElement());
On 2014/05/06 15:59:42, haraken wrote:
> 
> clearAnimatedType is doing a bunch of non-trivial stuff and I'm not sure if we
> can simply remove all of them from the destructor. kouhei-san would know
better.

Removing <animate> tag means it should stop the SMIL animation immediately. See
m_animator->stopAnimValAnimation() being called from clearAnimatedType. Is the
behavior kept in this patch?

I'm not sure if the behavior is covered by the current layouttests.

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElementInstance.h (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
Source/core/svg/SVGElementInstance.h:160: SVGUseElement* m_directUseElement;
RawPtrWillBeMember?

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/animati...
File Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.cpp:205:
m_timeContainer->unschedule(this, m_targetElement, m_attributeName);
On 2014/05/06 15:59:42, haraken wrote:
> 
> It's not clear to me if we can remove this in oilpan builds. kouhei-san, do
you
> have any idea on this?

I think we can move these to removedFrom.

[kouhei (in TOK), 2014-05-07 04:11:50]

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/propert...
File Source/core/svg/properties/SVGAnimatedProperty.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/propert...
Source/core/svg/properties/SVGAnimatedProperty.cpp:55: ASSERT(!isAnimating());
isAnimating() check only touches SVGAnimatedPropertyBase, so this should be
kept.

You are doing something wrong if this assert fails.

[Mads Ager (chromium), 2014-05-07 12:13:15]

Thanks for your comments guys. I have uploaded a code review which addresses
most of these and I have filed bug reports and/or put in FIXME comments for the
ones that I did not address yet.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/Contain...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:489: if (m_firstChild == &oldChild)
On 2014/05/07 00:41:19, tkent wrote:
> We should add operator==(const Member<Node>, const Node&) and so on like we
have
> them for RefPtr<Node>.  See Node.h.

Yes, we should, thanks!

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

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2251: dispose();
On 2014/05/06 15:59:42, haraken wrote:
> 
> I do agree with you that Document::dispose() should be completely removed from
> Oilpan. The fact that Document::dispose() is doing something implies that
we're
> changing a state of Document (which is sometimes observable from JavaScript)
> when the Document is detached. When the Document is detached, we should do
only
> things that are necessary to detach the Document. All other things should die
> together when the entire tree dies.
> 
> So shall we add a FIXME to mention that Document::dispose() should be
completely
> removed?

Yes, done.

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

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/Documen...
Source/core/dom/Document.h:1323: WeakPtr<Document> m_contextDocument;
On 2014/05/06 15:59:42, haraken wrote:
> 
> Nit: I haven't yet investigated this pointer, but WeakPtr tends to be tricky
> because it does weak processing when the Document is destructed. You might
want
> to take care of this WeakPtr in a follow-up.

I completely missed that one, well spotted. Added a FIXME to the code so I will
remember to revisit.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/Documen...
Source/core/dom/Document.h:1374: RawPtrWillBeMember<Document>
m_templateDocumentHost; // Manually managed weakref (backpointer from
m_templateDocument).
On 2014/05/06 15:59:42, haraken wrote:
> 
> Nit: "weakref" is confusing. This is just a back, strong pointer.

Yeah, I kept the comment for the non-oilpan build. Let me update the comment to
state that this is a manually managed back pointer in the non-oilpan build and
that the template document and its host live and die together with oilpan.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/IdTarge...
File Source/core/dom/IdTargetObserver.h (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/IdTarge...
Source/core/dom/IdTargetObserver.h:39: virtual void unregister();
On 2014/05/06 15:59:42, haraken wrote:
> 
> Thanks to your comments, I understand how unregister() works but it's
> complicated. Shall we add a FIXME and mention that IdTargetObserver should be
> moved to the heap and remove the explicit unregistration?

It is unclear to me exactly what the semantics should be. Currently, the
IdTargetObservers are in OwnPtrs, so the unregistration happens without delay
when the OwnPtr is overwritten. That behavior is maintained with the explicit
unregistration. We could make IdTargetObservers heap allocated and use weak
processing, but that would leave observers in the set until the next GC which is
a change in behavior from what is happening now.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (left):

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:2382: dispose();
On 2014/05/06 15:59:42, haraken wrote:
> 
> I agree that we're going toward a right direction, but might need to keep in
> mind that this change can cause some web-exposed behavior change. Before this
> CL, Document was disposed when the last ref to the TreeScope is gone. After
this
> CL, Document is disposed when the Document is detached. This means that the
> point where the Document is disposed becomes earlier than before, which can
> cause web-exposed behavior change.

Yes, I completely agree and I did think about that. We need to continue working
on making the dispose call that is currently happening in detach unnecessary.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:57: : m_document(&document)
On 2014/05/06 15:59:42, haraken wrote:
> 
> Probably we should announce to blink-dev@ that Oilpan is going to revert the
> reference back to a raw pointer.

Yes, we should write a message explaining the fact that references are a problem
for our transition types. I like Kent's suggestion on how to deal with this.
Replace the reference with a RawPtrWillBeMember and then have a accessor that
returns a reference. That way we can keep the code as if this was a reference.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:93: m_fontSelector->clearDocument();
On 2014/05/06 15:59:42, haraken wrote:
> 
> This is problematic but will be fixed in Gustav's CL shortly.
> 
> Nit: It would be great to have a FIXME for this kind of thing even if it is
> going to be fixed in the next CL. We're doing complicated things and thus it's
> hard to keep track of what we're doing.

Yes, I should have done that. Gustav's change is in, so all is well now.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:109: HTMLImportsController* import =
m_document->importsController();
On 2014/05/07 00:41:19, tkent wrote:
> We should have |Document& StyleEngine::document()|, and replace m_document
usage
> with it.

That is a good idea. That will be less intrusive in this code and will create a
pattern that is easy to recognize in the unlikely case that we abandon Oilpan
and have to restore the references. Will do!

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/TreeSco...
File Source/core/dom/TreeScope.h (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.h:147: void destroyTreeScopeData();
On 2014/05/06 15:59:42, haraken wrote:
> 
> You can add #if !ENABLE(OILPAN) to this method.

Done.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.h:158: virtual void dispose() { }
On 2014/05/06 15:59:42, haraken wrote:
> 
> You can add #if !ENABLE(OILPAN) to this method and ShadowRoot::dispose(). In
> oilpan, we only need Document::dispose().

I left this one in so I don't have to change the declaration of dispose in
Document. We can ifdef this one out when we can remove dispose on Document as
well.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.h:164: #if SECURITY_ASSERT_ENABLED && !ENABLE(OILPAN)
On 2014/05/06 15:59:42, haraken wrote:
> 
> This should be:
> 
> #if !ENABLE(OILPAN)
> int refCount() const;
> #if SECURITY_ASSERT_ENABLED
> ...
> #else
> ...
> #endif
> #endif

Done.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/shadow/...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/shadow/...
Source/core/dom/shadow/ElementShadow.cpp:173:
InspectorInstrumentation::willPopShadowRoot(shadowHost, oldRoot.get());
On 2014/05/06 15:59:42, haraken wrote:
> 
> Add a FIXME and mention if this is OK to remove or not.

I'll file a bug report to investigate and reach out to the devtools team for
this.

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/shadow/...
File Source/core/dom/shadow/ElementShadow.h (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/dom/shadow/...
Source/core/dom/shadow/ElementShadow.h:77: void removeDetachedShadowRoots();
On 2014/05/06 15:59:42, haraken wrote:
> 
> You can add !ENABLE(OILPAN) to this method.

Done.

https://codereview.chromium.org/262093006/diff/150001/Source/core/html/FormAs...
File Source/core/html/FormAssociatedElement.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/html/FormAs...
Source/core/html/FormAssociatedElement.cpp:256: void
FormAssociatedElement::setFormAttributeTargetObserver(PassOwnPtr<FormAttributeTargetObserver>
newObserver)
On 2014/05/06 15:59:42, haraken wrote:
> 
> This complexity will be removed once we move IdTargetObserver to the heap and
> implement weak processing for the observer set.

Yes, if that is the right thing to do. It will make the unregistration depend on
GC which it currently doesn't. With these changes, the unregistration happens
with explicit notifications and therefore matches what happened pre-oilpan.

https://codereview.chromium.org/262093006/diff/150001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:68: hideVisibleValidationMessage();
On 2014/05/07 00:41:19, tkent wrote:
> On 2014/05/06 15:59:42, haraken wrote:
> > 
> > tkent-san: Would you confirm that this change is OK (as well as the
> > corresponding change in ValidationMessage)?
> 
> This line is unnecessary.  One in removedFrom is enough because detached
> elements can't have validation message.

The question is what happens if the entire document dies without this element
being detached. So this element is still in the document and the entire document
structure is dead. However, it looks like documentDetached is called on the
client via the Page if the document is detached in that case and that clears any
validation messages. Does that sound right to you Kent?

Remove the call.

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGAnim...
File Source/core/svg/SVGAnimateElement.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGAnim...
Source/core/svg/SVGAnimateElement.cpp:54: clearAnimatedType(targetElement());
On 2014/05/07 01:43:13, kouhei wrote:
> On 2014/05/06 15:59:42, haraken wrote:
> > 
> > clearAnimatedType is doing a bunch of non-trivial stuff and I'm not sure if
we
> > can simply remove all of them from the destructor. kouhei-san would know
> better.
> 
> Removing <animate> tag means it should stop the SMIL animation immediately.
See
> m_animator->stopAnimValAnimation() being called from clearAnimatedType. Is the
> behavior kept in this patch?
> 
> I'm not sure if the behavior is covered by the current layouttests.

Kouhei, would it be possible to write a test case for this? Could you help me
either get a test case or create a patch with some instrumentation that will
show whether the animation is correctly stopped?

If you explicitly remove a SVGSMILElement you should get a 
SVGSMILElement::removedFrom call which I think should stop the animation. So
this code is only for the case where the SVGAnimateElement is still in the
hierarchy and is dying with the rest of the hierarchy.

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElementInstance.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
Source/core/svg/SVGElementInstance.cpp:142:
m_element->removeInstanceMapping(this);
On 2014/05/06 15:59:42, haraken wrote:
> 
> In order to remove this, I think you need to:
> 
> - Make SVGElementRareData::m_elementInstances a hash set of WeakMembers.
> 
> - Add !ENABLE(OILPAN) to SVGElement::removeInstanceMapping.
> 
> What do you think?

I changed it to use weak processing. That will keep the current behavior and
there is already a lot of weak processing in SVGElementRareData. I'm  not
entirely sure that we have to have the weak processing. It seems to me that the
element and the instances should live and die together instead?

I don't have to remove this code here. It is not called via the destructor
anymore and when it is called directly it should probably remove the instance
mapping explicitly.

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
Source/core/svg/SVGElementInstance.cpp:143: // DO NOT clear ref to m_element
because JavaScriptCore uses it for garbage collection
On 2014/05/06 15:59:42, haraken wrote:
> 
> This comment is obsolete :)

Yes it is, let me remove it. :)

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
Source/core/svg/SVGElementInstance.cpp:150: #if !ENABLE(OILPAN)
On 2014/05/06 15:59:42, haraken wrote:
> 
> Can detach() be called in oilpan bulids? If no, you can remove the
> !ENABLE(OILPAN) and instead add !ENABLE(OILPAN) to detach().

Yes, it can be called via the SVGUseElement.

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElementInstance.h (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
Source/core/svg/SVGElementInstance.h:64: void detach();
On 2014/05/06 15:59:42, haraken wrote:
> 
> You can add !ENABLE(OILPAN) to this.

It is still called via SVGUseElement at this point.

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
Source/core/svg/SVGElementInstance.h:129: void removedLastRef();
On 2014/05/06 15:59:42, haraken wrote:
> 
> Can we add !ENABLE(OILPAN) to this?

Yes, we should be able to do that.

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
Source/core/svg/SVGElementInstance.h:141: friend void
removeDetachedChildrenInContainer(GenericNodeContainer&);
On 2014/05/06 15:59:42, haraken wrote:
> 
> You can add !ENABLE(OILPAN) to this (but it won't be that helpful).

Done.

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/SVGElem...
Source/core/svg/SVGElementInstance.h:160: SVGUseElement* m_directUseElement;
On 2014/05/07 01:43:13, kouhei wrote:
> RawPtrWillBeMember?

Yes, and I replace the RefPtr from SVGElementInstance to SVGUseElement with a
Member as well so that they will live and die together when they are connected.

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/animati...
File Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.cpp:205:
m_timeContainer->unschedule(this, m_targetElement, m_attributeName);
On 2014/05/07 01:43:13, kouhei wrote:
> On 2014/05/06 15:59:42, haraken wrote:
> > 
> > It's not clear to me if we can remove this in oilpan builds. kouhei-san, do
> you
> > have any idea on this?
> 
> I think we can move these to removedFrom.

This is already in removedFrom which is why I believe it is safe to remove this
from here.

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/propert...
File Source/core/svg/properties/SVGAnimatedProperty.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/svg/propert...
Source/core/svg/properties/SVGAnimatedProperty.cpp:55: ASSERT(!isAnimating());
On 2014/05/07 04:11:51, kouhei wrote:
> isAnimating() check only touches SVGAnimatedPropertyBase, so this should be
> kept.
> 
> You are doing something wrong if this assert fails.

OK, thanks Kouhei. I'll add a FIXME here and file a bug report so I remember to
revisit this.

[tkent, 2014-05-07 13:56:53]

https://codereview.chromium.org/262093006/diff/150001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/262093006/diff/150001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:68: hideVisibleValidationMessage();
On 2014/05/07 12:13:16, Mads Ager (chromium) wrote:
> On 2014/05/07 00:41:19, tkent wrote:
> > On 2014/05/06 15:59:42, haraken wrote:
> > > 
> > > tkent-san: Would you confirm that this change is OK (as well as the
> > > corresponding change in ValidationMessage)?
> > 
> > This line is unnecessary.  One in removedFrom is enough because detached
> > elements can't have validation message.
> 
> The question is what happens if the entire document dies without this element
> being detached. So this element is still in the document and the entire
document
> structure is dead. However, it looks like documentDetached is called on the
> client via the Page if the document is detached in that case and that clears
any
> validation messages. Does that sound right to you Kent?
> 
> Remove the call.

Yeah, it's right.