Accurately update Document node counts on disposing shadow roots.

Accurately update Document node counts on disposing shadow roots.

When cleaning out a ShadowRoot's child Nodes as part of its disposal,
the child Nodes will in some cases not have other references than the
shadow root. If so, the Node will be summarily deleted without being
notified of it being removed from its DOM tree first. By calling its
removedFrom().

As a Document need to maintain an accurate attached node count, arrange
for the otherwise-unreferenced Node's document to be decremented.

R=
BUG=521520
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201044

[sof, 2015-08-22 18:54:19]

sigbjornf@opera.com changed reviewers:
+ haraken@chromium.org, keishi@chromium.org

[sof, 2015-08-22 18:54:20]

please take a look.

[haraken, 2015-08-23 01:49:44]

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

https://codereview.chromium.org/1309083002/diff/20001/Source/core/dom/Documen...
Source/core/dom/Document.h:1038: if (isDisposed())

I'm just curious but is it possible that insertedInto/removedFrom is called for
a document that has been already disposed?

Also Document::dispose() is removed in oilpan. Are you intentionally using
isDisposed(), or should this be isDetached()?

[sof, 2015-08-23 05:36:54]

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

https://codereview.chromium.org/1309083002/diff/20001/Source/core/dom/Documen...
Source/core/dom/Document.h:1038: if (isDisposed())
On 2015/08/23 01:49:44, haraken wrote:
> 
> I'm just curious but is it possible that insertedInto/removedFrom is called
for
> a document that has been already disposed?
> 
> Also Document::dispose() is removed in oilpan. Are you intentionally using
> isDisposed(), or should this be isDetached()?

We wouldn't be here if removedFrom() couldn't be called on a disposed document;
see associated bug for stacktrace.

And if removedFrom() can be called, then I don't see anything fundamentally that
prevents insertedFrom() (if that's the only place node incrementing ends up
being done) from being invoked either in a disposed state. Hence, making it
behave consistently in a disposed state appears preferable. See ps#1 for an
alternative.

This is only for disposed states (as that's where the node count is reset); I
can make this !ENABLE(OILPAN), if preferable.

[haraken, 2015-08-23 13:17:42]

On 2015/08/23 05:36:54, sof wrote:
>
https://codereview.chromium.org/1309083002/diff/20001/Source/core/dom/Document.h
> File Source/core/dom/Document.h (right):
> 
>
https://codereview.chromium.org/1309083002/diff/20001/Source/core/dom/Documen...
> Source/core/dom/Document.h:1038: if (isDisposed())
> On 2015/08/23 01:49:44, haraken wrote:
> > 
> > I'm just curious but is it possible that insertedInto/removedFrom is called
> for
> > a document that has been already disposed?
> > 
> > Also Document::dispose() is removed in oilpan. Are you intentionally using
> > isDisposed(), or should this be isDetached()?
> 
> We wouldn't be here if removedFrom() couldn't be called on a disposed
document;
> see associated bug for stacktrace.
> 
> And if removedFrom() can be called, then I don't see anything fundamentally
that
> prevents insertedFrom() (if that's the only place node incrementing ends up
> being done) from being invoked either in a disposed state. Hence, making it
> behave consistently in a disposed state appears preferable. See ps#1 for an
> alternative.
> 
> This is only for disposed states (as that's where the node count is reset); I
> can make this !ENABLE(OILPAN), if preferable.

Thanks for the clarification. Makes sense.

Would it make more sense to reset m_nodeCount in Document::detach (instead of
Document::dispose) and change the check to use isDetached()? It seems strange to
reset m_nodeCount in code that is enabled only in non-oilpan.

[sof, 2015-08-23 13:34:18]

On 2015/08/23 13:17:42, haraken wrote:
> On 2015/08/23 05:36:54, sof wrote:
> >
>
https://codereview.chromium.org/1309083002/diff/20001/Source/core/dom/Document.h
> > File Source/core/dom/Document.h (right):
> > 
> >
>
https://codereview.chromium.org/1309083002/diff/20001/Source/core/dom/Documen...
> > Source/core/dom/Document.h:1038: if (isDisposed())
> > On 2015/08/23 01:49:44, haraken wrote:
> > > 
> > > I'm just curious but is it possible that insertedInto/removedFrom is
called
> > for
> > > a document that has been already disposed?
> > > 
> > > Also Document::dispose() is removed in oilpan. Are you intentionally using
> > > isDisposed(), or should this be isDetached()?
> > 
> > We wouldn't be here if removedFrom() couldn't be called on a disposed
> document;
> > see associated bug for stacktrace.
> > 
> > And if removedFrom() can be called, then I don't see anything fundamentally
> that
> > prevents insertedFrom() (if that's the only place node incrementing ends up
> > being done) from being invoked either in a disposed state. Hence, making it
> > behave consistently in a disposed state appears preferable. See ps#1 for an
> > alternative.
> > 
> > This is only for disposed states (as that's where the node count is reset);
I
> > can make this !ENABLE(OILPAN), if preferable.
> 
> Thanks for the clarification. Makes sense.
> 
> Would it make more sense to reset m_nodeCount in Document::detach (instead of
> Document::dispose) and change the check to use isDetached()? It seems strange
to
> reset m_nodeCount in code that is enabled only in non-oilpan.

I would prefer not to change that here, as once Oilpan is enabled always this
wrinkle can be removed.

[haraken, 2015-08-23 13:42:02]

On 2015/08/23 13:34:18, sof wrote:
> On 2015/08/23 13:17:42, haraken wrote:
> > On 2015/08/23 05:36:54, sof wrote:
> > >
> >
>
https://codereview.chromium.org/1309083002/diff/20001/Source/core/dom/Document.h
> > > File Source/core/dom/Document.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1309083002/diff/20001/Source/core/dom/Documen...
> > > Source/core/dom/Document.h:1038: if (isDisposed())
> > > On 2015/08/23 01:49:44, haraken wrote:
> > > > 
> > > > I'm just curious but is it possible that insertedInto/removedFrom is
> called
> > > for
> > > > a document that has been already disposed?
> > > > 
> > > > Also Document::dispose() is removed in oilpan. Are you intentionally
using
> > > > isDisposed(), or should this be isDetached()?
> > > 
> > > We wouldn't be here if removedFrom() couldn't be called on a disposed
> > document;
> > > see associated bug for stacktrace.
> > > 
> > > And if removedFrom() can be called, then I don't see anything
fundamentally
> > that
> > > prevents insertedFrom() (if that's the only place node incrementing ends
up
> > > being done) from being invoked either in a disposed state. Hence, making
it
> > > behave consistently in a disposed state appears preferable. See ps#1 for
an
> > > alternative.
> > > 
> > > This is only for disposed states (as that's where the node count is
reset);
> I
> > > can make this !ENABLE(OILPAN), if preferable.
> > 
> > Thanks for the clarification. Makes sense.
> > 
> > Would it make more sense to reset m_nodeCount in Document::detach (instead
of
> > Document::dispose) and change the check to use isDetached()? It seems
strange
> to
> > reset m_nodeCount in code that is enabled only in non-oilpan.
> 
> I would prefer not to change that here, as once Oilpan is enabled always this
> wrinkle can be removed.

LGTM

[sof, 2015-08-23 16:49:37]

On 2015/08/23 13:42:02, haraken wrote:
> On 2015/08/23 13:34:18, sof wrote:
> > On 2015/08/23 13:17:42, haraken wrote:
> > > On 2015/08/23 05:36:54, sof wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1309083002/diff/20001/Source/core/dom/Document.h
> > > > File Source/core/dom/Document.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1309083002/diff/20001/Source/core/dom/Documen...
> > > > Source/core/dom/Document.h:1038: if (isDisposed())
> > > > On 2015/08/23 01:49:44, haraken wrote:
> > > > > 
> > > > > I'm just curious but is it possible that insertedInto/removedFrom is
> > called
> > > > for
> > > > > a document that has been already disposed?
> > > > > 
> > > > > Also Document::dispose() is removed in oilpan. Are you intentionally
> using
> > > > > isDisposed(), or should this be isDetached()?
> > > > 
> > > > We wouldn't be here if removedFrom() couldn't be called on a disposed
> > > document;
> > > > see associated bug for stacktrace.
> > > > 
> > > > And if removedFrom() can be called, then I don't see anything
> fundamentally
> > > that
> > > > prevents insertedFrom() (if that's the only place node incrementing ends
> up
> > > > being done) from being invoked either in a disposed state. Hence, making
> it
> > > > behave consistently in a disposed state appears preferable. See ps#1 for
> an
> > > > alternative.
> > > > 
> > > > This is only for disposed states (as that's where the node count is
> reset);
> > I
> > > > can make this !ENABLE(OILPAN), if preferable.
> > > 
> > > Thanks for the clarification. Makes sense.
> > > 
> > > Would it make more sense to reset m_nodeCount in Document::detach (instead
> of
> > > Document::dispose) and change the check to use isDetached()? It seems
> strange
> > to
> > > reset m_nodeCount in code that is enabled only in non-oilpan.
> > 
> > I would prefer not to change that here, as once Oilpan is enabled always
this
> > wrinkle can be removed.
> 
> LGTM

Ah, the clearing of m_nodeCount in Document::dispose() is something that
https://codereview.chromium.org/1252683003/ added. I had assumed it was some
hard-earned, older change that brought it. We can do better..

[sof, 2015-08-23 18:18:57]

On 2015/08/23 16:49:37, sof wrote:
>
...
>
> Ah, the clearing of m_nodeCount in Document::dispose() is something that
> https://codereview.chromium.org/1252683003/ added. I had assumed it was some
> hard-earned, older change that brought it. We can do better..

Changed to handle the fast-path disposal of detached nodes instead + sync'ed the
description.

[haraken, 2015-08-23 23:30:16]

On 2015/08/23 18:18:57, sof wrote:
> On 2015/08/23 16:49:37, sof wrote:
> >
> ...
> >
> > Ah, the clearing of m_nodeCount in Document::dispose() is something that
> > https://codereview.chromium.org/1252683003/ added. I had assumed it was some
> > hard-earned, older change that brought it. We can do better..
> 
> Changed to handle the fast-path disposal of detached nodes instead + sync'ed
the
> description.

Thanks, LGTM

[haraken, 2015-08-23 23:30:22]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-08-23 23:30:26]

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

[commit-bot: I haz the power, 2015-08-23 23:34:30]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201044