Summary: | Use IGNORE_EXCEPTION for initialized, but unused, ExceptionCodes. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike West <mkwst> | ||||||||
Component: | New Bugs | Assignee: | 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
Mike West
2013-02-08 06:09:13 PST
Created attachment 187305 [details]
Patch
Waiting until 108771 lands before throwing this to the bots. 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. Created attachment 187322 [details]
Patch
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? 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! (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. (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. Created attachment 187421 [details]
Patch
Comment on attachment 187421 [details] Patch Clearing flags on attachment: 187421 Committed r142375: <http://trac.webkit.org/changeset/142375> All reviewed patches have been landed. Closing bug. |