Bug 166687 - Assertion hit on redfin.com: ASSERTION FAILED: collection->length() > 1
Summary: Assertion hit on redfin.com: ASSERTION FAILED: collection->length() > 1
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-01-04 09:36 PST by Chris Dumez
Modified: 2017-01-04 13:27 PST (History)
13 users (show)

See Also:


Attachments
Patch (7.03 KB, patch)
2017-01-04 10:03 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (14.79 KB, patch)
2017-01-04 11:06 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (20.45 KB, patch)
2017-01-04 12:45 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-01-04 09:36:37 PST
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
Comment 1 Chris Dumez 2017-01-04 09:37:00 PST
<rdar://problem/29865854>
Comment 2 Chris Dumez 2017-01-04 10:03:01 PST
Created attachment 298024 [details]
Patch
Comment 3 Chris Dumez 2017-01-04 10:03:43 PST
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.
Comment 4 Darin Adler 2017-01-04 10:10:45 PST
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?
Comment 5 Chris Dumez 2017-01-04 10:14:47 PST
(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.
Comment 6 Chris Dumez 2017-01-04 11:06:13 PST
Created attachment 298031 [details]
Patch
Comment 7 Chris Dumez 2017-01-04 11:49:19 PST
(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.
Comment 8 Darin Adler 2017-01-04 12:04:23 PST
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.
Comment 9 Chris Dumez 2017-01-04 12:45:53 PST
Created attachment 298040 [details]
Patch
Comment 10 Chris Dumez 2017-01-04 12:50:37 PST
(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.
Comment 11 WebKit Commit Bot 2017-01-04 13:27:02 PST
Comment on attachment 298040 [details]
Patch

Clearing flags on attachment: 298040

Committed r210284: <http://trac.webkit.org/changeset/210284>
Comment 12 WebKit Commit Bot 2017-01-04 13:27:08 PST
All reviewed patches have been landed.  Closing bug.