notifyNodeInsertedIntoTree currently asserts that it can dispatch an event. This is completely wrong. We need to assert that it CANNOT dispatch an event.
Created attachment 323995 [details] Fixes the bug
<rdar://problem/35022857>
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?
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.
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.
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?
(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.
(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.
Thanks for the review!
Committed r223458: <https://trac.webkit.org/changeset/223458>