Closed Bug 1157754 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bkelly, Assigned: bzbarsky)

References

Details

Attachments

(3 files, 4 obsolete files)

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.
Another case where ClearMessage() is used is when the error is effectively ignored, but the API in use requires an ErrorResult.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8596646 - Attachment is obsolete: true
Attachment #8596646 - Flags: review?(peterv)
Attachment #8596648 - Attachment is obsolete: true
Attachment #8596648 - Flags: review?(bkelly)
Attachment #8596655 - Attachment is obsolete: true
Attachment #8596655 - Flags: review?(peterv)
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+
I can wait for you to land bug 1120501 if you prefer.  It's not like this bug has all its reviews yet anyway.
Blocks: 1157898
Attachment #8596682 - Attachment is obsolete: true
Attachment #8596682 - Flags: review?(peterv)
Attachment #8597009 - Flags: review?(peterv) → review+
Attachment #8596649 - Flags: review?(peterv) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.