Bug 108180 – Cleanup: Normalize ExceptionCode handling.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mike West]

To ease the progress of my ongoing "sed-driven development", I'd like to normalize WebKit's handling of ExceptionCodes. Specifically, I would like:

* every callsite that might throw an exception to create ExceptionCode variables named "ec" (90% already do, the rest are "exception", "exceptionCode", "code", and so on).
* ExceptionCode member variables to be named "m_ec".
* every callsite that ignored exceptions to create ExceptionCode variables named "ignoredException" (~50% do; it's a wash between "unused", "ignored", and others).
* every method that accepts a pass-by-reference ExceptionCode to do so as "ExceptionCode& ec" (positioning of the "&" is variable).

If anything else pops up, I'll note it here. :)

[Mike West]

* ExceptionCodes should always be initialized to 0, even if they're (currently) ignored.

[Mike West]

Created attachment 185239 [details]
Patch

[Mike West]

Hi, Alexey. Apologies in advance, but you seem like a brilliant place to start for a review on this patch. I hope it mostly codifies practices that were already more or less in place in the code, and will make my (eventual) work on https://bugs.webkit.org/show_bug.cgi?id=98050 (which I haven't forgotten!) a bit simpler in the future.

I'll find reviewers for the port-specific portions of the patch, but I'd appreciate you reviewing the general concept, and the WebKit2 bits in particular.

Thanks!

---

Jochen, perhaps you could skim the Chromium bits?

[Build Bot]

Comment on attachment 185239 [details]
Patch

Attachment 185239 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16200190

Comment on attachment 185239 [details]
Patch

the chromium bits look good

[Eric Seidel (no email)]

I'm not sure if this is a win.  At least not all at once.  You shoudl also be aware of our ASSERT_NO_EXCEPTION object (or it may be named silghtly differnetly) which you pass instead of an ExceptionCode and it asserts that it came back 0.  Many of these callers were written before such existed.

I'm not a huge fan of m_ec over m_exceptionCode.

Since these are mostly mechanical, I would have done these one at a time.  Or less in one patch.  That's less controvertial.  For example, I would easily r+ a patch which set all of these to 0 on initialization as you've done.

That said... Arv (I think?) had an earlier patch which made exception codes classes and got rid of the actual integer value for many of them since newer DOM exceptions have no associated int and thus we are careful not to expose it... iirc.  If ExceptionCode was a class, it could have a constructor which did the inititalization anyway. :)

I guesss my point is that a larger cleanup patch like this is easy to bikeshed about.  Smaller single-idea patches are going to be much easier for someone to rubber stamp.

[Erik Arvidsson]

(In reply to comment #6)
> That said... Arv (I think?) had an earlier patch which made exception codes classes and got rid of the actual integer value for many of them since newer DOM exceptions have no associated int and thus we are careful not to expose it... iirc.  If ExceptionCode was a class, it could have a constructor which did the inititalization anyway. :)

That was actually Mike too.

[Mike West]

(In reply to comment #7)
> (In reply to comment #6)
> > That said... Arv (I think?) had an earlier patch which made exception codes classes and got rid of the actual integer value for many of them since newer DOM exceptions have no associated int and thus we are careful not to expose it... iirc.  If ExceptionCode was a class, it could have a constructor which did the inititalization anyway. :)
> 
> That was actually Mike too.

Right. I think Arv is (correct me if I'm wrong) replacing the ExceptionCode enum with one that doesn't directly encode the ID of the DOM exception. I'm in the process of figuring out how to replace the enum with a class without double-digit perf regressions. :)

[Alexey Proskuryakov]

Comment on attachment 185239 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        - Member variables are named 'm_ec'.

I'm with Eric here, this seems questionable.

> Source/WebCore/ChangeLog:26
> +        - ExceptionCode variables are always initialized to 0, even if they're
> +          not (yet) used to throw exceptions.

What is the win here? The rule has been that any code that cares about exceptionCode initializes it, and code that doesn't care doesn't initialize.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:429
> -                    ExceptionCode ec; // Exception (for sandboxed documents) ignored.
> +                    ExceptionCode ec = 0; // Exception (for sandboxed documents) ignored.

You were renaming these to ignoredExpection.

> Source/WebCore/dom/ContainerNode.cpp:-219
> -    if (ExceptionCode code = checkAcceptChild(newParent, newChild, 0)) {
> -        ec = code;
> +    ec = checkAcceptChild(newParent, newChild, 0);
> +    if (ec)
>          return false;
> -    }

This was strange code. The change may be correct (depends on call sites), but it's definitely not mechanical.

> Source/WebCore/dom/ContainerNode.cpp:-229
> -    if (ExceptionCode code = checkAcceptChild(newParent, newChild, oldChild)) {
> -        ec = code;
> +    ec = checkAcceptChild(newParent, newChild, oldChild);
> +    if (ec)
>          return false;
> -    }

Ditto.

> Source/WebCore/dom/Document.cpp:2849
> +            ExceptionCode ec = 0; // Exception (for sandboxed documents) ignored.

ignoredException

> Source/WebCore/dom/Node.cpp:629
> -            ExceptionCode ec;
> +            ExceptionCode ec = 0;
>              text->appendData(nextText->data(), ec);
>              document()->textNodesMerged(nextText.get(), offset);
>              nextText->remove(ec);

The usefulness of this initialization is particularly questionable, given that ex is not reset to 0 before the remove() call.

> Source/WebCore/editing/AlternativeTextController.cpp:710
> +    ExceptionCode ec = 0;

ignoredException.

> Source/WebCore/editing/CompositeEditCommand.cpp:898
> +    ExceptionCode ec = 0;

And so on... As Eric said, most of these may actually be cases for ASSERT_NO_EXCEPTION.

[Mike West]

Comment on attachment 185239 [details]
Patch

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

(In reply to comment #6)
> I'm not sure if this is a win.  At least not all at once.  You shoudl also be aware of our ASSERT_NO_EXCEPTION object (or it may be named silghtly differnetly) which you pass instead of an ExceptionCode and it asserts that it came back 0.  Many of these callers were written before such existed.

I'd like to make use of that, but without knowing the intentions of the original authors, it's difficult to just insert ASSERTions willy-nilly. Some of the places where the exceptions are ignored might very well have assertions as their meaning, but others might mean "I don't care, just do this thing."

> I'm not a huge fan of m_ec over m_exceptionCode.

If I had a dollar for every time I wasn't a fan of someone arguing for "Consistency"... :) But sure, I recognize that that might be controversial.

> Since these are mostly mechanical, I would have done these one at a time.  Or less in one patch.  That's less controvertial.  For example, I would easily r+ a patch which set all of these to 0 on initialization as you've done.

I can certainly split this out into multiple patches. Part of the reason I put these changes into one patch was the fact that they're fairly interrelated: if I change `ExceptionCode e;` to `ExceptionCode ec = 0;`, it just makes sense to do it in one step rather than two.

Given the ~5 changes I'm making here, which would you like to see split out for further discussion? "All five" is a valid answer, of course. :)

---

>> Source/WebCore/ChangeLog:18
>> +        - Member variables are named 'm_ec'.
> 
> I'm with Eric here, this seems questionable.

I'll split it out to argue about it. My argument will be that we've tacitly decided on 'ec' as a naming convention for stored exception codes, which makes spelling out 'exceptionCode' in certain circumstances (or shortening to 'code' or the like) the exception (ha!) rather than the role.

>> Source/WebCore/ChangeLog:26
>> +          not (yet) used to throw exceptions.
> 
> What is the win here? The rule has been that any code that cares about exceptionCode initializes it, and code that doesn't care doesn't initialize.

1) Uninitialized variables are a bad habit. I subscribe to the broken windows theory of coding. :)
2) If/when we change our minds about ignoring these exceptions, missing initializations are a possible source of error.

What's the downside to making the change?

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:429
>> +                    ExceptionCode ec = 0; // Exception (for sandboxed documents) ignored.
> 
> You were renaming these to ignoredExpection.

I renamed the ones I noticed. Thanks for catching a few that I didn't; I'll ensure they're changed on the next pass.

>> Source/WebCore/dom/ContainerNode.cpp:-219
>> -    }
> 
> This was strange code. The change may be correct (depends on call sites), but it's definitely not mechanical.

Arg. I did mean to call these two out as particularly non-mechanical.

These were the only instances of this pattern, and it seemed particularly strange. The change I've made has the side effect of setting `ec` to 0 if there's no exception, which didn't happen in the existing code. My assumption is that that would be a no-op, but you're right: I'll check the call sites to be sure.

>> Source/WebCore/dom/Node.cpp:629
>>              nextText->remove(ec);
> 
> The usefulness of this initialization is particularly questionable, given that ex is not reset to 0 before the remove() call.

This is likely another case where renaming to `ignoredException` would be reasonable.

[Mike West]

(In reply to comment #6)
> I guesss my point is that a larger cleanup patch like this is easy to bikeshed about.  Smaller single-idea patches are going to be much easier for someone to rubber stamp.

I'll start breaking this up into smaller patches, as you're right: some of the changes aren't as universally appreciated as I thought they'd be. :)

[Mike West]

Comment on attachment 185239 [details]
Patch

Dropping r? in favor of the following bugs:

http://wkbug.com/108766 for the non-mechanical change in ContainerNode::checkXxxChild().
http://wkbug.com/108769 for pass-by-reference style.
http://wkbug.com/108770 for assignment.
http://wkbug.com/108771 for making ignored exceptions explicit.

[Mike West]

(In reply to comment #12)
> (From update of attachment 185239 [details])
> Dropping r? in favor of the following bugs:
> 
> http://wkbug.com/108766 for the non-mechanical change in ContainerNode::checkXxxChild().
> http://wkbug.com/108769 for pass-by-reference style.
> http://wkbug.com/108770 for assignment.
> http://wkbug.com/108771 for making ignored exceptions explicit.

http://wkbug.com/109044 for replacing ExceptionCode assertions with the ASSERT_NO_EXCEPTION macro.