Summary: | errorDescriptionForValue() should not assume error value is an Object | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Curtis <chris_curtis> | ||||||||
Component: | JavaScriptCore | Assignee: | Chris Curtis <chris_curtis> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bfulgham, chris_curtis, commit-queue, jbriance, joepeck, mark.lam, msaboff, oliver | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 119803 | ||||||||||
Attachments: |
|
Description
Chris Curtis
2013-08-14 13:41:00 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. (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. Created attachment 208776 [details]
patch
Added a check to make sure the JSValue was an object before casting it.
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 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? (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. (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? (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. (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. 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.
(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 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. Created attachment 209228 [details]
patch2
Comment on attachment 209228 [details]
patch2
r=me
Comment on attachment 209228 [details] patch2 Clearing flags on attachment: 209228 Committed r154457: <http://trac.webkit.org/changeset/154457> All reviewed patches have been landed. Closing bug. Why no test? Darin even mentioned we should have a test in an earlier review. |