Bug 109295

Summary: Use IGNORE_EXCEPTION for initialized, but unused, ExceptionCodes.
Product: WebKit Reporter: Mike West <mkwst>
Component: New BugsAssignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, alecflett, apinheiro, cfleizach, darin, dgrogan, dmazzoni, d-r, eric.carlson, eric, feature-media-reviews, fmalita, jdiggs, jochen, jsbell, mifenton, ojan.autocc, pdr, schenney, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 108771    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Mike West 2013-02-08 06:09:13 PST
Use IGNORE_EXCEPTION for Editor and TextCheckingHelper.
Comment 1 Mike West 2013-02-08 06:20:02 PST
Created attachment 187305 [details]
Patch
Comment 2 Mike West 2013-02-08 06:20:31 PST
Waiting until 108771 lands before throwing this to the bots.
Comment 3 Mike West 2013-02-08 07:40:41 PST
Repurposing this, as I found a lot more callsites.

http://trac.webkit.org/changeset/142271 migrated callsites that never initialized 'ExceptionCode ec;' to IGNORE_EXCEPTION. It looks like many callsites that do initialize the variable never use it, which is unexpected. This patch will migrate those callsites to IGNORE_EXCEPTION as well.
Comment 4 Mike West 2013-02-08 08:13:23 PST
Created attachment 187322 [details]
Patch
Comment 5 Mike West 2013-02-08 08:17:00 PST
So. That turned out to be more than I expected. This is just the callsites in WebCore. I haven't yet gone through the other potential callsites (WebKit2/WebKit). This patch looks for the following pattern:

    ExceptionCode ec = 0;
    methodThatGeneratesExceptions(ec);

and replaces it with

    methodThatGeneratesExceptions(IGNORE_EXCEPTION);

Once this lands and sticks, I plan on dropping a note to webkit-dev@ noting that the new macro exists, and giving a brief overview of the types of exceptions we ignore. The vast majority seems to be simple DOM manipulation, as Darin noted in the last bug.
Comment 6 jochen 2013-02-08 09:16:03 PST
Comment on attachment 187322 [details]
Patch

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

> Source/WebCore/page/DOMSelection.cpp:412
> +        if (r->compareBoundaryPoints(Range::END_TO_START, range.get(), IGNORE_EXCEPTION) < 1 && !IGNORE_EXCEPTION) {

it's not ignoring the exception here, right?
Comment 7 Darin Adler 2013-02-08 09:34:57 PST
Comment on attachment 187322 [details]
Patch

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

I think the vast majority of these should be ASSERT_NO_EXCEPTION instead. It’s kind of random where we are using IGNORE_EXCEPTION and where ASSERT_NO_EXCEPTION, but using the macros makes this no worse. Adding hundreds of assertions all at once would risk creating lots of new problems for people doing development with debug builds, so we probably could want to do it only a few at a time.

review+ as long as you fix the one obviously wrong one below

>> Source/WebCore/page/DOMSelection.cpp:412
>> +        if (r->compareBoundaryPoints(Range::END_TO_START, range.get(), IGNORE_EXCEPTION) < 1 && !IGNORE_EXCEPTION) {
> 
> it's not ignoring the exception here, right?

Absolutely right. This one is wrong!
Comment 8 Mike West 2013-02-08 09:40:16 PST
(In reply to comment #7)
> (From update of attachment 187322 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187322&action=review
> 
> I think the vast majority of these should be ASSERT_NO_EXCEPTION instead. It’s kind of random where we are using IGNORE_EXCEPTION and where ASSERT_NO_EXCEPTION, but using the macros makes this no worse. Adding hundreds of assertions all at once would risk creating lots of new problems for people doing development with debug builds, so we probably could want to do it only a few at a time.

I agree. My hope is that posting these out to the list will help folks who know more about these specific callsites deal with them in a reasonable way. Sometimes it might make sense to ignore, or assert, or throw away the ExceptionCode entirely in favor of a different API.

> review+ as long as you fix the one obviously wrong one below
> 
> >> Source/WebCore/page/DOMSelection.cpp:412
> >> +        if (r->compareBoundaryPoints(Range::END_TO_START, range.get(), IGNORE_EXCEPTION) < 1 && !IGNORE_EXCEPTION) {
> > 
> > it's not ignoring the exception here, right?
> 
> Absolutely right. This one is wrong!

/me facepalm.

Looks like I was a little overzealous with the regexing.
Comment 9 Darin Adler 2013-02-08 10:02:30 PST
(In reply to comment #8)
> My hope is that posting these out to the list will help folks who know more about these specific callsites deal with them in a reasonable way.

That seems unlikely. Unless someone has a reason to work on the code already, this doesn’t seem like a hugely productive way of spending time.
Comment 10 Mike West 2013-02-09 00:27:46 PST
Created attachment 187421 [details]
Patch
Comment 11 WebKit Review Bot 2013-02-09 11:20:47 PST
Comment on attachment 187421 [details]
Patch

Clearing flags on attachment: 187421

Committed r142375: <http://trac.webkit.org/changeset/142375>
Comment 12 WebKit Review Bot 2013-02-09 11:20:53 PST
All reviewed patches have been landed.  Closing bug.