Bug 108771 – Add a new IGNORE_EXCEPTION helper to ignore ExceptionCodes when they are expected but uninteresting

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

Hi Alexey and Eric, please feel free to CC whoever's relevant. :)

Currently, we have at least three mechanisms of indicating that a DOM exception will be ignored:

1. The 'ec' variable is uninitialized (according to ap@ here: https://bugs.webkit.org/show_bug.cgi?id=108180#c9).
2. A comment notes "// Exceptions ignored." or something to that effect (see http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/websockets/WebSocketChannel.cpp?rev=141715#L429 for example).
3. The ExceptionCode is named something like 'ignoredException' or 'unused'.

We also have one mechanism of ensuring that no exception occurs, while ignoring the result in release: ASSERT_NO_EXCEPTION.

I'd like to standardize on either one or two mechanisms. One mechanism that seems appealing is to create a new macro, IGNORE_EXCEPTION, which would use the same framework as ASSERT_NO_EXCEPTION, only without the ASSERT. That could be a drop-in replacement for all the instances currently in place where the exception is ignored; some (many?) should probably be ASSERT_NO_EXCEPTION instead, but that's tough to tell without closely examining each call site. A new macro would give clarity immediately without changing behavior.

WDYT?

[Mike West]

Created attachment 186264 [details]
Patch

[Mike West]

Attached is a "theory" patch. If you approve of the approach, I'll move on to "practice". If not, I'd appreciate alternative suggestions.

Thanks!

[Eric Seidel (no email)]

Comment on attachment 186264 [details]
Patch

LGTM.  I'll be sure to CC Darin Adler as he often has opinions on changes like this.

[Eric Seidel (no email)]

A slightly tighter programming style would have been for this code to instead ASSERT that the exception on came in the times it would expect.  Or to only call this when it could expect not ASSERT, but it's possible that breaks layering.

[Eric Seidel (no email)]

This is also a symptom of often not having enough impl-specific methods, which avoid the JS-APIs entirely.  It seems to me that most of the time the impl does not want to be using the JS-APIs which have exception codes.

[Mike West]

Ok. If we go this route, it might moot some of the disagreement on https://bugs.webkit.org/show_bug.cgi?id=108770, so I'll flesh this out first to see how many call sites are effected and put it up for re-review then.

Regarding the questions of elegance, I agree completely: ignoring exceptions seems like it has to be the wrong thing to do. But it's what we're doing now, so I simply want to make that problem explicit.

As you say, non-JS implementations are probably the right answer in most cases. It'll be easier to evaluate that once the callsites are clearly greppable.

[Eric Seidel (no email)]

I agree that this patch doesn't make "the problem" any worse, and if anything makes it more explicit and easier to grep for. :)

[Alexey Proskuryakov]

> Hi Alexey and Eric, please feel free to CC whoever's relevant. :)

In addition to CC'ing, perhaps you'll want to e-mail webkit-dev about this set of patches. Whatever the changes agreed upon will be, they need to be widely communicated.

[Mike West]

Created attachment 186395 [details]
Patch

[Mike West]

Attached is a stab at adding the macro, and applying it to WebCore. I'm sure I've missed a few, but I'll hopefully track them down in subsequent changes, since I suspect I'll be playing with ExceptionCode a bit more this week.

Alexey, I wasn't sure it would be worthwhile to get everyone involved on discussion of a direction, but if this macro makes sense to you folks, I'm happy to propose it on the list.

Thanks!

[Eric Seidel (no email)]

Comment on attachment 186395 [details]
Patch

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

This is amazing.  The unfortunate thing is that basically every one of these callers is wrong in my mind.  At least this macro makes them super easy to grep for.  But we should probably send a PSA to webkit-dev noting how many places we ignore exceptions and noting that this is bad, and that folks shoudl grep for IGNORE_EXCEPTION and fix them. :(

> Source/WebCore/editing/AppendNodeCommand.cpp:63
> +    m_parent->appendChild(m_node.get(), IGNORE_EXCEPTION, true /* lazyAttach */);

There must be a better API for us to call than this...  I think IGNORE_EXCEPTOIN is just basically a "FIXME" with a different name. :)

[Mike West]

Created attachment 186396 [details]
Patch

[Mike West]

(In reply to comment #11)
> (From update of attachment 186395 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186395&action=review
>
> This is amazing.  The unfortunate thing is that basically every one of these callers is wrong in my mind.  At least this macro makes them super easy to grep for.  But we should probably send a PSA to webkit-dev noting how many places we ignore exceptions and noting that this is bad, and that folks shoudl grep for IGNORE_EXCEPTION and fix them. :(

I agree.

> > Source/WebCore/editing/AppendNodeCommand.cpp:63
> > +    m_parent->appendChild(m_node.get(), IGNORE_EXCEPTION, true /* lazyAttach */);
> 
> There must be a better API for us to call than this...  I think IGNORE_EXCEPTOIN is just basically a "FIXME" with a different name. :)

I debated calling it FIXME_BECAUSE_I_SHOULDNT_BE_IGNORING_EXCEPTIONS, but that seemed verbose. :)  Suggestions would be ever so welcome.

[Darin Adler]

(In reply to comment #11)
> The unfortunate thing is that basically every one of these callers is wrong in my mind.

I think you need to explain this point better. There are plenty of good reasons to ignore exceptions, just as there are good reasons to ignore other types of return values.

I need to understand the goals a bit better to have an opinion on this.

The fact that the "ec" variable is uninitialized is technically not a way to indicate that an exception is to be ignored; it's a way to avoid generating excess code in the case where an exception is uninteresting. But it's true that if it's uninitialized then the exception *must* be ignored, because there’s no reliable way to detect it.

[Mike West]

(In reply to comment #14)
> (In reply to comment #11)
> > The unfortunate thing is that basically every one of these callers is wrong in my mind.
> 
> I think you need to explain this point better. There are plenty of good reasons to ignore exceptions, just as there are good reasons to ignore other types of return values.

Rather than ignoring exceptions, I'd prefer that we have non-DOM versions of these methods for cases where we don't care about the restraints placed upon the interface we expose to the DOM. The changes for Range::* that you reviewed in https://bugs.webkit.org/show_bug.cgi?id=108773 are what I'd like to see for each of these callsites.

> I need to understand the goals a bit better to have an opinion on this.

My goal is to make our usage of ExceptionCode explicit throughout the codebase. If we're ignoring an exception, I'd like to clearly see that from the code. I don't think that the practice of not initializing the 'ec' variable isn't explicit enough to give someone who's not familiar with the code a good overview of what's actually going on.

> But it's true that if it's uninitialized then the exception *must* be ignored, because there’s no reliable way to detect it.

I've found at least three places where 'ExceptionCode ec;' has been followed by 'ASSERT(!ec);'. That's another part of what I'd like to be fixing with this patch and others.

[Mike West]

Created attachment 187113 [details]
Patch

[Mike West]

Created attachment 187116 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 187116 [details]
Patch

This is a huge change. :(  But LGTM.

[Mike West]

I'm happy to split it up if you can suggest a division that makes sense. :)

[Eric Seidel (no email)]

Eh.  I's OK.  I read through it.  You could also litterally cut it in half if you wanted, but it doesn't really matter.

[Mike West]

Comment on attachment 187116 [details]
Patch

Though I generally agree that smaller patches are better, I'd prefer to CQ this all at once. If it breaks things (I don't believe it will), I'd rather revert just change.

[WebKit Review Bot]

Comment on attachment 187116 [details]
Patch

Clearing flags on attachment: 187116

Committed r142271: <http://trac.webkit.org/changeset/142271>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.