Bug 109976 - Calling DOM Element.attributes shouldn't force creation of ElementData.
Summary: Calling DOM Element.attributes shouldn't force creation of ElementData.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on: 110026
Blocks: 109505
  Show dependency treegraph
 
Reported: 2013-02-15 15:00 PST by Andreas Kling
Modified: 2013-03-15 12:01 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.04 KB, patch)
2013-02-15 15:03 PST, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2013-02-15 15:00:16 PST
We shouldn't construct ElementData "just to have some" when Element.attributes is called via DOM API.
Comment 1 Andreas Kling 2013-02-15 15:03:05 PST
Created attachment 188649 [details]
Patch
Comment 2 Darin Adler 2013-02-15 16:21:41 PST
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?
Comment 3 Andreas Kling 2013-02-15 16:23:12 PST
(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().
Comment 4 WebKit Review Bot 2013-02-15 17:40:59 PST
Comment on attachment 188649 [details]
Patch

Clearing flags on attachment: 188649

Committed r143076: <http://trac.webkit.org/changeset/143076>
Comment 5 WebKit Review Bot 2013-02-15 17:41:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Ryosuke Niwa 2013-02-16 13:23:55 PST
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
Comment 8 WebKit Review Bot 2013-02-16 13:28:21 PST
Re-opened since this is blocked by bug 110026
Comment 9 Darin Adler 2013-02-16 13:31:00 PST
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!
Comment 10 Andreas Kling 2013-02-16 13:33:11 PST
(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.
Comment 11 Andreas Kling 2013-02-16 13:35:44 PST
(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.