Better handling of DocumentOrderedMap same-ID lookups during tree removals

Better handling of DocumentOrderedMap same-ID lookups during tree removals

Under select and unusal conditions, the removal of an element with ID
A might trigger further lookups of A from a TreeScope's DocumentOrderedMap
as part of handling the removal of that element. The tree and
DocumentOrderedMap is not in a consistent state to precisely handle such
lookups -- add machinery to spot that we're in a transitory state and
not trigger an assert over such failing lookups.

See code comments for further details.

R=tkent,esprehn
BUG=426005
Committed: https://crrev.com/c7a51bc56703af56f1c227fc8452913d86a840bc
Cr-Commit-Position: refs/heads/master@{#366066}

[sof, 2015-12-17 14:27:52]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-17 14:28:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1532103002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1532103002/1

[sof, 2015-12-17 15:45:41]

sigbjornf@opera.com changed reviewers:
+ esprehn@chromium.org, tkent@chromium.org

[sof, 2015-12-17 15:45:41]

Please take a look.

Ripping out the assert considered as hiding too many unexpected failures, so
work with it instead for this particular (& unusual) case.

[tkent, 2015-12-18 00:30:57]

lgtm

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DocumentOrderedMap.h (right):

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DocumentOrderedMap.h:94: const AtomicString*
m_removingId;
Can you make this |AtomicString| and check AtomicString::isNull() instead of
nullptr?

[esprehn, 2015-12-18 00:42:26]

This seems bad, we're covering up that we do an entire tree walk looking for an
id that doesn't exist...

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DocumentOrderedMap.cpp (right):

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DocumentOrderedMap.cpp:146: // register the
element ID it is in the process of removing.
this is bad though, it means in those cases we walk the entire tree looking for
an element with that id and never find it, so we might walk the entire document
which is slow.

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DocumentOrderedMap.h (right):

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DocumentOrderedMap.h:66: void
willBeRemovingId(const AtomicString*);
willRemoveId()

no "Be"

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DocumentOrderedMap.h:70:
DocumentOrderedMap();
we should declare the destructor too

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DocumentOrderedMap.h:94: const AtomicString*
m_removingId;
On 2015/12/18 at 00:30:57, tkent wrote:
> Can you make this |AtomicString| and check AtomicString::isNull() instead of
nullptr?

+1

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TreeScope.cpp:196:
RemovingElementIdScope(DocumentOrderedMap* elementsById, const AtomicString& id)
reference for elementsById

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TreeScope.cpp:199:
m_elementsById->willBeRemovingId(&id);
no ptr

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TreeScope.cpp:224: // unusual and unexpected
lookups.
this doesn't seem right, can we notify observers afterwards instead?

[sof, 2015-12-18 06:20:01]

On 2015/12/18 00:42:26, esprehn wrote:
> This seems bad, we're covering up that we do an entire tree walk looking for
an
> id that doesn't exist...
> 

We have no way of readily knowing where that duplicate ID might reside, as a
child of the removed node or somewhere else.

[sof, 2015-12-18 07:31:53]

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DocumentOrderedMap.cpp (right):

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DocumentOrderedMap.cpp:146: // register the
element ID it is in the process of removing.
On 2015/12/18 00:42:26, esprehn wrote:
> this is bad though, it means in those cases we walk the entire tree looking
for
> an element with that id and never find it, so we might walk the entire
document
> which is slow.

The context is worth not losing sight of -- this is for documents with duplicate
IDs, with a <form> being id-associated with one of them.

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DocumentOrderedMap.h (right):

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DocumentOrderedMap.h:66: void
willBeRemovingId(const AtomicString*);
On 2015/12/18 00:42:26, esprehn wrote:
> willRemoveId()
> 
> no "Be"

Renamed to willRemoveId().

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DocumentOrderedMap.h:70:
DocumentOrderedMap();
On 2015/12/18 00:42:26, esprehn wrote:
> we should declare the destructor too

Done, but see below.

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DocumentOrderedMap.h:94: const AtomicString*
m_removingId;
On 2015/12/18 00:30:57, tkent wrote:
> Can you make this |AtomicString| and check AtomicString::isNull() instead of
> nullptr?

Done that; slight downside with such a design is it leaves you with a
non-trivial finalizer on the Oilpan side.

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TreeScope.cpp:196:
RemovingElementIdScope(DocumentOrderedMap* elementsById, const AtomicString& id)
On 2015/12/18 00:42:26, esprehn wrote:
> reference for elementsById

Done.

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TreeScope.cpp:199:
m_elementsById->willBeRemovingId(&id);
On 2015/12/18 00:42:26, esprehn wrote:
> no ptr

No; this has to be a Member<>.

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TreeScope.cpp:224: // unusual and unexpected
lookups.
On 2015/12/18 00:42:26, esprehn wrote:
> this doesn't seem right, can we notify observers afterwards instead?

You would have to batch up all IDs having been dropped as a result of removing
element E (and its children), coordinated with mutation event dispatching
somehow, so not an easy thing to get right. The complexity isn't worth it.

[esprehn, 2015-12-18 07:53:40]

lgtm

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/1532103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TreeScope.cpp:199:
m_elementsById->willBeRemovingId(&id);
On 2015/12/18 at 07:31:53, sof wrote:
> On 2015/12/18 00:42:26, esprehn wrote:
> > no ptr
> 
> No; this has to be a Member<>

I was talking about the &id,  don't pass pointers to String or AtomicString.

[sof, 2015-12-18 08:07:42]

Description was changed from

==========
Better handling of DocumentOrderedMap same-ID lookups during tree removals

Under select and unusal conditions, the removal of an element with ID
A might trigger further lookups of A from a TreeScope's DocumentOrderedMap
as part of handling the removal of that element. The tree and
DocumentOrderedMap is not in a consistent state to precisely handle such
lookups -- add machinery to spot that we're in a transitory state and
not trigger an assert over such failing lookups.

See code comments for further details.

R=
BUG=426005
==========

to

==========
Better handling of DocumentOrderedMap same-ID lookups during tree removals

Under select and unusal conditions, the removal of an element with ID
A might trigger further lookups of A from a TreeScope's DocumentOrderedMap
as part of handling the removal of that element. The tree and
DocumentOrderedMap is not in a consistent state to precisely handle such
lookups -- add machinery to spot that we're in a transitory state and
not trigger an assert over such failing lookups.

See code comments for further details.

R=tkent,esprehn
BUG=426005
==========

[sof, 2015-12-18 08:07:54]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-12-18 08:07:54]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1532103002/#ps70001
(title: "review updates, part2")

[commit-bot: I haz the power, 2015-12-18 08:08:44]

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

[commit-bot: I haz the power, 2015-12-18 09:34:00]

Description was changed from

==========
Better handling of DocumentOrderedMap same-ID lookups during tree removals

Under select and unusal conditions, the removal of an element with ID
A might trigger further lookups of A from a TreeScope's DocumentOrderedMap
as part of handling the removal of that element. The tree and
DocumentOrderedMap is not in a consistent state to precisely handle such
lookups -- add machinery to spot that we're in a transitory state and
not trigger an assert over such failing lookups.

See code comments for further details.

R=tkent,esprehn
BUG=426005
==========

to

==========
Better handling of DocumentOrderedMap same-ID lookups during tree removals

Under select and unusal conditions, the removal of an element with ID
A might trigger further lookups of A from a TreeScope's DocumentOrderedMap
as part of handling the removal of that element. The tree and
DocumentOrderedMap is not in a consistent state to precisely handle such
lookups -- add machinery to spot that we're in a transitory state and
not trigger an assert over such failing lookups.

See code comments for further details.

R=tkent,esprehn
BUG=426005
==========

[commit-bot: I haz the power, 2015-12-18 09:34:01]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2015-12-18 09:35:19]

Description was changed from

==========
Better handling of DocumentOrderedMap same-ID lookups during tree removals

Under select and unusal conditions, the removal of an element with ID
A might trigger further lookups of A from a TreeScope's DocumentOrderedMap
as part of handling the removal of that element. The tree and
DocumentOrderedMap is not in a consistent state to precisely handle such
lookups -- add machinery to spot that we're in a transitory state and
not trigger an assert over such failing lookups.

See code comments for further details.

R=tkent,esprehn
BUG=426005
==========

to

==========
Better handling of DocumentOrderedMap same-ID lookups during tree removals

Under select and unusal conditions, the removal of an element with ID
A might trigger further lookups of A from a TreeScope's DocumentOrderedMap
as part of handling the removal of that element. The tree and
DocumentOrderedMap is not in a consistent state to precisely handle such
lookups -- add machinery to spot that we're in a transitory state and
not trigger an assert over such failing lookups.

See code comments for further details.

R=tkent,esprehn
BUG=426005

Committed: https://crrev.com/c7a51bc56703af56f1c227fc8452913d86a840bc
Cr-Commit-Position: refs/heads/master@{#366066}
==========

[commit-bot: I haz the power, 2015-12-18 09:35:20]

Patchset 5 (id:??) landed as
https://crrev.com/c7a51bc56703af56f1c227fc8452913d86a840bc
Cr-Commit-Position: refs/heads/master@{#366066}