178376 2017-10-16 21:47:58 -0700 Assert that Node::insertedInto doesn't fire an event 2017-10-16 22:52:40 -0700 1 1 1 Unclassified WebKit DOM WebKit Nightly Build All All RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=149236 InRadar P2 Normal --- 1 rniwa rniwa buildbot cdumez dbates esprehn+autocc gyuyoung.kim jiewen_tan kangil.han koivisto webkit-bug-importer zalan oldest_to_newest 1361118 0 rniwa 2017-10-16 21:47:58 -0700 notifyNodeInsertedIntoTree currently asserts that it can dispatch an event. This is completely wrong. We need to assert that it CANNOT dispatch an event. 1361122 1 323995 rniwa 2017-10-16 21:56:43 -0700 Created attachment 323995 Fixes the bug 1361127 2 webkit-bug-importer 2017-10-16 22:15:03 -0700 <rdar://problem/35022857> 1361129 3 323995 zalan 2017-10-16 22:25:08 -0700 Comment on attachment 323995 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? 1361130 4 dbates 2017-10-16 22:29:34 -0700 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. 1361131 5 323995 rniwa 2017-10-16 22:33:12 -0700 Comment on attachment 323995 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. 1361133 6 323995 dbates 2017-10-16 22:40:40 -0700 Comment on attachment 323995 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? 1361135 7 rniwa 2017-10-16 22:43:41 -0700 (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. 1361137 8 rniwa 2017-10-16 22:46:01 -0700 (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. 1361138 9 rniwa 2017-10-16 22:52:33 -0700 Thanks for the review! 1361139 10 rniwa 2017-10-16 22:52:40 -0700 Committed r223458: <https://trac.webkit.org/changeset/223458> 323995 2017-10-16 21:56:43 -0700 2017-10-16 22:49:19 -0700 Fixes the bug bug-178376-20171016215642.patch text/plain 5223 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIyMzQ1NSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI4IEBACisyMDE3LTEwLTE2ICBSeW9zdWtl IE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIEFzc2VydCB0aGF0IE5vZGU6Omlu c2VydGVkSW50byBkb2Vzbid0IGZpcmUgYW4gZXZlbnQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE3ODM3NgorCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgIEZpeGVkIHRoZSBhc3NlcnRpb24gaW4gbm90aWZ5Q2hp bGROb2RlSW5zZXJ0ZWQgc2luY2UgdGhpcyBmdW5jdGlvbiBNVVNUIE5PVCBkaXNwYXRjaCBhbiBl dmVudCwKKyAgICAgICAgYW5kIG1vdmVkIGEgYnVuY2ggb2YgZXZlbnQtZGlzcGF0Y2hpbmcgY29k ZSBmcm9tIE5vZGU6Omluc2VydGVkSW50byBpbnRvIE5vZGU6OmZpbmlzaGVkSW5zZXJ0aW5nU3Vi dHJlZS4KKworICAgICAgICBObyBuZXcgdGVzdHMgc2luY2UgdGhlIGV4aXN0aW5nIHRlc3RzIGNv dmVyIHRoZSBiZWhhdmlvcmFsIGNoYW5nZS4KKworICAgICAgICAqIGRvbS9Db250YWluZXJOb2Rl QWxnb3JpdGhtcy5jcHA6CisgICAgICAgIChXZWJDb3JlOjpub3RpZnlDaGlsZE5vZGVJbnNlcnRl ZCk6IEZpeGVkIHRoZSBhc3NlcnRpb24uCisgICAgICAgICogZG9tL1Byb2Nlc3NpbmdJbnN0cnVj dGlvbi5jcHA6CisgICAgICAgIChXZWJDb3JlOjpQcm9jZXNzaW5nSW5zdHJ1Y3Rpb246Omluc2Vy dGVkSW50byk6CisgICAgICAgIChXZWJDb3JlOjpQcm9jZXNzaW5nSW5zdHJ1Y3Rpb246OmZpbmlz aGVkSW5zZXJ0aW5nU3VidHJlZSk6IEV4dHJhY3RlZCBmcm9tIGluc2VydGVkSW50byBzaW5jZQor ICAgICAgICBjaGVja1N0eWxlU2hlZXQgY2FuIGRpc3BhdGNoIGFuIGV2ZW50LgorICAgICAgICAq IGRvbS9Qcm9jZXNzaW5nSW5zdHJ1Y3Rpb24uaDoKKyAgICAgICAgKiBodG1sL0hUTUxCb2R5RWxl bWVudC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpIVE1MQm9keUVsZW1lbnQ6Omluc2VydGVkSW50 byk6CisgICAgICAgIChXZWJDb3JlOjpIVE1MQm9keUVsZW1lbnQ6OmZpbmlzaGVkSW5zZXJ0aW5n U3VidHJlZSk6IEV4dHJhY3RlZCBmcm9tIGluc2VydGVkSW50byBzaW5jZQorICAgICAgICBzZXRJ bnRlZ3JhbEF0dHJpYnV0ZSBjb3VsZCBkaXNwYXRjaCBET01BdHRyTW9kaWZpZWQgc3luY2hyb25v dXNseS4KKyAgICAgICAgKiBodG1sL0hUTUxCb2R5RWxlbWVudC5oOgorCiAyMDE3LTEwLTE2ICBK ZXIgTm9ibGUgIDxqZXIubm9ibGVAYXBwbGUuY29tPgogCiAgICAgICAgIEltYWdlRGVjb2RlckFW Rk9iakMgZmFpbHMgdG8gY3JlYXRlIG1vcmUgQ01TYW1wbGVCdWZmZXJzIGFmdGVyIGNyZWF0aW5n IGFib3V0IDMyTUIgd29ydGguCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9kb20vQ29udGFpbmVyTm9k ZUFsZ29yaXRobXMuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2RvbS9Db250YWlu ZXJOb2RlQWxnb3JpdGhtcy5jcHAJKHJldmlzaW9uIDIyMzQ0NSkKKysrIFNvdXJjZS9XZWJDb3Jl L2RvbS9Db250YWluZXJOb2RlQWxnb3JpdGhtcy5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTg1LDcg Kzg1LDcgQEAgc3RhdGljIHZvaWQgbm90aWZ5Tm9kZUluc2VydGVkSW50b1RyZWUoQwogCiB2b2lk IG5vdGlmeUNoaWxkTm9kZUluc2VydGVkKENvbnRhaW5lck5vZGUmIGluc2VydGlvblBvaW50LCBO b2RlJiBub2RlLCBOb2RlVmVjdG9yJiBwb3N0SW5zZXJ0aW9uTm90aWZpY2F0aW9uVGFyZ2V0cykK IHsKLSAgICBSRUxFQVNFX0FTU0VSVChOb0V2ZW50RGlzcGF0Y2hBc3NlcnRpb246OmlzRXZlbnRE aXNwYXRjaEFsbG93ZWRJblN1YnRyZWUoaW5zZXJ0aW9uUG9pbnQpKTsKKyAgICBOb0V2ZW50RGlz cGF0Y2hBc3NlcnRpb24gYXNzZXJ0Tm9FdmVudERpc3BhdGNoOwogCiAgICAgSW5zcGVjdG9ySW5z dHJ1bWVudGF0aW9uOjpkaWRJbnNlcnRET01Ob2RlKG5vZGUuZG9jdW1lbnQoKSwgbm9kZSk7CiAK SW5kZXg6IFNvdXJjZS9XZWJDb3JlL2RvbS9Qcm9jZXNzaW5nSW5zdHJ1Y3Rpb24uY3BwCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2RvbS9Qcm9jZXNzaW5nSW5zdHJ1Y3Rpb24uY3BwCShy ZXZpc2lvbiAyMjM0NDUpCisrKyBTb3VyY2UvV2ViQ29yZS9kb20vUHJvY2Vzc2luZ0luc3RydWN0 aW9uLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMjgwLDggKzI4MCwxMiBAQCBOb2RlOjpJbnNlcnRp b25Ob3RpZmljYXRpb25SZXF1ZXN0IFByb2NlCiAgICAgaWYgKCFpbnNlcnRpb25Qb2ludC5pc0Nv bm5lY3RlZCgpKQogICAgICAgICByZXR1cm4gSW5zZXJ0aW9uRG9uZTsKICAgICBkb2N1bWVudCgp LnN0eWxlU2NvcGUoKS5hZGRTdHlsZVNoZWV0Q2FuZGlkYXRlTm9kZSgqdGhpcywgbV9jcmVhdGVk QnlQYXJzZXIpOworICAgIHJldHVybiBJbnNlcnRpb25TaG91bGRDYWxsRmluaXNoZWRJbnNlcnRp bmdTdWJ0cmVlOworfQorCit2b2lkIFByb2Nlc3NpbmdJbnN0cnVjdGlvbjo6ZmluaXNoZWRJbnNl cnRpbmdTdWJ0cmVlKCkKK3sKICAgICBjaGVja1N0eWxlU2hlZXQoKTsKLSAgICByZXR1cm4gSW5z ZXJ0aW9uRG9uZTsKIH0KIAogdm9pZCBQcm9jZXNzaW5nSW5zdHJ1Y3Rpb246OnJlbW92ZWRGcm9t KENvbnRhaW5lck5vZGUmIGluc2VydGlvblBvaW50KQpJbmRleDogU291cmNlL1dlYkNvcmUvZG9t L1Byb2Nlc3NpbmdJbnN0cnVjdGlvbi5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2Rv bS9Qcm9jZXNzaW5nSW5zdHJ1Y3Rpb24uaAkocmV2aXNpb24gMjIzNDQ1KQorKysgU291cmNlL1dl YkNvcmUvZG9tL1Byb2Nlc3NpbmdJbnN0cnVjdGlvbi5oCSh3b3JraW5nIGNvcHkpCkBAIC01OSw2 ICs1OSw3IEBAIHByaXZhdGU6CiAgICAgUmVmPE5vZGU+IGNsb25lTm9kZUludGVybmFsKERvY3Vt ZW50JiwgQ2xvbmluZ09wZXJhdGlvbikgb3ZlcnJpZGU7CiAKICAgICBJbnNlcnRpb25Ob3RpZmlj YXRpb25SZXF1ZXN0IGluc2VydGVkSW50byhDb250YWluZXJOb2RlJikgb3ZlcnJpZGU7CisgICAg dm9pZCBmaW5pc2hlZEluc2VydGluZ1N1YnRyZWUoKSBvdmVycmlkZTsKICAgICB2b2lkIHJlbW92 ZWRGcm9tKENvbnRhaW5lck5vZGUmKSBvdmVycmlkZTsKIAogICAgIHZvaWQgY2hlY2tTdHlsZVNo ZWV0KCk7CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxCb2R5RWxlbWVudC5jcHAKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MQm9keUVsZW1lbnQuY3BwCShyZXZp c2lvbiAyMjM0NDUpCisrKyBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxCb2R5RWxlbWVudC5jcHAJ KHdvcmtpbmcgY29weSkKQEAgLTE5OSw3ICsxOTksMTQgQEAgTm9kZTo6SW5zZXJ0aW9uTm90aWZp Y2F0aW9uUmVxdWVzdCBIVE1MQgogICAgIGF1dG8qIG93bmVyRWxlbWVudCA9IGRvY3VtZW50KCku b3duZXJFbGVtZW50KCk7CiAgICAgaWYgKCFpczxIVE1MRnJhbWVFbGVtZW50QmFzZT4ob3duZXJF bGVtZW50KSkKICAgICAgICAgcmV0dXJuIEluc2VydGlvbkRvbmU7Ci0gICAgCisKKyAgICByZXR1 cm4gSW5zZXJ0aW9uU2hvdWxkQ2FsbEZpbmlzaGVkSW5zZXJ0aW5nU3VidHJlZTsKK30KKwordm9p ZCBIVE1MQm9keUVsZW1lbnQ6OmZpbmlzaGVkSW5zZXJ0aW5nU3VidHJlZSgpCit7CisgICAgYXV0 byogb3duZXJFbGVtZW50ID0gZG9jdW1lbnQoKS5vd25lckVsZW1lbnQoKTsKKyAgICBSRUxFQVNF X0FTU0VSVChpczxIVE1MRnJhbWVFbGVtZW50QmFzZT4ob3duZXJFbGVtZW50KSk7CiAgICAgYXV0 byYgb3duZXJGcmFtZUVsZW1lbnQgPSBkb3duY2FzdDxIVE1MRnJhbWVFbGVtZW50QmFzZT4oKm93 bmVyRWxlbWVudCk7CiAKICAgICAvLyBSZWFkIHZhbHVlcyBmcm9tIHRoZSBvd25lciBiZWZvcmUg c2V0dGluZyBhbnkgYXR0cmlidXRlcywgc2luY2Ugc2V0dGluZyBhbiBhdHRyaWJ1dGUgY2FuIHJ1 biBhcmJpdHJhcnkKQEAgLTIxMSw4ICsyMTgsNiBAQCBOb2RlOjpJbnNlcnRpb25Ob3RpZmljYXRp b25SZXF1ZXN0IEhUTUxCCiAgICAgICAgIHNldEludGVncmFsQXR0cmlidXRlKG1hcmdpbndpZHRo QXR0ciwgbWFyZ2luV2lkdGgpOwogICAgIGlmIChtYXJnaW5IZWlnaHQgIT0gLTEpCiAgICAgICAg IHNldEludGVncmFsQXR0cmlidXRlKG1hcmdpbmhlaWdodEF0dHIsIG1hcmdpbkhlaWdodCk7Ci0K LSAgICByZXR1cm4gSW5zZXJ0aW9uRG9uZTsKIH0KIAogYm9vbCBIVE1MQm9keUVsZW1lbnQ6Omlz VVJMQXR0cmlidXRlKGNvbnN0IEF0dHJpYnV0ZSYgYXR0cmlidXRlKSBjb25zdApJbmRleDogU291 cmNlL1dlYkNvcmUvaHRtbC9IVE1MQm9keUVsZW1lbnQuaAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv V2ViQ29yZS9odG1sL0hUTUxCb2R5RWxlbWVudC5oCShyZXZpc2lvbiAyMjM0NDUpCisrKyBTb3Vy Y2UvV2ViQ29yZS9odG1sL0hUTUxCb2R5RWxlbWVudC5oCSh3b3JraW5nIGNvcHkpCkBAIC00NSw2 ICs0NSw3IEBAIHByaXZhdGU6CiAgICAgdm9pZCBjb2xsZWN0U3R5bGVGb3JQcmVzZW50YXRpb25B dHRyaWJ1dGUoY29uc3QgUXVhbGlmaWVkTmFtZSYsIGNvbnN0IEF0b21pY1N0cmluZyYsIE11dGFi bGVTdHlsZVByb3BlcnRpZXMmKSBmaW5hbDsKIAogICAgIEluc2VydGlvbk5vdGlmaWNhdGlvblJl cXVlc3QgaW5zZXJ0ZWRJbnRvKENvbnRhaW5lck5vZGUmKSBmaW5hbDsKKyAgICB2b2lkIGZpbmlz aGVkSW5zZXJ0aW5nU3VidHJlZSgpIGZpbmFsOwogCiAgICAgYm9vbCBpc1VSTEF0dHJpYnV0ZShj b25zdCBBdHRyaWJ1dGUmKSBjb25zdCBmaW5hbDsKICAgICAK