Bug 119812 - errorDescriptionForValue() should not assume error value is an Object
Summary: errorDescriptionForValue() should not assume error value is an Object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Curtis
URL:
Keywords:
Depends on:
Blocks: 119803
  Show dependency treegraph
 
Reported: 2013-08-14 13:41 PDT by Chris Curtis
Modified: 2013-08-22 14:24 PDT (History)
8 users (show)

See Also:


Attachments
patch (1.70 KB, patch)
2013-08-14 17:21 PDT, Chris Curtis
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
winLauncher Patch (1.85 KB, patch)
2013-08-20 13:14 PDT, Chris Curtis
no flags Details | Formatted Diff | Diff
patch2 (1.87 KB, patch)
2013-08-20 14:05 PDT, Chris Curtis
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 Curtis 2013-08-14 13:41:00 PDT
+++ 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
Comment 1 Brent Fulgham 2013-08-14 13:47:41 PDT
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.
Comment 2 Chris Curtis 2013-08-14 13:51:52 PDT
(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.
Comment 3 Chris Curtis 2013-08-14 17:21:55 PDT
Created attachment 208776 [details]
patch

Added a check to make sure the JSValue was an object before casting it.
Comment 4 Oliver Hunt 2013-08-14 17:29:57 PDT
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?
Comment 5 Darin Adler 2013-08-14 18:36:30 PDT
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?
Comment 6 Chris Curtis 2013-08-14 20:21:10 PDT
(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.
Comment 7 Oliver Hunt 2013-08-14 20:24:02 PDT
(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?
Comment 8 Chris Curtis 2013-08-14 20:26:35 PDT
(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.
Comment 9 Chris Curtis 2013-08-14 20:43:41 PDT
(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.
Comment 10 Chris Curtis 2013-08-20 13:14:06 PDT
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.
Comment 11 Chris Curtis 2013-08-20 13:15:05 PDT
(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
Comment 12 Geoffrey Garen 2013-08-20 13:41:48 PDT
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.
Comment 13 Chris Curtis 2013-08-20 14:05:09 PDT
Created attachment 209228 [details]
patch2
Comment 14 Geoffrey Garen 2013-08-22 12:21:30 PDT
Comment on attachment 209228 [details]
patch2

r=me
Comment 15 WebKit Commit Bot 2013-08-22 12:43:13 PDT
Comment on attachment 209228 [details]
patch2

Clearing flags on attachment: 209228

Committed r154457: <http://trac.webkit.org/changeset/154457>
Comment 16 WebKit Commit Bot 2013-08-22 12:43:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Joseph Pecoraro 2013-08-22 14:24:34 PDT
Why no test? Darin even mentioned we should have a test in an earlier review.