Bug 101206

Summary: [V8] toV8(impl) should return null if impl is 0
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: WebCore JavaScriptAssignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Kentaro Hara
Reported 2012-11-05 03:49:27 PST
toV8(impl) should return null if impl is 0. However, V8HTMLCollectionCustom::toV8() does not have the null check. I don't think there is a call path that hits the null check, we should add it because all other toV8()s have the null check.
Attachments
Patch (1.88 KB, patch)
2012-11-05 03:51 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
Patch (2.48 KB, patch)
2012-11-05 03:55 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
Patch (1.78 KB, patch)
2012-11-05 06:13 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Kentaro Hara
Comment 1 2012-11-05 03:51:00 PST
Created attachment 172311 [details] Patch
Kentaro Hara
Comment 2 2012-11-05 03:55:04 PST
Created attachment 172314 [details] Patch
WebKit Review Bot
Comment 3 2012-11-05 06:10:14 PST
Comment on attachment 172314 [details] Patch Attachment 172314 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14683224 New failing tests: storage/indexeddb/keyrange.html
Kentaro Hara
Comment 4 2012-11-05 06:13:48 PST
Created attachment 172323 [details] Patch
Kentaro Hara
Comment 5 2012-11-05 06:15:09 PST
I removed a change for IDBKeyCustom.cpp from the patch. I will address it in another patch.
Adam Barth
Comment 6 2012-11-05 10:03:09 PST
Comment on attachment 172323 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172323&action=review > Source/WebCore/ChangeLog:12 > + No tests. I think there will be no call path that hits the change. Should we add an ASSERT instead?
Kentaro Hara
Comment 7 2012-11-05 10:05:05 PST
(In reply to comment #6) > (From update of attachment 172323 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172323&action=review > > > Source/WebCore/ChangeLog:12 > > + No tests. I think there will be no call path that hits the change. > > Should we add an ASSERT instead? That's possible. That being said, all other toV8() in CodeGeneratorV8.pm and custom bindings have 'if(!impl) return v8NullWithCheck(isolate)', which looks like an idiom. Both are OK to me.
Adam Barth
Comment 8 2012-11-05 10:12:41 PST
Comment on attachment 172323 [details] Patch Yeah, we should be consistent with the generated code.
WebKit Review Bot
Comment 9 2012-11-05 10:41:40 PST
Comment on attachment 172323 [details] Patch Clearing flags on attachment: 172323 Committed r133496: <http://trac.webkit.org/changeset/133496>
WebKit Review Bot
Comment 10 2012-11-05 10:41:43 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.