117836 2013-06-20 08:21:17 -0700 REGRESSION(r149652): accessing items in .children via id doesn't work when element is not rooted in DOM tree 2013-08-02 15:47:17 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 115584 1 graouts rniwa benjamin commit-queue esprehn+autocc kling koivisto rniwa webkit-bug-importer oldest_to_newest 902157 0 205092 graouts 2013-06-20 08:21:17 -0700 Created attachment 205092 Reduced testcase Take an element such as this one: <div> <div id=foo></div> </div> If you call `.children.foo` on this element when it's not yet in the DOM tree, you'll get null, while the same code once the element is rooted in the DOM tree gets a reference to the <div id=foo> element. This is only failing in WebKit nightlies and works fine on Safari 6.0.5. 902158 1 graouts 2013-06-20 08:21:52 -0700 <rdar://problem/14118228> 902163 2 graouts 2013-06-20 08:43:43 -0700 I think the problem is coming from HTMLCollection::namedItem() and this code right at the start of the method: ContainerNode* root = rootContainerNode(); if (name.isEmpty() || !root) return 0; This was added in http://trac.webkit.org/changeset/138195 to fix https://webkit.org/b/105324. 902164 3 graouts 2013-06-20 08:46:38 -0700 Cc'ing Antti who committed the change that changed the behaviour here. Looking at the spec at http://www.w3.org/TR/dom/#dom-htmlcollection-nameditem, I don't see why we would perform such a check, it seems perfectly valid to operate without a root: "The namedItem(key) method must return the first element in the collection that falls into one of the following categories: - It is an a, applet, area, embed, form, frame, frameset, iframe, img, or object element, in the HTML namespace, with a name attribute equal to key, or, - It is an element with an ID equal to key. If no such elements are found, then the method must return null." 902181 4 graouts 2013-06-20 09:30:05 -0700 Actually, the case we hit is the final "else" clause in this chunk of code: TreeScope* treeScope = root->treeScope(); Element* candidate = 0; if (treeScope->hasElementWithId(name.impl())) { if (!treeScope->containsMultipleElementsWithId(name)) candidate = treeScope->getElementById(name); } else if (treeScope->hasElementWithName(name.impl())) { if (!treeScope->containsMultipleElementsWithName(name)) { candidate = treeScope->getElementByName(name); if (candidate && type() == DocAll && (!candidate->isHTMLElement() || !nameShouldBeVisibleInDocumentAll(toHTMLElement(candidate)))) candidate = 0; } } else return 0; So the treeScope is not returning true for `treeScope->hasElementWithId(name.impl())`, which is surprising in this case. 902184 5 graouts 2013-06-20 09:34:32 -0700 Indeed, the treeScope's m_elementsById map is empty. 902187 6 graouts 2013-06-20 09:42:44 -0700 Element::updateIdForTreeScope() is where the map of element IDs is populated, and it's not yet been called at the time we're trying to access the namedItem in our test case. We need to be sure we have an ID map before calling namedItem(), although this may need to happen in other places too. Maybe this should happen in TreeScope::hasElementWithId(). 902204 7 graouts 2013-06-20 10:38:26 -0700 Actually, this may have regressed with http://trac.webkit.org/changeset/149652 which fixed http://webkit.org/b/115584. 902225 8 graouts 2013-06-20 11:24:22 -0700 So the regression is indeed coming from http://trac.webkit.org/changeset/149652. The problem, I think, is that we don't get to register this id in the treeScope due to this early return in Element::insertedInto(): if (!insertionPoint->isInTreeScope()) return InsertionDone; As we hit this, we can't make the call to: updateIdForTreeScope(newScope, nullAtom, idValue); later on in the function. This needs to happen for the treeScope's m_elementsById to be populated with the expected elements when we eventually call HTMLCollection::namedItem(). 902229 9 rniwa 2013-06-20 11:32:12 -0700 If a node is not a descendent of the tree scope, then it shouldn't be in the hash map. Instead, we need to go through the slow path. 902310 10 205119 rniwa 2013-06-20 14:02:29 -0700 Created attachment 205119 Fixes the bug 902400 11 rniwa 2013-06-20 19:48:12 -0700 Taking it over. 902418 12 rniwa 2013-06-20 20:51:05 -0700 Committed r151821: <http://trac.webkit.org/changeset/151821> 205092 2013-06-20 08:21:17 -0700 2013-06-20 08:21:17 -0700 Reduced testcase test.html text/html 335 graouts PHNjcmlwdD4KCiAgICB2YXIgZGl2ID0gZG9jdW1lbnQuY3JlYXRlRWxlbWVudCgiZGl2Iik7CiAg ICBkaXYuaW5uZXJIVE1MID0gIjxkaXYgaWQ9Zm9vPjwvZGl2PiI7CgogICAgY29uc29sZS5sb2co IkJlZm9yZSBhcHBlbmRpbmciLCAhIWRpdi5jaGlsZHJlbi5mb28pOwoKICAgIHdpbmRvdy5hZGRF dmVudExpc3RlbmVyKCJET01Db250ZW50TG9hZGVkIiwgZnVuY3Rpb24oKSB7CiAgICAgICAgZG9j dW1lbnQuYm9keS5hcHBlbmRDaGlsZChkaXYpOwogICAgICAgIGNvbnNvbGUubG9nKCJBZnRlciBh cHBlbmRpbmciLCAhIWRpdi5jaGlsZHJlbi5mb28pOwogICAgfSk7Cgo8L3NjcmlwdD4= 205119 2013-06-20 14:02:29 -0700 2013-06-20 20:35:45 -0700 Fixes the bug bug-117836-20130620140038.patch text/plain 4298 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE1MTc5OSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIwIEBACisyMDEzLTA2LTIwICBSeW9zdWtl IE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIFJFR1JFU1NJT046IGFjY2Vzc2lu ZyBpdGVtcyBpbiAuY2hpbGRyZW4gdmlhIGlkIGRvZXNuJ3Qgd29yayB3aGVuIGVsZW1lbnQgaXMg bm90IHJvb3RlZCBpbiBET00gdHJlZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MTE3ODM2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgV2hlbiB0aGUgcm9vdCBub2RlIG9mIGEgSFRNTCBjb2xsZWN0aW9uIGlz IG5vdCBpbiB0aGUgZG9jdW1lbnQgb3IgaW4gYSBzaGFkb3cgdHJlZSwKKyAgICAgICAgd2Ugc2hv dWxkbid0IHVzZSBpdHMgdHJlZSBzY29wZSdzIGlkIGFuZCBuYW1lIG1hcHMgdG8gZmluZCBuYW1l IGdldHRlcnMuCisKKyAgICAgICAgQWx3YXlzIHVzZSB0aGUgc2xvdyBwYXRoIGluIHN1Y2ggY2Fz ZXMuCisKKyAgICAgICAgVGVzdDogZmFzdC9kb20vaHRtbGFsbGNvbGxlY3Rpb24tZGV0YWNoZWQt bm9kZS1jaGlsZHJlbi5odG1sCisKKyAgICAgICAgKiBodG1sL0hUTUxDb2xsZWN0aW9uLmNwcDoK KyAgICAgICAgKFdlYkNvcmU6OkhUTUxDb2xsZWN0aW9uOjpuYW1lZEl0ZW0pOgorCiAyMDEzLTA2 LTIwICBDb21taXQgUXVldWUgIDxjb21taXQtcXVldWVAd2Via2l0Lm9yZz4KIAogICAgICAgICBV bnJldmlld2VkLCByb2xsaW5nIG91dCByMTUxNDUxLgpJbmRleDogU291cmNlL1dlYkNvcmUvaHRt bC9IVE1MQ29sbGVjdGlvbi5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvaHRtbC9I VE1MQ29sbGVjdGlvbi5jcHAJKHJldmlzaW9uIDE1MTc4NSkKKysrIFNvdXJjZS9XZWJDb3JlL2h0 bWwvSFRNTENvbGxlY3Rpb24uY3BwCSh3b3JraW5nIGNvcHkpCkBAIC02MTUsNyArNjE1LDcgQEAg Tm9kZSogSFRNTENvbGxlY3Rpb246Om5hbWVkSXRlbShjb25zdCBBdAogICAgIGlmIChuYW1lLmlz RW1wdHkoKSB8fCAhcm9vdCkKICAgICAgICAgcmV0dXJuIDA7CiAKLSAgICBpZiAoIW92ZXJyaWRl c0l0ZW1BZnRlcigpKSB7CisgICAgaWYgKCFvdmVycmlkZXNJdGVtQWZ0ZXIoKSAmJiByb290LT5p c0luVHJlZVNjb3BlKCkpIHsKICAgICAgICAgVHJlZVNjb3BlKiB0cmVlU2NvcGUgPSByb290LT50 cmVlU2NvcGUoKTsKICAgICAgICAgRWxlbWVudCogY2FuZGlkYXRlID0gMDsKICAgICAgICAgaWYg KHRyZWVTY29wZS0+aGFzRWxlbWVudFdpdGhJZChuYW1lLmltcGwoKSkpIHsKSW5kZXg6IExheW91 dFRlc3RzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDE1MTc5OSkKKysrIExheW91dFRlc3RzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpA QCAtMSwzICsxLDE1IEBACisyMDEzLTA2LTIwICBSeW9zdWtlIE5pd2EgIDxybml3YUB3ZWJraXQu b3JnPgorCisgICAgICAgIFJFR1JFU1NJT046IGFjY2Vzc2luZyBpdGVtcyBpbiAuY2hpbGRyZW4g dmlhIGlkIGRvZXNuJ3Qgd29yayB3aGVuIGVsZW1lbnQgaXMgbm90IHJvb3RlZCBpbiBET00gdHJl ZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTE3ODM2 CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkIGEg cmVncmVzc2lvbiB0ZXN0IGZvciBuYW1lZCBnZXR0ZXIgZm9yIGEgZGV0YWNoZWQgZWxlbWVudC4K KworICAgICAgICAqIGZhc3QvZG9tL2h0bWxhbGxjb2xsZWN0aW9uLWRldGFjaGVkLW5vZGUtY2hp bGRyZW4tZXhwZWN0ZWQudHh0OiBBZGRlZC4KKyAgICAgICAgKiBmYXN0L2RvbS9odG1sYWxsY29s bGVjdGlvbi1kZXRhY2hlZC1ub2RlLWNoaWxkcmVuLmh0bWw6IEFkZGVkLgorCiAyMDEzLTA2LTIw ICBDb21taXQgUXVldWUgIDxjb21taXQtcXVldWVAd2Via2l0Lm9yZz4KIAogICAgICAgICBVbnJl dmlld2VkLCByb2xsaW5nIG91dCByMTUxNDUxLgpJbmRleDogTGF5b3V0VGVzdHMvZmFzdC9kb20v aHRtbGFsbGNvbGxlY3Rpb24tZGV0YWNoZWQtbm9kZS1jaGlsZHJlbi1leHBlY3RlZC50eHQKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gTGF5b3V0VGVzdHMvZmFzdC9kb20vaHRtbGFsbGNvbGxlY3Rpb24tZGV0YWNo ZWQtbm9kZS1jaGlsZHJlbi1leHBlY3RlZC50eHQJKHJldmlzaW9uIDApCisrKyBMYXlvdXRUZXN0 cy9mYXN0L2RvbS9odG1sYWxsY29sbGVjdGlvbi1kZXRhY2hlZC1ub2RlLWNoaWxkcmVuLWV4cGVj dGVkLnR4dAkod29ya2luZyBjb3B5KQpAQCAtMCwwICsxLDEzIEBACitUaGlzIHRlc3RzIHZlcmlm aWVzIHRoYXQgdGhlIG5hbWUgZ2V0dGVyIG9uIGVsZW1lbnQuY2hpbGRyZW4gd29ya3MgZXZlbiB3 aGVuIGVsZW1lbnQgaXMgbm90IGluIHRoZSBkb2N1bWVudAorCitPbiBzdWNjZXNzLCB5b3Ugd2ls bCBzZWUgYSBzZXJpZXMgb2YgIlBBU1MiIG1lc3NhZ2VzLCBmb2xsb3dlZCBieSAiVEVTVCBDT01Q TEVURSIuCisKKworUEFTUyBlbGVtZW50LmNoaWxkcmVuWydmb28nXSBpcyB1bmRlZmluZWQuCitQ QVNTIHNwYW4gPSBjcmVhdGVFbGVtZW50V2l0aElkKCdmb28nKTsgZWxlbWVudC5hcHBlbmRDaGls ZChzcGFuKTsgZWxlbWVudC5jaGlsZHJlblsnZm9vJ10gaXMgc3BhbgorUEFTUyBkb2N1bWVudC5h bGxbJ2ZvbyddIGlzIHVuZGVmaW5lZC4KK1BBU1MgZG9jdW1lbnQuYm9keS5hcHBlbmRDaGlsZChz cGFuKTsgZG9jdW1lbnQuYWxsWydmb28nXSBpcyBzcGFuCitQQVNTIHN1Y2Nlc3NmdWxseVBhcnNl ZCBpcyB0cnVlCisKK1RFU1QgQ09NUExFVEUKKwpJbmRleDogTGF5b3V0VGVzdHMvZmFzdC9kb20v aHRtbGFsbGNvbGxlY3Rpb24tZGV0YWNoZWQtbm9kZS1jaGlsZHJlbi5odG1sCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIExheW91dFRlc3RzL2Zhc3QvZG9tL2h0bWxhbGxjb2xsZWN0aW9uLWRldGFjaGVkLW5vZGUt Y2hpbGRyZW4uaHRtbAkocmV2aXNpb24gMCkKKysrIExheW91dFRlc3RzL2Zhc3QvZG9tL2h0bWxh bGxjb2xsZWN0aW9uLWRldGFjaGVkLW5vZGUtY2hpbGRyZW4uaHRtbAkod29ya2luZyBjb3B5KQpA QCAtMCwwICsxLDI3IEBACis8IURPQ1RZUEUgaHRtbD4KKzxodG1sPgorPGhlYWQ+Cis8bWV0YSBj aGFyc2V0PSJ1dGYtOCI+Cis8c2NyaXB0IHNyYz0iLi4vanMvcmVzb3VyY2VzL2pzLXRlc3QtcHJl LmpzIj48L3NjcmlwdD4KKzwvaGVhZD4KKzxib2R5PgorPHNjcmlwdD4KKworZGVzY3JpcHRpb24o IlRoaXMgdGVzdHMgdmVyaWZpZXMgdGhhdCB0aGUgbmFtZSBnZXR0ZXIgb24gZWxlbWVudC5jaGls ZHJlbiB3b3JrcyBldmVuIHdoZW4gZWxlbWVudCBpcyBub3QgaW4gdGhlIGRvY3VtZW50Iik7CisK K2Z1bmN0aW9uIGNyZWF0ZUVsZW1lbnRXaXRoSWQoaWQpIHsKKyAgICB2YXIgc3BhbiA9IGRvY3Vt ZW50LmNyZWF0ZUVsZW1lbnQoJ3NwYW4nKTsKKyAgICBzcGFuLmlkID0gJ2Zvbyc7CisgICAgcmV0 dXJuIHNwYW47Cit9CisKK3ZhciBlbGVtZW50ID0gZG9jdW1lbnQuY3JlYXRlRWxlbWVudCgnZGl2 Jyk7CitzaG91bGRCZVVuZGVmaW5lZCgiZWxlbWVudC5jaGlsZHJlblsnZm9vJ10iKTsKK3Nob3Vs ZEJlKCJzcGFuID0gY3JlYXRlRWxlbWVudFdpdGhJZCgnZm9vJyk7IGVsZW1lbnQuYXBwZW5kQ2hp bGQoc3Bhbik7IGVsZW1lbnQuY2hpbGRyZW5bJ2ZvbyddIiwgInNwYW4iKTsKK3Nob3VsZEJlVW5k ZWZpbmVkKCJkb2N1bWVudC5hbGxbJ2ZvbyddIik7CitzaG91bGRCZSgiZG9jdW1lbnQuYm9keS5h cHBlbmRDaGlsZChzcGFuKTsgZG9jdW1lbnQuYWxsWydmb28nXSIsICJzcGFuIik7CisKKzwvc2Ny aXB0PgorPHNjcmlwdCBzcmM9Ii4uL2pzL3Jlc291cmNlcy9qcy10ZXN0LXBvc3QuanMiPjwvc2Ny aXB0PgorPC9ib2R5PgorPC9odG1sPgo=