Crash under JSC::JSCell::toNumber(JSC::ExecState*)

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Crash under JSC::JSCell::toNumber(JSC::ExecState*):
Thread 0 name:  Dispatch queue: com.apple.main-thread
Thread 0 Crashed ↩:
0   JavaScriptCore                	0x000000018a49e37c JSC::JSCell::toNumber(JSC::ExecState*) const + 0 (JSCell.cpp:175)
1   WebCore                       	0x000000018bc783d0 WebCore::DOMMatrixInit WebCore::convertDictionary<WebCore::DOMMatrixInit>(JSC::ExecState&, JSC::JSValue) + 4344 (JSCJSValueInlines.h:728)
2   WebCore                       	0x000000018bc99e44 WebCore::jsDOMPointReadOnlyPrototypeFunctionMatrixTransform(JSC::ExecState*) + 232 (JSDOMConvertDictionary.h:42)
3   ???                           	0x0000000dded3c240 0 + 59572994624
4   JavaScriptCore                	0x000000018a4fe4a0 llint_entry + 28848
5   JavaScriptCore                	0x000000018a4fe4a0 llint_entry + 28848
6   JavaScriptCore                	0x000000018a4fe4a0 llint_entry + 28848
7   JavaScriptCore                	0x000000018a4fe4a0 llint_entry + 28848
8   JavaScriptCore                	0x000000018a4fe4a0 llint_entry + 28848
9   JavaScriptCore                	0x000000018a4fe89c llint_entry + 29868
10  JavaScriptCore                	0x000000018a4f7220 vmEntryToJavaScript + 272
11  JavaScriptCore                	0x000000018aaa3ad4 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 184 (JITCode.cpp:81)
12  JavaScriptCore                	0x000000018a40111c JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 464 (Interpreter.cpp:1028)
13  WebCore                       	0x000000018c7472c4 WebCore::HTMLMediaElement::didAddUserAgentShadowRoot(WebCore::ShadowRoot&) + 1132 (HTMLMediaElement.cpp:7177)
14  WebCore                       	0x000000018c5cc3f4 WebCore::Element::addShadowRoot(WTF::Ref<WebCore::ShadowRoot, WTF::DumbPtrTraits<WebCore::ShadowRoot> >&&) + 380 (Element.cpp:1798)
15  WebCore                       	0x000000018b9a1ddc WebCore::Element::ensureUserAgentShadowRoot() + 80 (Element.cpp:1893)
16  WebCore                       	0x000000018c730660 WebCore::HTMLMediaElement::configureMediaControls() + 532 (HTMLMediaElement.cpp:4279)
17  WebCore                       	0x000000018c5824a4 WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck(WebCore::Node&) + 960 (ContainerNode.cpp:200)
18  WebCore                       	0x000000018c584798 WebCore::ContainerNode::appendChild(WebCore::Node&) + 80 (ContainerNode.cpp:672)
19  WebCore                       	0x000000018b99311c WebCore::jsNodePrototypeFunctionAppendChild(JSC::ExecState*) + 236 (JSNode.cpp:851)

[Chris Dumez]

<rdar://problem/37694346>

[Chris Dumez]

Created attachment 334301 [details]
Patch

[Mark Lam]

Comment on attachment 334301 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        No new tests, extended new test.

Did you mean "extended existing test"?

> Source/WebCore/bindings/js/JSDOMConvertNumbers.h:388
>      static JSC::JSValue convert(Type value)
>      {
> -        return JSC::jsNumber(value);
> +        return JSC::jsNumber(JSC::purifyNaN(value));
>      }

If you look in JSCJSValue.h, you'll see that jsNumber() can also take integer values.  Is IDLUnrestrictedDouble::ImplementationType guaranteed to be a double?  If not, then maybe you should have 2 versions of convert: one for integral types and one for double.

> Source/WebCore/testing/TypeConversions.h:147
> +    double testImpureNaNUnrestrictedDouble() const { return bitwise_cast<double>(0xffff000000000000ll); }
> +    double testImpureNaN2UnrestrictedDouble() const { return bitwise_cast<double>(0x7ff8000000000001ll); }
> +    double testQuietNaNUnrestrictedDouble() const { return std::numeric_limits<double>::quiet_NaN(); }

Maybe you should add a case for PureNaN as well just to make sure that it is also working as expected.

[Chris Dumez]

Comment on attachment 334301 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        No new tests, extended new test.
> 
> Did you mean "extended existing test"?

Yep. Will fix.

>> Source/WebCore/bindings/js/JSDOMConvertNumbers.h:388
>>      }
> 
> If you look in JSCJSValue.h, you'll see that jsNumber() can also take integer values.  Is IDLUnrestrictedDouble::ImplementationType guaranteed to be a double?  If not, then maybe you should have 2 versions of convert: one for integral types and one for double.

AFAICT IDLUnrestrictedDouble::ImplementationType has to be a double.

>> Source/WebCore/testing/TypeConversions.h:147
>> +    double testQuietNaNUnrestrictedDouble() const { return std::numeric_limits<double>::quiet_NaN(); }
> 
> Maybe you should add a case for PureNaN as well just to make sure that it is also working as expected.

Good point, will add.

[Chris Dumez]

Created attachment 334304 [details]
Patch

[Mark Lam]

Comment on attachment 334304 [details]
Patch

r=me if EWS bots are happy.

[Chris Dumez]

Comment on attachment 334304 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMConvertNumbers.h:363
> +        ASSERT(!std::isnan(value));

I will add this assertion in a follow-up because it hits for the following test:
LayoutTests/media/track/track-add-remove-cue.html 

Seems to be related to VTTCue.startTime where we do not match the spec and sometimes return NaN.

[Chris Dumez]

Created attachment 334313 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 334313 [details]
Patch

Clearing flags on attachment: 334313

Committed r228851: <https://trac.webkit.org/changeset/228851>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.