Bug 119812 – errorDescriptionForValue() should not assume error value is an Object

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Chris Curtis]

+++ This bug was initially created as a clone of Bug #119803 +++
Bug 119803 <https://bugs.webkit.org/show_bug.cgi?id=119803> reported 2 crashes: 1 in a release build, the other in a debug build.  The release build issues is due to errorDescriptionForValue() assuming that the passed in error value is either one of the following: null, undefined, int32, double, true, false, string, function, or object.  If the value did not turn out to be one of the first 8 cases, we assumed it was an object.  This turned out to not be true i.e. the steps below produced an error JSValue that was not any of the 9 cases.  This bug will fix that issue.

The debug crash is due to something else, and we'll let <https://bugs.webkit.org/show_bug.cgi?id=119803> track that.

Steps to reproduce the crash:

Visiting the website http://html5test.com using WinLauncher on Windows crashes with the following stacktrace:

In release we crash as follows:

>	JavaScriptCore.dll!JSC::JSCell::methodTable()  Line 157	C++
 	JavaScriptCore.dll!JSC::errorDescriptionForValue(JSC::ExecState * exec, JSC::JSValue v)  Line 110 + 0x8 bytes	C++
 	JavaScriptCore.dll!JSC::createError(JSC::ExecState * exec, JSC::JSObject * (JSC::ExecState *, const WTF::String &)* errorFactory, JSC::JSValue value, const WTF::String & message)  Line 115 + 0x24 bytes	C++
 	JavaScriptCore.dll!JSC::createNotAnObjectError(JSC::ExecState * exec, JSC::JSValue value)  Line 139 + 0x28 bytes	C++
 	JavaScriptCore.dll!JSC::JSValue::synthesizePrototype(JSC::ExecState * exec)  Line 111 + 0xe bytes	C++
 	JavaScriptCore.dll!JSC::JSValue::get(JSC::ExecState * exec, JSC::PropertyName propertyName, JSC::PropertySlot & slot)  Line 637	C++
 	JavaScriptCore.dll!JSC::getByVal(JSC::ExecState * callFrame, JSC::JSValue baseValue, JSC::JSValue subscript, JSC::ReturnAddressPtr returnAddress)  Line 1544 + 0x2b bytes	C++
 	JavaScriptCore.dll!cti_op_get_by_val_generic(void * * args)  Line 1605	C++
 	0b8307d0()	
 	JavaScriptCore.dll!JSC::JITCode::execute(JSC::JSStack * stack, JSC::ExecState * callFrame, JSC::VM * vm)  Line 46 + 0x20 bytes	C++
 	JavaScriptCore.dll!JSC::Interpreter::execute(JSC::ProgramExecutable * program, JSC::ExecState * callFrame, JSC::JSObject * thisObj)  Line 851 + 0x2d bytes	C++
 	JavaScriptCore.dll!JSC::evaluate(JSC::ExecState * exec, const JSC::SourceCode & source, JSC::JSValue thisValue, JSC::JSValue * returnedException)  Line 85	C++
 	WebKit.dll!WebCore::JSMainThreadExecState::evaluate(JSC::ExecState * exec, const JSC::SourceCode & source, JSC::JSValue thisValue, JSC::JSValue * exception)  Line 74 + 0x1b bytes	C++
 	WebKit.dll!WebCore::ScriptController::evaluateInWorld(const WebCore::ScriptSourceCode & sourceCode, WebCore::DOMWrapperWorld * world)  Line 142 + 0x34 bytes	C++
 	WebKit.dll!WebCore::ScriptController::evaluate(const WebCore::ScriptSourceCode & sourceCode)  Line 158 + 0x40 bytes	C++
 	WebKit.dll!WebCore::ScriptElement::executeScript(const WebCore::ScriptSourceCode & sourceCode)  Line 316 + 0x16 bytes	C++
 	WebKit.dll!WebCore::ScriptRunner::timerFired(WebCore::Timer<WebCore::ScriptRunner> * timer)  Line 121 + 0x2a5 bytes	C++
 	WebKit.dll!WebCore::Timer<WebCore::Settings>::fired()  Line 114 + 0xb bytes	C++
 	WebKit.dll!WebCore::ThreadTimers::sharedTimerFiredInternal()  Line 132	C++
 	WebKit.dll!WebCore::TimerWindowWndProc(HWND__ * hWnd, unsigned int message, unsigned int wParam, long lParam)  Line 111	C++
 	user32.dll!_InternalCallWinProc@20()  + 0x23 bytes	
 	user32.dll!_UserCallWinProcCheckWow@36()  + 0xbd bytes	
 	user32.dll!_DispatchMessageWorker@8()  + 0xf8 bytes	
 	user32.dll!_DispatchMessageW@4()  + 0x10 bytes	
 	CoreFoundation.dll!__CFRunLoopRun(__CFRunLoop * rl, __CFRunLoopMode * rlm, double seconds, unsigned char stopAfterHandle, __CFRunLoopMode * previousMode)  Line 42292	C++
 	CoreFoundation.dll!CFRunLoopRunSpecific(__CFRunLoop * rl, const __CFString * modeName, double seconds, unsigned char returnAfterSourceHandled)  Line 42413 + 0x12 bytes	C++
 	CoreFoundation.dll!CFRunLoopRun()  Line 42440 + 0x1d bytes	C++
 	WinLauncher.dll!dllLauncherEntryPoint(HINSTANCE__ * __formal, HINSTANCE__ * __formal, HINSTANCE__ * __formal, int nCmdShow)  Line 456	C++
 	WinLauncher.exe!004018b8() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for WinLauncher.exe]	
 	msvcr100.dll!_free()  + 0x1c bytes	
 	msvcr100.dll!__wsetenvp()  + 0xa2 bytes	
 	msvcr100.dll!___wgetmainargs()  + 0x53 bytes	
 	WinLauncher.exe!004024c9() 	
 	WinLauncher.exe!00402636() 	
 	kernel32.dll!@BaseThreadInitThunk@12()  + 0xe bytes	
 	ntdll.dll!___RtlUserThreadStart@8()  + 0x27 bytes	
 	ntdll.dll!__RtlUserThreadStart@8()  + 0x1b bytes

[Brent Fulgham]

Chris, the 'crash' under Bug #119803 is simply an assertion. We have debug asserts perform a WTFCrash, which stops in the debugger so you can see what's going on.

I mentioned it in my original bug because I thought it might be a clue to the problem you resolved here.

[Chris Curtis]

(In reply to comment #1)
> Chris, the 'crash' under Bug #119803 is simply an assertion. We have debug asserts perform a WTFCrash, which stops in the debugger so you can see what's going on.
> 
> I mentioned it in my original bug because I thought it might be a clue to the problem you resolved here.

This is true but that assert happens in a completely different area that is unrelated to the release crash. The patch will fix the release crash, but the debug assert crash will still happen.

[Chris Curtis]

Created attachment 208776 [details]
patch

Added a check to make sure the JSValue was an object before casting it.

[Oliver Hunt]

Comment on attachment 208776 [details]
patch

What is the type?  Is it not a cell?  If it is a cell why class info does it say it has?

[Darin Adler]

Comment on attachment 208776 [details]
patch

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

We need a regression test for this. Or an explanation of why you can’t make one.

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:109
> +        return jsString(exec, asObject(v)->methodTable()->className(asObject(v)));

Should use “object” here instead of “asObject(v)”.

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:111
> +    return jsString(exec, "");

Should be:

    return vm->smallStrings.emptyString();

But why specifically the empty string?

[Chris Curtis]

(In reply to comment #4)
> (From update of attachment 208776 [details])
> What is the type?  Is it not a cell?  If it is a cell why class info does it say it has?

It has no type. The JSValue in this case only evaluates as a primitive. All other types evaluate as false. It could be something unique with the site because it tests features of html5 that are not yet supported. The other unique thing is that this is only present on windows. I can not duplicate error on mac.

[Oliver Hunt]

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 208776 [details] [details])
> > What is the type?  Is it not a cell?  If it is a cell why class info does it say it has?
> 
> It has no type. The JSValue in this case only evaluates as a primitive. All other types evaluate as false. It could be something unique with the site because it tests features of html5 that are not yet supported. The other unique thing is that this is only present on windows. I can not duplicate error on mac.

No, everything must have a type.  Hence my question: does value.isCell() return true?  If it does it could be that we're incorrectly claiming that it's guaranteed to be a cell and so we're blindly replacing the tag with CellTag.  If it is a real cell it must have a structure, and the structure has a class info.

Have you tried reproducing on 32bit mac?

[Chris Curtis]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > (From update of attachment 208776 [details] [details] [details])
> > > What is the type?  Is it not a cell?  If it is a cell why class info does it say it has?
> > 
> > It has no type. The JSValue in this case only evaluates as a primitive. All other types evaluate as false. It could be something unique with the site because it tests features of html5 that are not yet supported. The other unique thing is that this is only present on windows. I can not duplicate error on mac.
> 
> No, everything must have a type.  Hence my question: does value.isCell() return true?  If it does it could be that we're incorrectly claiming that it's guaranteed to be a cell and so we're blindly replacing the tag with CellTag.  If it is a real cell it must have a structure, and the structure has a class info.
> 
> Have you tried reproducing on 32bit mac?

"Hence my question: does value.isCell() return true?" No, value.isCell() evaluates false. I reduced it down to the 32 bit build on the mac side, and still mac does not fail.

[Chris Curtis]

(In reply to comment #5)
> (From update of attachment 208776 [details])
> > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:111
> > +    return jsString(exec, "");
> 
> Should be:
> 
>     return vm->smallStrings.emptyString();
> 
> But why specifically the empty string?

At this point in the code if the JSValue had any type, it would have been picked out. This piece of code is used to stringily a JSValue for the error object message. There is also a stipulation that this function cannot be allowed to throw another error so the normal method of calling toString() can not be used.

The empty string was selected because there was no type to send back.

[Chris Curtis]

Created attachment 209223 [details]
winLauncher Patch

This patch will fix the problem on win launcher that is causing it to crash. I have created a separate bugzilla report to look into why the JSValue isEmpty() in the first place.

[Chris Curtis]

(In reply to comment #10)
> Created an attachment (id=209223) [details]
> winLauncher Patch
> 
> This patch will fix the problem on win launcher that is causing it to crash. I have created a separate bugzilla report to look into why the JSValue isEmpty() in the first place.

The new bugzilla bug for follow up: https://bugs.webkit.org/show_bug.cgi?id=120080

[Geoffrey Garen]

Comment on attachment 209223 [details]
winLauncher Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Added a check to make sure that the JSValue was an object before casting it as an object. Also, In case the JSValue was has no type

"Also, In" should be "Also, in".

JSValues always have a type. The type in question here is the empty value ("JSValue()").

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:113
> +    // FIXME: https://bugs.webkit.org/show_bug.cgi?id=120080 The JSValue here should never be empty here.

"here" is twice in this sentence.

[Chris Curtis]

Created attachment 209228 [details]
patch2

[Geoffrey Garen]

Comment on attachment 209228 [details]
patch2

r=me

[WebKit Commit Bot]

Comment on attachment 209228 [details]
patch2

Clearing flags on attachment: 209228

Committed r154457: <http://trac.webkit.org/changeset/154457>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Why no test? Darin even mentioned we should have a test in an earlier review.