Bug 109976 – Calling DOM Element.attributes shouldn't force creation of ElementData.

• Component: DOM
• Status: RESOLVED FIXED

[Andreas Kling]

We shouldn't construct ElementData "just to have some" when Element.attributes is called via DOM API.

[Andreas Kling]

Created attachment 188649 [details]
Patch

[Darin Adler]

Comment on attachment 188649 [details]
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?

[Andreas Kling]

(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().

[WebKit Review Bot]

Comment on attachment 188649 [details]
Patch

Clearing flags on attachment: 188649

Committed r143076: <http://trac.webkit.org/changeset/143076>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[WebKit Review Bot]

Re-opened since this is blocked by bug 110026

[Darin Adler]

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!

[Andreas Kling]

(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.

[Andreas Kling]

(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.