Bug 178376 – Assert that Node::insertedInto doesn't fire an event

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

notifyNodeInsertedIntoTree currently asserts that it can dispatch an event.
This is completely wrong. We need to assert that it CANNOT dispatch an event.

[Ryosuke Niwa]

Created attachment 323995 [details]
Fixes the bug

[Radar WebKit Bug Importer]

<rdar://problem/35022857>

[zalan]

Comment on attachment 323995 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=323995&action=review

> Source/WebCore/ChangeLog:11
> +        No new tests since the existing tests cover the behavioral change.

Is it possible to come up with a test case that would be asserting on trunk (broken behavior) while it passes with this patch?

[Daniel Bates]

For completeness, the assert was originally added in <https://trac.webkit.org/changeset/189896/> (bug #149236). The condition of the assert was renamed and changed truthfulness over time.

[Ryosuke Niwa]

Comment on attachment 323995 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=323995&action=review

>> Source/WebCore/ChangeLog:11
>> +        No new tests since the existing tests cover the behavioral change.
> 
> Is it possible to come up with a test case that would be asserting on trunk (broken behavior) while it passes with this patch?

A whole bunch of tests in LayoutTests/dom would assert if I didn't have the fix for HTMLBodyElement and ProcessingInstruction.

[Daniel Bates]

Comment on attachment 323995 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=323995&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=178376

Reminder to add radar URL before landing. I know you imported this bug after you posted the patch.

> Source/WebCore/ChangeLog:17
> +        (WebCore::ProcessingInstruction::finishedInsertingSubtree): Extracted from insertedInto since

Out of curiosity, what existing test do we have for this change?

> Source/WebCore/ChangeLog:22
> +        (WebCore::HTMLBodyElement::finishedInsertingSubtree): Extracted from insertedInto since

Out of curiosity, what existing test do we have for this change?

[Ryosuke Niwa]

(In reply to Daniel Bates from comment #4)
> For completeness, the assert was originally added in
> <https://trac.webkit.org/changeset/189896/> (bug #149236). The condition of
> the assert was renamed and changed truthfulness over time.

The assertion existed before that:
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/dom/ContainerNodeAlgorithms.h?annotate=blame&rev=189895#L216

The RAII class was initially introduced in https://trac.webkit.org/changeset/130077 but the underlying assertion even predates the original refactoring to add insertInto in https://trac.webkit.org/changeset/114351 as you can see in https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/dom/ContainerNode.cpp?annotate=blame&rev=114350#L1012
or
https://trac.webkit.org/browser/webkit/trunk/WebCore/khtml/xml/dom_nodeimpl.cpp?annotate=blame&rev=11248#L500

In fact, this assertion was introduced 12 years ago in https://trac.webkit.org/changeset/10992.

[Ryosuke Niwa]

(In reply to Daniel Bates from comment #6)
> Comment on attachment 323995 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323995&action=review
>
> > Source/WebCore/ChangeLog:17
> > +        (WebCore::ProcessingInstruction::finishedInsertingSubtree): Extracted from insertedInto since
> 
> Out of curiosity, what existing test do we have for this change?

Replied in radar.

> > Source/WebCore/ChangeLog:22
> > +        (WebCore::HTMLBodyElement::finishedInsertingSubtree): Extracted from insertedInto since
> 
> Out of curiosity, what existing test do we have for this change?

Ditto.

[Ryosuke Niwa]

Thanks for the review!

[Ryosuke Niwa]

Committed r223458: <https://trac.webkit.org/changeset/223458>