Bug 166687 – Assertion hit on redfin.com: ASSERTION FAILED: collection->length() > 1

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Assertion hit on redfin.com: ASSERTION FAILED: collection->length() > 1

Source/WebCore/bindings/js/JSDOMWindowProperties.cpp(65) : bool WebCore::jsDOMWindowPropertiesGetOwnPropertySlotNamedItemGetter(WebCore::JSDOMWindowProperties *, WebCore::Frame &, JSC::ExecState *, JSC::PropertyName, JSC::PropertySlot &)
1   0x10fa8eead WTFCrash
2   0x11367bdc7 WebCore::jsDOMWindowPropertiesGetOwnPropertySlotNamedItemGetter(WebCore::JSDOMWindowProperties*, WebCore::Frame&, JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&)
3   0x11367ba46 WebCore::JSDOMWindowProperties::getOwnPropertySlot(JSC::JSObject*, JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&)
4   0x10e6ce8ea JSC::JSObject::getNonIndexPropertySlot(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&)
5   0x10e6cd236 JSC::JSObject::getPropertySlot(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&)
6   0x10e6cce67 JSC::JSValue::getPropertySlot(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&) const
7   0x10e6cccc5 JSC::JSValue::get(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&) const
8   0x10f65027a llint_slow_path_get_by_id
9   0x10f6618d0 llint_entry
10  0x10f65e61e vmEntryToJavaScript
11  0x10f472989 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
12  0x10f42ab34 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*)
13  0x10ed1939d JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
14  0x10ed19560 JSC::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
15  0x114700bcb WebCore::JSMainThreadExecState::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
16  0x1147009b8 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*)
17  0x114700cad WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&, WebCore::ExceptionDetails*)
18  0x114715157 WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&)
19  0x113ebee88 WebCore::LoadableClassicScript::execute(WebCore::ScriptElement&)
20  0x114715c0f WebCore::ScriptElement::executeScriptAndDispatchEvent(WebCore::LoadableScript&)
21  0x114715d66 WebCore::ScriptElement::executePendingScript(WebCore::PendingScript&)
22  0x11472d85a WebCore::ScriptRunner::timerFired()
23  0x1147305fb void std::__1::__invoke_void_return_wrapper<void>::__call<std::__1::__bind<void (WebCore::ScriptRunner::*&)(), WebCore::ScriptRunner*>&>(std::__1::__bind<void (WebCore::ScriptRunner::*&)(), WebCore::ScriptRunner*>&&&)
24  0x1147304e9 std::__1::__function::__func<std::__1::__bind<void (WebCore::ScriptRunner::*&)(), WebCore::ScriptRunner*>, std::__1::allocator<std::__1::__bind<void (WebCore::ScriptRunner::*&)(), WebCore::ScriptRunner*> >, void ()>::operator()()
25  0x11234b02a std::__1::function<void ()>::operator()() const
26  0x11234af49 WebCore::Timer::fired()
27  0x114be5a0a WebCore::ThreadTimers::sharedTimerFiredInternal()
28  0x114be6c31 WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0::operator()() const
29  0x114be6bfd void std::__1::__invoke_void_return_wrapper<void>::__call<WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0&>(WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0&&&)
30  0x114be6ba9 std::__1::__function::__func<WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0, std::__1::allocator<WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0>, void ()>::operator()()
31  0x11234b02a std::__1::function<void ()>::operator()() const

[Chris Dumez]

<rdar://problem/29865854>

[Chris Dumez]

Created attachment 298024 [details]
Patch

[Chris Dumez]

Comment on attachment 298024 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298024&action=review

> Source/WebCore/dom/Element.cpp:3225
> +    if (isInShadowTree())

Alternatively, we could do this check at the cal sites if you'd prefer.

[Darin Adler]

Comment on attachment 298024 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298024&action=review

>> Source/WebCore/dom/Element.cpp:3225
>> +    if (isInShadowTree())
> 
> Alternatively, we could do this check at the cal sites if you'd prefer.

I like doing this check as close as possible to the code that actually manipulates the collection so we are more likely to get it right.

But I don’t see isInShadowTree checks in HTMLImageElement::parseAttribute and HTMLObjectElement::updateDocNamedItem (function name should not abbreviate "document"). Should I be worried about those cases?

[Chris Dumez]

(In reply to comment #4)
> Comment on attachment 298024 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298024&action=review
> 
> >> Source/WebCore/dom/Element.cpp:3225
> >> +    if (isInShadowTree())
> > 
> > Alternatively, we could do this check at the cal sites if you'd prefer.
> 
> I like doing this check as close as possible to the code that actually
> manipulates the collection so we are more likely to get it right.
> 
> But I don’t see isInShadowTree checks in HTMLImageElement::parseAttribute
> and HTMLObjectElement::updateDocNamedItem (function name should not
> abbreviate "document"). Should I be worried about those cases?

That's a good point. I suspect we have the same bug for document named items. I'll extend the layout test to confirm.

[Chris Dumez]

Created attachment 298031 [details]
Patch

[Chris Dumez]

(In reply to comment #4)
> Comment on attachment 298024 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298024&action=review
> 
> >> Source/WebCore/dom/Element.cpp:3225
> >> +    if (isInShadowTree())
> > 
> > Alternatively, we could do this check at the cal sites if you'd prefer.
> 
> I like doing this check as close as possible to the code that actually
> manipulates the collection so we are more likely to get it right.
> 
> But I don’t see isInShadowTree checks in HTMLImageElement::parseAttribute
> and HTMLObjectElement::updateDocNamedItem (function name should not
> abbreviate "document"). Should I be worried about those cases?

The latest iteration covers the other call sites you mentioned and extends layout test coverage.

[Darin Adler]

Comment on attachment 298031 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298031&action=review

> Source/WebCore/html/HTMLImageElement.cpp:229
> +            if (m_hadNameBeforeAttributeChanged != willHaveName && inDocument() && !isInShadowTree() && is<HTMLDocument>(document())) {

I assume that we want to sort these checks so that the ones most likely to quickly return false come first, or in some sort of conceptually logical order. How did you decide where to add this new check? Maybe the last one is the only virtual function call and the others are all cheaper than that?

> Source/WebCore/html/HTMLObjectElement.cpp:448
> +    if (isNamedItem != wasNamedItem && inDocument() && !isInShadowTree() && is<HTMLDocument>(document())) {

Same comment.

[Chris Dumez]

Created attachment 298040 [details]
Patch

[Chris Dumez]

(In reply to comment #8)
> Comment on attachment 298031 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298031&action=review
> 
> > Source/WebCore/html/HTMLImageElement.cpp:229
> > +            if (m_hadNameBeforeAttributeChanged != willHaveName && inDocument() && !isInShadowTree() && is<HTMLDocument>(document())) {
> 
> I assume that we want to sort these checks so that the ones most likely to
> quickly return false come first, or in some sort of conceptually logical
> order. How did you decide where to add this new check? Maybe the last one is
> the only virtual function call and the others are all cheaper than that?

Actually, all 4 checks are cheap (is<HTMLDocument>() does not cause a virtual function call, merely checks a flag). I added he new check after inDocument() because I felt it was logical to first check that the Element is in a document (more general case) but then make sure it is *not* in a shadow tree (more specific case).

isInShadowTree() will be false in most cases so I don't think there are benefits to make it come first.

[WebKit Commit Bot]

Comment on attachment 298040 [details]
Patch

Clearing flags on attachment: 298040

Committed r210284: <http://trac.webkit.org/changeset/210284>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.