Bug 109295 – Use IGNORE_EXCEPTION for initialized, but unused, ExceptionCodes.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mike West]

Use IGNORE_EXCEPTION for Editor and TextCheckingHelper.

[Mike West]

Created attachment 187305 [details]
Patch

[Mike West]

Waiting until 108771 lands before throwing this to the bots.

[Mike West]

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.

[Mike West]

Created attachment 187322 [details]
Patch

[Mike West]

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 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?

[Darin Adler]

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!

[Mike West]

(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.

[Darin Adler]

(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.

[Mike West]

Created attachment 187421 [details]
Patch

[WebKit Review Bot]

Comment on attachment 187421 [details]
Patch

Clearing flags on attachment: 187421

Committed r142375: <http://trac.webkit.org/changeset/142375>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.