1157754 - ErrorResult should provide method to dispose of JS exception in addition to clearing message

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

We currently have a fair amount of code that calls ErrorResult::ClearMessage() after artificially consuming an ErrorResult in c++ code.  For example, if the code converts to nsresult with ErrorCode() or in dom/cache where ErrorResult is passed over IPC.

On IRC Boris suggested we should really be checking for JS exceptions in these cases as well.  So lets add a new method to do that.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Another case where ClearMessage() is used is when the error is effectively ignored, but the API in use requires an ErrorResult.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a way to "catch" an ErrorResult, and a way to safely convert an ErrorResult to an nsresult

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Convert consumers of ErrorResult::ClearMessage() to the new better APIs we have for suppressing exceptions on ErrorResult

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Make ClearMessage private on ErrorResult

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a way to "catch" an ErrorResult, and a way to safely convert an ErrorResult to an nsresult

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Convert consumers of ErrorResult::ClearMessage() to the new better APIs we have for suppressing exceptions on ErrorResult

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a way to "catch" an ErrorResult, and a way to safely convert an ErrorResult to an nsresult

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8596656 [details] [diff] [review]
part 2.  Convert consumers of ErrorResult::ClearMessage() to the new better APIs we have for suppressing exceptions on ErrorResult

Review of attachment 8596656 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/FetchPut.cpp
@@ +160,5 @@
>    MOZ_ASSERT(!mListener);
>    mManager->RemoveListener(this);
>    mManager->ReleaseCacheId(mCacheId);
> +  mResult.SuppressException(); // XXXbz should we really be ending up here with
> +                               // a failed mResult we never reported to anyone?

This is going away in bug 1120501.

@@ +389,5 @@
>          ErrorResult headerRv;
>          nsAutoCString value;
>          requestHeaders->Get(header, value, headerRv);
>          if (NS_WARN_IF(headerRv.Failed())) {
> +          headerRv.SuppressException();

This will remain after bug 1120501, but will require me to rebase. :-\

Comment 9

[Boris Zbarsky [:bzbarsky]]

I can wait for you to land bug 1120501 if you prefer.  It's not like this bug has all its reviews yet anyway.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a way to "catch" an ErrorResult, and a way to safely convert an ErrorResult to an nsresult

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a03d304acdb3
https://hg.mozilla.org/integration/mozilla-inbound/rev/c944fc7df692
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b8a27223558

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a03d304acdb3
https://hg.mozilla.org/mozilla-central/rev/c944fc7df692
https://hg.mozilla.org/mozilla-central/rev/4b8a27223558