109976 2013-02-15 15:00:16 -0800 Calling DOM Element.attributes shouldn't force creation of ElementData. 2013-03-15 12:01:14 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 110026 109505 1 kling kling cmarcelo darin kling ojan.autocc rniwa webkit.review.bot oldest_to_newest 834407 0 kling 2013-02-15 15:00:16 -0800 We shouldn't construct ElementData "just to have some" when Element.attributes is called via DOM API. 834409 1 188649 kling 2013-02-15 15:03:05 -0800 Created attachment 188649 Patch 834468 2 188649 darin 2013-02-15 16:21:41 -0800 Comment on attachment 188649 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188649&action=review > Source/WebCore/dom/Element.cpp:-316 > - ensureElementDataWithSynchronizedAttributes(); Why was this here before? Was this just a misunderstanding? 834469 3 kling 2013-02-15 16:23:12 -0800 (In reply to comment #2) > (From update of attachment 188649 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=188649&action=review > > > Source/WebCore/dom/Element.cpp:-316 > > - ensureElementDataWithSynchronizedAttributes(); > > Why was this here before? Was this just a misunderstanding? Yeah looks like it. Though if we just removed that, NamedNodeMap::length() would crash with a null pointer dereference in Element::attributeCount(). 834546 4 188649 webkit.review.bot 2013-02-15 17:40:59 -0800 Comment on attachment 188649 Patch Clearing flags on attachment: 188649 Committed r143076: <http://trac.webkit.org/changeset/143076> 834547 5 webkit.review.bot 2013-02-15 17:41:03 -0800 All reviewed patches have been landed. Closing bug. 834792 6 rniwa 2013-02-16 13:22:16 -0800 This patch caused a crash on acid3: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=http%2Ftests%2Fmisc%2Facid3.html 834794 7 rniwa 2013-02-16 13:23:55 -0800 http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r143107%20(6917)/http/tests/misc/acid3-crash-log.txt Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x000000010e1e9c8b WebCore::NamedNodeMap::removeNamedItemNS(WTF::AtomicString const&, WTF::AtomicString const&, int&) + 59 (Element.h:88) 1 com.apple.WebCore 0x000000010dff54e6 WebCore::jsNamedNodeMapPrototypeFunctionRemoveNamedItemNS(JSC::ExecState*) + 518 (JSNamedNodeMap.cpp:367) 2 ??? 0x00002848d2001045 0 + 44293225975877 3 com.apple.JavaScriptCore 0x000000010d0b9693 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 611 (JSCJSValueInlines.h:363) 4 com.apple.JavaScriptCore 0x000000010cfbe765 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 69 (CallData.cpp:40) 5 com.apple.WebCore 0x000000010de6595e WebCore::JSMainThreadExecState::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 190 (JSMainThreadExecState.h:56) 6 com.apple.WebCore 0x000000010e3eee51 WebCore::ScheduledAction::executeFunctionInContext(JSC::JSGlobalObject*, JSC::JSValue, WebCore::ScriptExecutionContext*) + 529 (ScheduledAction.cpp:112) 7 com.apple.WebCore 0x000000010e3eeabc WebCore::ScheduledAction::execute(WebCore::Document*) + 156 (ScheduledAction.cpp:134) 8 com.apple.WebCore 0x000000010db6cba4 WebCore::DOMTimer::fired() + 388 (InspectorInstrumentation.h:289) 9 com.apple.WebCore 0x000000010e5c60af WebCore::ThreadTimers::sharedTimerFiredInternal() + 175 (ThreadTimers.cpp:132) 10 com.apple.WebCore 0x000000010e452f03 WebCore::timerFired(__CFRunLoopTimer*, void*) + 51 (SharedTimerMac.mm:167) 11 com.apple.CoreFoundation 0x00007fff8ee2eda4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20 12 com.apple.CoreFoundation 0x00007fff8ee2e8bd __CFRunLoopDoTimer + 557 13 com.apple.CoreFoundation 0x00007fff8ee14099 __CFRunLoopRun + 1513 834797 8 webkit.review.bot 2013-02-16 13:28:21 -0800 Re-opened since this is blocked by bug 110026 834800 9 darin 2013-02-16 13:31:00 -0800 Looks like the Acid3 bug is the same thing we are discussing in bug 110019. Too bad we missed our chance to just fix that instead of rolling this out! 834802 10 kling 2013-02-16 13:33:11 -0800 (In reply to comment #9) > Looks like the Acid3 bug is the same thing we are discussing in bug 110019. Too bad we missed our chance to just fix that instead of rolling this out! We don't necessarily have to roll this out, it's just my personal preference to roll out and roll back in with everything in the right place when possible. Mostly because it makes SVN archaeology a bit easier later on. 834806 11 kling 2013-02-16 13:35:44 -0800 (In reply to comment #10) > (In reply to comment #9) > > Looks like the Acid3 bug is the same thing we are discussing in bug 110019. Too bad we missed our chance to just fix that instead of rolling this out! > > We don't necessarily have to roll this out, it's just my personal preference to roll out and roll back in with everything in the right place when possible. Mostly because it makes SVN archaeology a bit easier later on. That said, there's already a copy of ACID3 in the LayoutTests tree, so my comment about adding a test in 110019 (the sole reason for rejecting that patch) doesn't even apply. I'll r+, cq+ that instead. 188649 2013-02-15 15:03:05 -0800 2013-02-15 17:40:59 -0800 Patch bug-109976.diff text/plain 2086 kling ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBmOGU3YjBiLi44MjM5NmRhIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMjUg QEAKIDIwMTMtMDItMTUgIEFuZHJlYXMgS2xpbmcgIDxha2xpbmdAYXBwbGUuY29tPgogCisgICAg ICAgIENhbGxpbmcgRE9NIEVsZW1lbnQuYXR0cmlidXRlcyBzaG91bGRuJ3QgZm9yY2UgY3JlYXRp b24gb2YgRWxlbWVudERhdGEuCisgICAgICAgIDxodHRwOi8vd2Via2l0Lm9yZy9iLzEwOTk3Nj4K KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBEb24ndCBj cmVhdGUgRWxlbWVudERhdGEgZm9yIGFuIEVsZW1lbnQgdW5uZWNlc3NhcmlseSBqdXN0IGJlY2F1 c2Ugc29tZW9uZSBjYWxscyAuYXR0cmlidXRlcyBvbiBpdC4KKyAgICAgICAgUHJldmlvdXNseSwg SlMgbGlrZSB0aGlzIHdvdWxkIGNyZWF0ZSBlbXB0eSBFbGVtZW50RGF0YSB3aGVuICdlbGVtZW50 JyBoYXMgbm8gYXR0cmlidXRlczoKKworICAgICAgICAgICAgZm9yIChpID0gMDsgaSA8IGVsZW1l bnQuYXR0cmlidXRlcy5sZW5ndGg7ICsraSkKKyAgICAgICAgICAgICAgICBkb1N0dWZmKGVsZW1l bnQuYXR0cmlidXRlc1tpXSk7CisKKyAgICAgICAgTWFrZSBOYW1lZE5vZGVNYXA6Omxlbmd0aCgp IHNob3J0LWNpcmN1aXQgYW5kIHJldHVybiAwIGlmICFFbGVtZW50OjpoYXNBdHRyaWJ1dGVzKCku CisKKyAgICAgICAgKiBkb20vRWxlbWVudC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpFbGVtZW50 OjphdHRyaWJ1dGVzKToKKyAgICAgICAgKiBkb20vTmFtZWROb2RlTWFwLmNwcDoKKyAgICAgICAg KFdlYkNvcmU6Ok5hbWVkTm9kZU1hcDo6bGVuZ3RoKToKKworMjAxMy0wMi0xNSAgQW5kcmVhcyBL bGluZyAgPGFrbGluZ0BhcHBsZS5jb20+CisKICAgICAgICAgU2hhcmVhYmxlRWxlbWVudERhdGEg c2hvdWxkIHVzZSB6ZXJvLWxlbmd0aCBhcnJheSBmb3Igc3RvcmFnZS4KICAgICAgICAgPGh0dHA6 Ly93ZWJraXQub3JnL2IvMTA5OTU5PgogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9kb20v RWxlbWVudC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9kb20vRWxlbWVudC5jcHAKaW5kZXggMDI1YWJh MS4uNmVjZDU5NiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvZG9tL0VsZW1lbnQuY3BwCisr KyBiL1NvdXJjZS9XZWJDb3JlL2RvbS9FbGVtZW50LmNwcApAQCAtMzEzLDcgKzMxMyw2IEBAIHZv aWQgRWxlbWVudDo6c2V0Qm9vbGVhbkF0dHJpYnV0ZShjb25zdCBRdWFsaWZpZWROYW1lJiBuYW1l LCBib29sIHZhbHVlKQogCiBOYW1lZE5vZGVNYXAqIEVsZW1lbnQ6OmF0dHJpYnV0ZXMoKSBjb25z dAogewotICAgIGVuc3VyZUVsZW1lbnREYXRhV2l0aFN5bmNocm9uaXplZEF0dHJpYnV0ZXMoKTsK ICAgICBFbGVtZW50UmFyZURhdGEqIHJhcmVEYXRhID0gY29uc3RfY2FzdDxFbGVtZW50Kj4odGhp cyktPmVuc3VyZUVsZW1lbnRSYXJlRGF0YSgpOwogICAgIGlmIChOYW1lZE5vZGVNYXAqIGF0dHJp YnV0ZU1hcCA9IHJhcmVEYXRhLT5hdHRyaWJ1dGVNYXAoKSkKICAgICAgICAgcmV0dXJuIGF0dHJp YnV0ZU1hcDsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2RvbS9OYW1lZE5vZGVNYXAuY3Bw IGIvU291cmNlL1dlYkNvcmUvZG9tL05hbWVkTm9kZU1hcC5jcHAKaW5kZXggNDljNDY5Ny4uZTlm ZjNiMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvZG9tL05hbWVkTm9kZU1hcC5jcHAKKysr IGIvU291cmNlL1dlYkNvcmUvZG9tL05hbWVkTm9kZU1hcC5jcHAKQEAgLTExMCw2ICsxMTAsOCBA QCBQYXNzUmVmUHRyPE5vZGU+IE5hbWVkTm9kZU1hcDo6aXRlbSh1bnNpZ25lZCBpbmRleCkgY29u c3QKIAogc2l6ZV90IE5hbWVkTm9kZU1hcDo6bGVuZ3RoKCkgY29uc3QKIHsKKyAgICBpZiAoIW1f ZWxlbWVudC0+aGFzQXR0cmlidXRlcygpKQorICAgICAgICByZXR1cm4gMDsKICAgICByZXR1cm4g bV9lbGVtZW50LT5hdHRyaWJ1dGVDb3VudCgpOwogfQogCg==