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

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bkelly, Assigned: bzbarsky)

Tracking

unspecified
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

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

Comment 2

4 years ago
Created attachment 8596646 [details] [diff] [review]
part 1.  Add a way to "catch" an ErrorResult, and a way to safely convert an ErrorResult to an nsresult
Attachment #8596646 - Flags: review?(peterv)
(Assignee)

Updated

4 years ago
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Created attachment 8596648 [details] [diff] [review]
part 2.  Convert consumers of ErrorResult::ClearMessage() to the new better APIs we have for suppressing exceptions on ErrorResult
Attachment #8596648 - Flags: review?(bkelly)
(Assignee)

Comment 4

4 years ago
Created attachment 8596649 [details] [diff] [review]
part 3.  Make ClearMessage private on ErrorResult
Attachment #8596649 - Flags: review?(peterv)
(Assignee)

Comment 5

4 years ago
Created attachment 8596655 [details] [diff] [review]
part 1.  Add a way to "catch" an ErrorResult, and a way to safely convert an ErrorResult to an nsresult
Attachment #8596655 - Flags: review?(peterv)
(Assignee)

Updated

4 years ago
Attachment #8596646 - Attachment is obsolete: true
Attachment #8596646 - Flags: review?(peterv)
(Assignee)

Comment 6

4 years ago
Created attachment 8596656 [details] [diff] [review]
part 2.  Convert consumers of ErrorResult::ClearMessage() to the new better APIs we have for suppressing exceptions on ErrorResult
Attachment #8596656 - Flags: review?(bkelly)
(Assignee)

Updated

4 years ago
Attachment #8596648 - Attachment is obsolete: true
Attachment #8596648 - Flags: review?(bkelly)
(Assignee)

Comment 7

4 years ago
Created attachment 8596682 [details] [diff] [review]
part 1.  Add a way to "catch" an ErrorResult, and a way to safely convert an ErrorResult to an nsresult
Attachment #8596682 - Flags: review?(peterv)
(Assignee)

Updated

4 years ago
Attachment #8596655 - Attachment is obsolete: true
Attachment #8596655 - Flags: review?(peterv)
(Reporter)

Comment 8

4 years ago
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. :-\
Attachment #8596656 - Flags: review?(bkelly) → review+
(Assignee)

Comment 9

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

Updated

3 years ago
Blocks: 1157898
Created attachment 8597009 [details] [diff] [review]
part 1.  Add a way to "catch" an ErrorResult, and a way to safely convert an ErrorResult to an nsresult
Attachment #8597009 - Flags: review?(peterv)
(Assignee)

Updated

3 years ago
Attachment #8596682 - Attachment is obsolete: true
Attachment #8596682 - Flags: review?(peterv)
Attachment #8597009 - Flags: review?(peterv) → review+
Attachment #8596649 - Flags: review?(peterv) → review+
https://hg.mozilla.org/mozilla-central/rev/a03d304acdb3
https://hg.mozilla.org/mozilla-central/rev/c944fc7df692
https://hg.mozilla.org/mozilla-central/rev/4b8a27223558
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.