1434686 - Rejigger how IgnoredErrorResult works

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Over in 1434318 comment 19, Nika suggested that we have a class like this:

  class IgnoreRV
  {
  public:
    operator ErrorResult&() && { return mInner; }
  private:
    IgnoredErrorResult mInner;
  };

to be used like so:

  foo->Bar(IgnoreRV());

in places where one would currently do:

  IgnoredErrorResult rv;
  foo->Bar(rv);

I actually like this idea a lot, especially because it avoids the "IgnoredErrorResult being reused after it got thrown on" problem.  In fact, I think we should completely rejigger IgnoredErrorResult (possibly renaming it) to work like this.  If we do something like the GuardObjectNotifier but in the opposite direction (asserting that the class _is_ used as a temporary), we could end up with:

  foo->Bar(IgnoredErrorResult());

possibly with a better name (IgnoreRV is fairly nice).  If we do a different name, we can convert incrementally.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Or maybe we can have a MOZ_TEMPORARY_CLASS static analysis thing, instead of or in addition to the GuardObject bits.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #1)
> Or maybe we can have a MOZ_TEMPORARY_CLASS static analysis thing, instead of
> or in addition to the GuardObject bits.

We already have MOZ_NON_TEMPORARY_CLASS which asserts that the type is never used as a temporary. Inverting that for a MOZ_TEMPORARY_CLASS analysis would be pretty trivial.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I think there are a class of use cases where:

1. Its safe to ignore the result because its impossible for a js exception to be thrown.
2. You want to test for failure to take some action, but its just an nsresult check.

I think some of these cases are using IgnoredErrorResult to avoid having to use SuppressException().

I never really liked this use because the name is weird since the result is not ignored.  But it would be nice to support this case.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

The biggest risk which exists here is probably something like:

ErrorResult& ignore = IgnoreRV();

which wouldn't trivially be caught by that analysis. I wouldn't be too hard to require that the coersion is only used to pass as an argument to another function.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> But it would be nice to support this case.

There are exceptions other than JS exceptions that need to be explicitly suppressed.  That's why ErrorResult has the asserts that fire if you don't SuppressException.

It's hard to say what the right ergonomic tradeoff here is between "let people write simple code that Just Works" and "keep people from accidentally shooting themselves in the foot".

> ErrorResult& ignore = IgnoreRV();

I'd hope that would at least raise reviewer eyebrows.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I do like the proposal, quite a lot.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> possibly with a better name 

smaug suggests IgnoreError(), which sounds good to me.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Well, winnt.h helpfully has:

  typedef enum _CM_ERROR_CONTROL_TYPE {
    IgnoreError=SERVICE_ERROR_IGNORE,

so we need a different name or explicit mozilla::IgnoreError() everywhere.  :(

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

IgnoredError() then?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Introduce a mozilla::IgnoreErrors which can be used as a temporary to pass to an ErrorResult& arg when the error should be ignored

Once bug 1434689 is fixed, we can mark this thing as being required to be a temporary.

MozReview-Commit-ID: 7VX0XSYVOc4

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Use IgnoreErrors() in dom/bindings

MozReview-Commit-ID: G8vxR2s2qUJ

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Use IgnoreErrors() outside of dom/

I left some IgnoredErrorResults for now where people warn on failure.  We could
consider adding a WarnOnError() thing or something.

MozReview-Commit-ID: L5ttZ9CGKg0

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Use IgnoreErrors() in dom/

MozReview-Commit-ID: GwVDrTLPTOb

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8947298 [details] [diff] [review]
part 4.  Use IgnoreErrors() in dom/

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

::: dom/base/CustomElementRegistry.cpp
@@ +75,3 @@
>    switch (mType) {
>      case nsIDocument::eConnected:
> +      static_cast<LifecycleConnectedCallback *>(mCallback.get())->Call(mThisObject);

I presume this change just now calls a different overload on these callbacks?

::: dom/serviceworkers/ServiceWorkerEvents.cpp
@@ +736,5 @@
>    nsCOMPtr<nsIInputStream> body;
>    ir->GetUnfilteredBody(getter_AddRefs(body));
>    // Errors and redirects may not have a body.
>    if (body) {
> +    ErrorResult error;

Aren't we no longer suppressing any exceptions here now? I'm confused as to how this is correct.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> I presume this change just now calls a different overload on these callbacks?

Yes, it calls one of the autogenerated overloads that the codegen patch changed, which just does the IgnoreErrors() internally.

> Aren't we no longer suppressing any exceptions here now? I'm confused as to how this is correct.

The relevant code is:

     ErrorResult error;
     response->SetBodyUsed(aCx, error);
     if (NS_WARN_IF(error.Failed())) {
       autoCancel.SetCancelErrorResult(aCx, error);

SetCancelErrorResult looks like this, after some initial asserts:

    if (!aRv.MaybeSetPendingException(aCx)) {
      return;
    }

and MaybeSetPendingException moves the exception from aRv to aCx.  So after that line, aRv no longer has an exception on it.

In other words, by the time the "error" in the caller goes out of scope, it's guaranteed to have no exception on it, so there is nothing to suppress.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27018abb80f8
part 1.  Introduce a mozilla::IgnoreErrors which can be used as a temporary to pass to an ErrorResult& arg when the error should be ignored.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4719d1d3ec3
part 2.  Use IgnoreErrors() in dom/bindings.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0dc4d5fc3dd
part 3.  Use IgnoreErrors() outside of dom/.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d5547b0b7f
part 4.  Use IgnoreErrors() in dom/.  r=mystor

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/27018abb80f8
https://hg.mozilla.org/mozilla-central/rev/f4719d1d3ec3
https://hg.mozilla.org/mozilla-central/rev/c0dc4d5fc3dd
https://hg.mozilla.org/mozilla-central/rev/a3d5547b0b7f