1416842 - Expose the underlying NS_ERROR in fetch rejection errors

Status: RESOLVED FIXED

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

Description

[Edouard Oger [:eoger]]

In bug 1409534, we are trying to replace our Sync network implementation with fetch.
However, we are losing accuracy when a fetch promise rejects with an error, since it only contains a generic error message.
Network error codes are very useful to us to diagnose what is happening when our users send us a log dump (eg. NS_ERROR_UNKNOWN_HOST, PROXY_CONNECTION_REFUSED)

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

In service worker meetings we have discussed storing more detailed error information in the InternalResponse.  This would not be exposed to content script, but the browser could access it.  For example, it would let the browser should the correct about:neterror page if a SW script does `evt.respondWith(fetch(evt.request))` and some kind of network/tls error happens.

This is something we would take, but I'm not sure we have anyone to work on it immediately.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Thanks for the update - Ed and I are having a bit of a look at this.

(In reply to Ben Kelly [:bkelly] from comment #1)
> In service worker meetings we have discussed storing more detailed error
> information in the InternalResponse.  This would not be exposed to content
> script, but the browser could access it.

It doesn't look too difficult to carry the underlying nsresult around in the InternalResponse, however, it's not clear to me how this would be communicated to browser code given most of the failures we care about end up as TypeErrors - the caller never gets a response object.

Specifically, if you attempt to fetch from a name that can't be resolved, https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/dom/fetch/Fetch.cpp#504 is where a TypeError is thrown - the InternalResponse with the error code is also there, but once the error is thrown we've lost that information.

There's a good chance I'm missing some context here, but is there any advice you can offer?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Well, we would need a chrome-only API for this.  Maybe some method on request which sets a flag that tells us to reject with the inner error and not a TypeError.

I think there is a way to make a chrome-only method.  Or maybe we can add it to RequestInit and ignore it for any non-system principal.  Not sure if there are webidl concerns with doing a chrome only dictionary field lile this.

Boris, what do you think?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I think there is a way to make a chrome-only method.

Yep, [ChromeOnly] annotation in webidl.

> Not sure if there are webidl concerns with doing a chrome only dictionary field lile this.

You can annotate a dictionary member as [ChromeOnly] and then bindings code will pretend that the property wasn't set on the object (and in fact not even check whether the property exists) if the caller is not system-principal.  So support for it it will be completely invisible to non-system code.  This is totally a supported thing to do.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Thanks!

Ok, then maybe a [ChromeOnly] dictionary field on RequestInit would be best:

https://searchfox.org/mozilla-central/source/dom/webidl/Request.webidl#42

So you can do something like:

  let req = new Request(url, { internalErrors: true })
  fetch(req).catch(e => {
    // e may be an internal error instead of a TypeError
  });

I don't really have a strong opinion on the name.  I guess we should pick something that would be unlikely to ever be used for real in the spec.

Comment 6

[Edouard Oger [:eoger]]

> I guess we should pick something that would be unlikely to ever be used for real in the spec.
mozInternalErrors?

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

I guess we could just do mozErrors then?  Shorter is good too.

Comment 8

[Edouard Oger [:eoger]]

Attachment: Bug 1416842 - Allow fetch to reject with nsresult in chrome code.

Review commit: https://reviewboard.mozilla.org/r/199898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199898/

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8928656 [details]
Bug 1416842 - Allow fetch to reject with nsresult in chrome code.

https://reviewboard.mozilla.org/r/199898/#review206024

Overall looks good, but it needs a test that verifies it works in chrome script and is unavailable in content script.  Also, I think we need to enforce that NetworkError Response objects always have an NS_FAILED() code.

::: dom/cache/TypeUtils.cpp:261
(Diff revision 1)
>  already_AddRefed<Response>
>  TypeUtils::ToResponse(const CacheResponse& aIn)
>  {
>    if (aIn.type() == ResponseType::Error) {
> -    RefPtr<InternalResponse> error = InternalResponse::NetworkError();
> +    // We don't bother tracking the internal error code for cached responses...
> +    RefPtr<InternalResponse> error = InternalResponse::NetworkError(NS_OK);

I think the internal error code should at least cause NS_FAILED() to be true.  So maybe use NS_ERROR_FAILURE here instead of NS_OK.

::: dom/fetch/Fetch.cpp:263
(Diff revision 1)
>  {
>    RefPtr<Promise> mPromise;
>    RefPtr<Response> mResponse;
>    RefPtr<FetchObserver> mFetchObserver;
>    RefPtr<AbortSignal> mSignal;
> +  bool mMozErrors;

const

::: dom/fetch/Fetch.cpp:508
(Diff revision 1)
>      }
>  
> +    if (mMozErrors) {
> +      mPromise->MaybeReject(aResponse->GetErrorCode());
> +      return;
> +    }

nit: extra line after this block

::: dom/fetch/FetchDriver.cpp:528
(Diff revision 1)
>      rv = httpChannel->GetResponseStatus(&responseStatus);
>      MOZ_ASSERT(NS_SUCCEEDED(rv));
>  
>      if (mozilla::net::nsHttpChannel::IsRedirectStatus(responseStatus)) {
>        if (mRequest->GetRedirectMode() == RequestRedirect::Error) {
> -        FailWithNetworkError();
> +        FailWithNetworkError(rv);

The rv variable does not contain an error here.  You probably want to pass something like NS_BINDING_ABORTED.  This is what the necko stack uses for this failure:

https://searchfox.org/mozilla-central/source/netwerk/base/nsAsyncRedirectVerifyHelper.cpp#85

::: dom/fetch/FetchDriver.cpp:634
(Diff revision 1)
>    // sure the Response is fully initialized before calling this.
>    mResponse = BeginAndGetFilteredResponse(response, foundOpaqueRedirect);
>    if (NS_WARN_IF(!mResponse)) {
>      // Fail to generate a paddingInfo for opaque response.
>      MOZ_DIAGNOSTIC_ASSERT(mResponse->Type() == ResponseType::Opaque);
> -    FailWithNetworkError();
> +    FailWithNetworkError(NS_ERROR_UNEXPECTED /* XXX - correct nsresult? */);

This is fine.  Please remove the XXX comment.

::: dom/fetch/InternalResponse.h:56
(Diff revision 1)
>    };
>  
>    already_AddRefed<InternalResponse> Clone(CloneType eCloneType);
>  
>    static already_AddRefed<InternalResponse>
> -  NetworkError()
> +  NetworkError(nsresult rv)

Please MOZ_DIAGNOSTIC_ASSERT(NS_FAILED(aRv)) here.

Also, please use `aRv` style for arguments in this code.  Thanks.

::: dom/fetch/InternalResponse.h:292
(Diff revision 1)
> +  {
> +    return mErrorCode;
> +  }
> +
> +  void
> +  SetErrorCode(nsresult r)

Is this needed?  Seems like its not used.

::: dom/fetch/Response.cpp:90
(Diff revision 1)
>  
>  /* static */ already_AddRefed<Response>
>  Response::Error(const GlobalObject& aGlobal)
>  {
>    nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aGlobal.GetAsSupports());
> -  RefPtr<InternalResponse> error = InternalResponse::NetworkError();
> +  RefPtr<InternalResponse> error = InternalResponse::NetworkError(NS_OK);

Please use a non-success code like NS_ERROR_FAILURE or NS_ERROR_DOM_ABORT_ERR.

::: dom/flyweb/FlyWebServerEvents.cpp:100
(Diff revision 1)
>        intResponse = nullptr;
>      }
>    }
>  
>    if (!intResponse) {
> -    intResponse = InternalResponse::NetworkError();
> +    intResponse = InternalResponse::NetworkError(NS_OK);

Please use NS_ERROR_FAILURE for these.  Probably doesn't matter much here because I think fly web is abandonded at this point.

::: dom/flyweb/FlyWebServerEvents.cpp:115
(Diff revision 1)
>  }
>  
>  void
>  FlyWebFetchEvent::RejectedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue)
>  {
> -  RefPtr<InternalResponse> err = InternalResponse::NetworkError();
> +  RefPtr<InternalResponse> err = InternalResponse::NetworkError(NS_OK);

NS_ERROR_FAILURE

Comment 10

[Edouard Oger [:eoger]]

Comment on attachment 8928656 [details]
Bug 1416842 - Allow fetch to reject with nsresult in chrome code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199898/diff/1-2/

Comment 11

[Edouard Oger [:eoger]]

Thank you Ben, I have amended my patch with your comments.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8928656 [details]
Bug 1416842 - Allow fetch to reject with nsresult in chrome code.

https://reviewboard.mozilla.org/r/199898/#review206094

Looks great.  Thanks!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55ebc5117e6a
Allow fetch to reject with nsresult in chrome code. r=bkelly

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/55ebc5117e6a