Summary: | Assert that Node::insertedInto doesn't fire an event | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||
Component: | DOM | Assignee: | Ryosuke Niwa <rniwa> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | buildbot, cdumez, dbates, esprehn+autocc, gyuyoung.kim, jiewen_tan, kangil.han, koivisto, webkit-bug-importer, zalan | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=149236 | ||||||
Attachments: |
|
Description
Ryosuke Niwa
2017-10-16 21:47:58 PDT
Created attachment 323995 [details]
Fixes the bug
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> |