Bug 109266

Summary: Migrate ExceptionCode ASSERTs in IDB to ASSERT_NO_EXCEPTION.
Product: WebKit Reporter: Mike West <mkwst>
Component: WebCore Misc.Assignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, dgrogan, jochen, jsbell, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 109044    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Mike West 2013-02-08 00:21:37 PST
I attempted to land some seemingly straightforward IDB changes as part of https://bugs.webkit.org/show_bug.cgi?id=109044; they caused crashes.

I'll figure out why and fix those files in a more focused patch.
Comment 1 Mike West 2013-02-08 03:39:07 PST
Created attachment 187280 [details]
Patch
Comment 2 Mike West 2013-02-08 03:40:37 PST
ASSERT_NO_EXCEPTIONS ensures, among other things, that methods which care about the value of 'ec' initialize it to 0. IDBCursor::continueFunction didn't, but ASSERTED(!ec) anyway. It doesn't in the current patch.

Moar review, Jochen?
Comment 3 Mike West 2013-02-08 04:10:20 PST
Created attachment 187287 [details]
Patch
Comment 4 jochen 2013-02-08 04:14:08 PST
Comment on attachment 187287 [details]
Patch

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

ok

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:220
> +    ec = 0;

why not initialize this first thing in the method?
Comment 5 Mike West 2013-02-08 04:18:16 PST
(In reply to comment #4)
> (From update of attachment 187287 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187287&action=review
> 
> ok
> 
> > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:220
> > +    ec = 0;
> 
> why not initialize this first thing in the method?

Premature optimization. I figured we only need to initialize it in the case where the above block of 'if' statements doesn't set the value and return.
Comment 6 Mike West 2013-02-08 04:23:42 PST
Created attachment 187289 [details]
Patch
Comment 7 Mike West 2013-02-08 04:25:15 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 187287 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=187287&action=review
> > 
> > ok
> > 
> > > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:220
> > > +    ec = 0;
> > 
> > why not initialize this first thing in the method?
> 
> Premature optimization. I figured we only need to initialize it in the case where the above block of 'if' statements doesn't set the value and return.

I moved the value up to the top of the method, and noticed two other methods where the same thing was happening. I don't think it's worth a rereview, as the change is exactly what you've already reviewed, just in two additional methods.
Comment 8 WebKit Review Bot 2013-02-08 05:06:08 PST
Comment on attachment 187289 [details]
Patch

Clearing flags on attachment: 187289

Committed r142262: <http://trac.webkit.org/changeset/142262>
Comment 9 WebKit Review Bot 2013-02-08 05:06:13 PST
All reviewed patches have been landed.  Closing bug.