Bug 108773 – Cleanup: Use exceptionless Range::* methods rather than ignoring exceptions.

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

[Mike West]

While working through http://wkbug.com/108180, I've seen many, many uses of Range::startContainer(ExceptionCode&) where the code is either asserted to be 0, or ignored. In these cases, we should simply use the non-ExceptionCode version of the method. The same applies to other Range::* methods, but startContainer is the most prolific.

[Mike West]

Created attachment 186311 [details]
WIP

[Eric Seidel (no email)]

Comment on attachment 186311 [details]
WIP

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

> Source/WebCore/dom/DocumentMarkerController.cpp:99
>          int exception = 0;

Should remove this, no?

> Source/WebCore/dom/DocumentMarkerController.cpp:573
> +    ExceptionCode ec = 0;

Do we not have endOffset/startOffset non-ec versions?

> Source/WebCore/editing/TextCheckingHelper.cpp:278
> +                misspellingRange->startContainer()->document()->markers()->addMarker(misspellingRange.get(), DocumentMarker::Spelling);

Does startContainer do the ASSERTing for us?  I assume it's just going to return null when failing?

[Mike West]

Note that I only uploaded this so I'd have the patch at work. :)

Regarding the start container(ec)->document() code, it's broken as written. The assert(!ec) is the only assert, and it comes too late. Adding an assert at the call site might be reasonable if the call can actually return 0. I plan on poking rniwa on IRC as I suspect he'll know.

[Mike West]

Created attachment 186321 [details]
Patch

[Mike West]

Ok, this is looking better, I think. When we ignore the ExceptionCode set by calls to Range::{start,end}{Offset,Container} and Range::collapsed, this patch simply uses the non-ExceptionCode version of these methods.

One pattern in particular looks dangerous:

    ExceptionCode ec = 0;
    range->startContainer(ec)->document()->...;
    ASSERT(!ec);

This, quite simply, doesn't work: if 'startContainer()' throws an exception, it does so while at the same time returning 0. The ASSERT comes too late, as we've already tried to deref 0. If 'startContainer()' can return 0 in these cases, we ought to ASSERT that it doesn't, rather than asserting that no exception was thrown. The current patch does that in the cases where the code currently asserts that no exception triggered, but doesn't where the current code ignored the exception entirely. I'm happy to either remove the ASSERTs I've added if they're unnecessary, or add ASSERTs to the cases I've skipped if they are.

rniwa@, would you mind taking a look at this patch? I think you probably know the relevant code pretty well.

Thanks!

[Eric Seidel (no email)]

Comment on attachment 186321 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1872
> -    return VisiblePosition(Position(it.range()->endContainer(ec), it.range()->endOffset(ec), Position::PositionIsOffsetInAnchor), UPSTREAM);
> +    return VisiblePosition(Position(it.range()->endContainer(), it.range()->endOffset(), Position::PositionIsOffsetInAnchor), UPSTREAM);

Does passing in a non-0 ec change behavior of endContainer/endOffset in a way we'd want?  If selectNodeContents gave an ec before, we'd be passing it in here.

> Source/WebCore/editing/EditorCommand.cpp:216
> +    if (newRange->collapsed())

It's not clear to me when collapsed can even return an ec.

[Mike West]

Comment on attachment 186321 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1872
>> +    return VisiblePosition(Position(it.range()->endContainer(), it.range()->endOffset(), Position::PositionIsOffsetInAnchor), UPSTREAM);
> 
> Does passing in a non-0 ec change behavior of endContainer/endOffset in a way we'd want?  If selectNodeContents gave an ec before, we'd be passing it in here.

No. The exception has no effect on the code. Range::endContainer/Offset just checks to see whether a local variable exists: if it doesn't, we throw an exception and return 0 (http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Range.cpp#L159).

>> Source/WebCore/editing/EditorCommand.cpp:216
>> +    if (newRange->collapsed())
> 
> It's not clear to me when collapsed can even return an ec.

If 'startContainer()' is null, it throws an INVALID_STATE_ERR and returns 0: http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Range.cpp#L190

It seems like these should be ASSERTs inside the Range class, honestly, but since I don't know that code at all, I'm loath to suggest changes.

[Darin Adler]

Comment on attachment 186321 [details]
Patch

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

Making a change like this is a good way to finish what I started when I created the non-exception versions of some of these functions for internal use.

It might possibly be a defensible design for the DOM Range to have a special detached state that throws exceptions whenever you try to do anything other than overwriting the range. However, when you think about it it's not great. It violates the Liskov substitution principle: A Range that can’t do any of the things that a Range normally does is kind of irritating to program with.

For use inside WebKit, it is better to have a class that has a more typical design for our project rather than this unusually strict contract that was chosen for the DOM. For a detached range, functions should do something logical rather than always throwing exceptions or asserting so we can program without always having to write extra code for exceptional cases. For example, the start container function can return null, offsets can be zero, and other functions can do things that are sensible for an empty range with nothing in it. That’s what I had in mind when creating the startContainer function with no exception argument. An alternative would creating a separate Range class for the many internal uses inside WebKit. Those have little reason to be burdened with the strange design choices of the public DOM Range class. That something we may still need to do soon for other reasons, if we change internal representation so that expressing everything with DOM nodes is costly.

But Range::collapsed is not like the other functions here. The ASSERT_NO_EXCEPTION in the header creates a function that asserts if the Range is detached. That's not the behavior that is most useful for an internal class and there’s a reason we don’t assert in the startContainer function. Instead, it would be helpful to have a function returns true when called on a detached Range. I suggest creating a new function named isCollapsed with that behavior, and removing the ASSERT_NO_EXCEPTION from the collapsed function that we have left there for the DOM’s benefit. Or we could change the return value from collapsed for the detached case, since it’s ignored by the JavaScript language bindings anyway, and continue to call collapsed, maybe with IGNORE_EXCEPTION instead of ASSERT_NO_EXCEPTION.

> Source/WebCore/editing/EditorCommand.cpp:274
> +    return Range::create(a->startContainer()->ownerDocument(), start->startContainer(), start->startOffset(), end->endContainer(), end->endOffset());

It's nutty that this code says a->startContainer()->ownerDocument() here. It could be a->ownerDocument() or a->startContainer()->document() or even start->startContainer()->document().

> Source/WebCore/editing/TextCheckingHelper.cpp:278
> +                ASSERT(misspellingRange->startContainer());

Good in theory to write a new assertion to replace the old one. But this exception isn’t helpful -- you should just omit it.

> Source/WebCore/editing/TextCheckingHelper.cpp:442
> +            ASSERT(badGrammarRange->startContainer());

Good in theory to write a new assertion to replace the old one. But this exception isn’t helpful -- you should just omit it.

[Mike West]

(In reply to comment #8)
> (From update of attachment 186321 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186321&action=review
> 
> Making a change like this is a good way to finish what I started when I created the non-exception versions of some of these functions for internal use.

Great. And thank you for the detailed description. I appreciate it, as it makes the design decisions clearer.

> But Range::collapsed is not like the other functions here.

I'll split 'collapsed' out into a separate patch: I'd suggest following the same convention as the other exceptionless functions: overload 'collapsed' with one exceptionless and one exceptionful method, and adjust the current callsites to explicitly use ASSERT_NO_EXCEPTION.

[Mike West]

Created attachment 186565 [details]
Patch

[Mike West]

(In reply to comment #10)
> Created an attachment (id=186565) [details]
> Patch

Extracted collapsed into https://bugs.webkit.org/show_bug.cgi?id=108921.

[WebKit Review Bot]

Comment on attachment 186565 [details]
Patch

Rejecting attachment 186565 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-04', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 186565, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
ebkit-commit-queue/Source/WebKit/chromium/testing/gtest --revision 629 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
24>At revision 629.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/16371660

[Mike West]

Created attachment 186572 [details]
Patch

[Mike West]

Comment on attachment 186572 [details]
Patch

Ugh. Not my morning... screwed up this patch as well. :/

[Mike West]

Created attachment 186575 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 186575 [details]
Patch for landing

Clearing flags on attachment: 186575

Committed r141877: <http://trac.webkit.org/changeset/141877>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.