create a copyable read-only ErrorResult to make reporting easier across IPC actors, MozPromise, etc

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

In bug 1293277 I am building a fairly large network of IPC actors to implement the DOM Clients API.  There are many cases where I'd like to be able to report a TypeError with a good message string from one process to another.  Right now this is quite hard since the ErrorResult does not have a copy constructor.  Specifically it means:

1. You can't use ErrorResult in ipdl structs or unions.
2. You can't capture an ErrorResult in a c++11 lambda.

I spoke with Boris in IRC a bit today.  We thought perhaps we could make a wrapper type like ReadOnlyErrorResult or CopyableErrorResult.  This type would:

1. Wrap a normal ErrorResult value.
2. Provide a copy constructor that clones the underlying value.
3. Implement the "ignored" behavior and auto-suppress the underlying value.
4. Not support errors with js stacks.
What do you think of this approach?  It just layers the auto-clone behavior on top of the IgnoredErrorResult auto-suppress.
Attachment #8890086 - Flags: feedback?(bzbarsky)
Comment on attachment 8890086 [details] [diff] [review]
Add CopyableErrorResult to opt in to auto-cloning ErrorResult values. r=bz

This seems fine, but you should probably add some asserts in the case of JS exceptions, if you want to move these things across threads, right?
Attachment #8890086 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Boris Zbarsky [:bz] from comment #2)
> This seems fine, but you should probably add some asserts in the case of JS
> exceptions, if you want to move these things across threads, right?

For some reason I thought CloneTo() did this, but it doesn't.  Maybe it used to.

Anyway, to do this I will need to add another template parameter to TErrorResult, I think.  Either I need to:

1. Add a "read-only" parameter and then assert for no exceptions at creation time.
2. Or add a "no js exceptions" parameters and assert if one is written into the object.

I think I will probably go with (2).
That seems reasonable.

And please add documentation about when it's appropriate to use this class, because we don't want people to be using it unless they really have to: it makes it too easy to accidentally suppress exceptions.
Unfortunately its not adequate to add a template policy change since we reinterpret_cast these values to ErrorResult.  The templated value would simply be lost.  I could add a member to store some information, but that may have some runtime perf costs and I seem to recall this is used in a hot path.

Instead I just added a check in the copy operation.  I MOZ_DIAGNOSTIC_ASSERT() and fallback to suppress/NS_ERROR_FAILURE in release builds.  If you like I could MOZ_CRASH() in all branches instead.
Attachment #8890086 - Attachment is obsolete: true
Attachment #8890557 - Flags: feedback?(bzbarsky)
Comment on attachment 8890557 [details] [diff] [review]
Add CopyableErrorResult to opt in to auto-cloning ErrorResult values. r=bz

Looks reasonable, though I'd do operator= like so:

    MOZ_DIAGNOSTIC_ASSERT(!aRight.IsJSException(),
                          "Attempt to copy ErrorResult with a JS exception value.");
    if (aRight.IsJSException() {
      Throw(NS_ERROR_FAILURE);
    } else {
      aRight.CloneTo(*this);
    }
    return *this.

along with the comments.
Attachment #8890557 - Flags: feedback?(bzbarsky) → feedback+
This gets me to something I can use in ipdl structs.  I had to add an operator==() as well.  It compiles, but I doubt I am doing the right thing for the union members.
Attachment #8890557 - Attachment is obsolete: true
Priority: -- → P3
I'm going to punt on this for the time being and come back to it later.
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
Blocks: 1412856
No longer blocks: 1293277
Blocks: 1436812
No longer blocks: 1436812
I discussed this a bit more with Boris in #content today.  Another use case I ran into is rejecting a MozPromise with an ErrorResult-like value.  We discussed maybe factoring out the message bits from ErrorResult so those could be used independently with less strict assertions, etc.
Summary: create a copyable read-only ErrorResult to make reporting easier across IPC actors → create a copyable read-only ErrorResult to make reporting easier across IPC actors, MozPromise, etc
Specifically, factoring out TErrorResult::Message and the methods that operate on it.  Perhaps those methods could operate on an (nsresult&, Message&) tuple and we could use mozilla::Tie() to bind that to the members of the TErrorResult or whatever the other consumers would want?
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #10)
> Specifically, factoring out TErrorResult::Message and the methods that
> operate on it.  Perhaps those methods could operate on an (nsresult&,
> Message&) tuple and we could use mozilla::Tie() to bind that to the members
> of the TErrorResult or whatever the other consumers would want?

I don't really like this option.  We have so many APIs that take ErrorResult.  It would mean going through a roto-tilling them all to accept a new type.
Why?  We could have a thing that takes an ErrorResult and extracts this pair from it.  So you pass ErrorResult to the API, then extract the guts.

Yes, that means you have to think about what happens if the ErrorResult is not a Message.  Which seems right to me... if the callee can throw something other than a Message, then you need to decide how to handle the case when they do, right?
Maybe I don't understand your proposal Boris.

What I need is to express some spec-defined algorithm in the parent process far from any js global.  Something like:

  typedef MozPromise<SpecThing, SpecErrorResult> SpecPromise;

  RefPtr<SpecPromise>
  DoSpecAlgorithm(const SpecArgs& aArgs);

The spec may require us to reject the algorithm with non-message errors like InvalidStateError or SecurityError.  It may also require us to reject with TypeError in which case we want to provide a message.

Ultimately the result of the MozPromise needs to be propagated back across the IPC boundary and to the binding layer.  So SpecErrorResult needs to populate a binding layer ErrorResult after being copied across IPC.

How would your proposal allow me to define an API that works with message and non-message errors?

Also, there are lots of APIs within gecko that use nsresult and now ErrorResult.  It would be nice not to have to change all these to support a new kind of error type.  I think creating a third separate error API ecosystem would be a bad idea.  We should be trying to move towards a unified API to avoid making the situation worse.
To clarify what I mean about "other APIs"... What if DoSpecAlgorithm wants to do something like call InternalHeaders::Append() which takes an ErrorResult?
OK.  So what should happen if DoSpecAlgorithm does something that results in an actual JS exception?
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> OK.  So what should happen if DoSpecAlgorithm does something that results in
> an actual JS exception?

How can it do that if its in a different process without a JSContext or a global?  I don't understand the question.

All the specs I have worked with that require this kind of implementation throw DOM errors like TypeError or SecurityError.  I don't have an example that produces an js exception like you are asking about.
> How can it do that if its in a different process without a JSContext or a global? 

That's an implementation detail, right?  And one that may be subject to change...  What happens if we end up implementing part of the spec algorithm via a call to chrome JS (in the other process, whatever)?

Basically, as I see it, we have three options here conceptually:

1)  Have some sort of dynamic check for JS exceptions, deal somehow.
2)  Have some sort of dynamic check for JS exceptions, MOZ_RELEASE_ASSERT they don't happen.
3)  Have a static way (via the type system) of indicating that some algorithm will not
    throw a JS exception.

Option 3 does involve creating a sort of third error API ecosystem, though we could easily make it possible to pass an ErrorResult to methods that use whatever this "no JS exceptions" type is.  We already have that sort of thing with OOMReporter.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #17)
> > How can it do that if its in a different process without a JSContext or a global? 
> 
> That's an implementation detail, right?  And one that may be subject to
> change...  What happens if we end up implementing part of the spec algorithm
> via a call to chrome JS (in the other process, whatever)?

Well, if the spec requires us to through an explicit DOM error, I think we would have to use that regardless of internal implementation.

Anyway...

The patch here kind of does what we are talking about.  I'm trying to see if it will solve my problem.
Assignee: nobody → bkelly
Attachment #8970596 - Attachment is obsolete: true
Status: NEW → ASSIGNED
I need something like this to properly fix the MozPromise usage in bug 1456466.
Blocks: 1456466
Of course this doesn't work because we hit this static analysis failure:

[task 2018-04-24T18:05:30.041Z] 18:05:30     INFO -  /builds/worker/workspace/build/src/dom/serviceworkers/ServiceWorkerRegistration.cpp:230:37: error: Type 'mozilla::CopyableErrorResult' must not be used as parameter
[task 2018-04-24T18:05:30.041Z] 18:05:30     INFO -      }, [outer] (CopyableErrorResult aRv) {
[task 2018-04-24T18:05:30.041Z] 18:05:30     INFO -                                      ^
[task 2018-04-24T18:05:30.041Z] 18:05:30     INFO -  /builds/worker/workspace/build/src/dom/serviceworkers/ServiceWorkerRegistration.cpp:230:37: note: Please consider passing a const reference instead
[task 2018-04-24T18:05:30.042Z] 18:05:30     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ErrorResult.h:661:7: note: 'mozilla::CopyableErrorResult' is a non-param type because it inherits from a non-param type 'binding_danger::TErrorResult<binding_danger::ThreadSafeJustSuppressCleanupPolicy>'
[task 2018-04-24T18:05:30.042Z] 18:05:30     INFO -  class CopyableErrorResult :
[task 2018-04-24T18:05:30.042Z] 18:05:30     INFO -        ^
[task 2018-04-24T18:05:30.042Z] 18:05:30     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ErrorResult.h:516:3: note: 'binding_danger::TErrorResult<binding_danger::ThreadSafeJustSuppressCleanupPolicy>' is a non-param type because member '' is a non-param type 'union (anonymous union at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ErrorResult.h:516:3)'
[task 2018-04-24T18:05:30.042Z] 18:05:30     INFO -    union {
[task 2018-04-24T18:05:30.042Z] 18:05:30     INFO -    ^
[task 2018-04-24T18:05:30.042Z] 18:05:30     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ErrorResult.h:518:28: note: 'union (anonymous union at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ErrorResult.h:516:3)' is a non-param type because member 'mJSException' is a non-param type 'JS::UninitializedValue'
[task 2018-04-24T18:05:30.042Z] 18:05:30     INFO -      JS::UninitializedValue mJSException; // valid when IsJSException()

I don't see an easy way around this.  Maybe I will just give up on trying to preserve error messages here.  I just need to get my main task done and not rewrite error processing through the entire gecko system.
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
No longer blocks: 1456466
Lets try one more thing.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Blocks: 1456466
Comment on attachment 8970623 [details] [diff] [review]
Add CopyableErrorResult to opt in to auto-cloning ErrorResult values. r=bz

Boris, this patch is my attempt to meet the needs of the use case in comment 13.  Specifically, it allows code to do:

  typedef MozPromise<SpecResult, CopyableErrorResult> SpecPromise;

And directly Reject() such a promise with nsresult or a normal ErrorResult.

A diagnostic assertion checks that a js exception is not used.  On release builds js exceptions are converted to NS_ERROR_FAILURE.

Errors are auto-suppressed similar to IgnoredErrorResult.  The logic here is that if you end up copying an ErrorResult many times the consumption check no longer makes a lot of sense.  We really want to check that at least one is consumed, but we have no way of knowing which one (or ones) that will be.

I also added a template flag to disable the owning thread assertions.  Since we don't permit js exceptions this class should be thread safe.

The one annoyance that remains is that you cannot do:

  void HandleError(CopyableErrorResult aStatus);

This fails because of the JS::UninitializedValue being MOZ_NON_PARAM.  Even though we are excluding it at runtime I don't see an easy way to escape the static analysis.

For now code will have to pass by reference instead even though its called "copyable".
Attachment #8970623 - Flags: review?(bzbarsky)
Maybe we should call this "InternalErrorResult" or "NativeErrorResult" to indicate its c++-only use.
Rebase on top of the recent changes adding "mExtra".

See comment 25 for the original review description.
Attachment #8970623 - Attachment is obsolete: true
Attachment #8970623 - Flags: review?(bzbarsky)
Attachment #8971271 - Flags: review?(bzbarsky)
> This fails because of the JS::UninitializedValue being MOZ_NON_PARAM.

This is going to change to be a JS::Value, but that's MOZ_NON_PARAM too...
Comment on attachment 8971271 [details] [diff] [review]
Add CopyableErrorResult to opt in to auto-cloning ErrorResult values. r=bz

>+  void CloneToInternal(TErrorResult& aRv) const;

This should be documented, in particular how it differs from CloneTo.

>+binding_danger::TErrorResult<CleanupPolicy>::operator==(const ErrorResult& aRight) const

So this tests for pointer-identity of the DOMException and Message.  Why is that a useful thing to test?

>+      aRight.CloneTo(*this);

Should this be CloneToInternal?

>+    MOZ_DIAGNOSTIC_ASSERT(!IsJSException(),

This should be asserting !aRight.IsJSException() too, right?

r=me.  When you check this in, send an email to DOM peers warning them to watch out for this class being used inappropriately, please.
Attachment #8971271 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #29)
> Comment on attachment 8971271 [details] [diff] [review]
> Add CopyableErrorResult to opt in to auto-cloning ErrorResult values. r=bz
> 
> >+  void CloneToInternal(TErrorResult& aRv) const;
> 
> This should be documented, in particular how it differs from CloneTo.

> >+      aRight.CloneTo(*this);
> 
> Should this be CloneToInternal?

Actually, now that I added the templated assertSameThread thing I can remove CloneToInternal() completely.

> 
> >+    MOZ_DIAGNOSTIC_ASSERT(!IsJSException(),
> 
> This should be asserting !aRight.IsJSException() too, right?

Yes.  I think I intended to just assert aRight.IsJSException().

> r=me.  When you check this in, send an email to DOM peers warning them to
> watch out for this class being used inappropriately, please.

Sure.  I was planning to email dev-platform about it.
Removed CloneToInternal().

Made operator==() do a deep comparison on the message types.
Attachment #8971271 - Attachment is obsolete: true
Attachment #8971352 - Flags: review+
> Actually, now that I added the templated assertSameThread thing I can remove CloneToInternal() completely.

Hmm.  I guess the aRv.AssertInOwningThread() thing it does will be OK because that's also a CopyableErrorResult statically.  OK.

> Yes.  I think I intended to just assert aRight.IsJSException().

I think you need to assert both.  If "this" has a JS exception at this point, bad things will happen...

I do wonder a bit what the use case is for the operator==.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #32)
> > Yes.  I think I intended to just assert aRight.IsJSException().
> 
> I think you need to assert both.  If "this" has a JS exception at this
> point, bad things will happen...

Ok, I'll add that back.

> I do wonder a bit what the use case is for the operator==.

The use case is that ipdl generated struct and union types require an operator==().
Assert !IsJSException() on both sides of the assignment.
Attachment #8971352 - Attachment is obsolete: true
Attachment #8971368 - Flags: review+

Comment 35

Last year
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f557f72f5d46
Add CopyableErrorResult to opt in to auto-cloning ErrorResult values. r=bz

Comment 36

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/f557f72f5d46
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1471303
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.