1173934 - ServiceWorker intercepted navigations that produce network errors should show an error message

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently if a top level navigation is intercepted and the respondWith() Response is a network error then the tab just does nothing.  It stays on the current page as if the link was never clicked.

We should have some kind of network error or service worker error page instead.

The UX is bad enough here I'm blocking v1, but of course we can debate that.

Comment 1

[(no longer active)]

I think I have seen something like this...  IIRC even if you copy the link, open a new tab, and paste it in the location bar and press Enter, nothing happens.  I agree that we should fix this before shipping.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I'm going to see if I can just set an error code on the intercepted nsIChannel.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Show a message if a docshell fails to load due to SW intercept failure. r=ehsan

So it turns out the FetchEvent.respondWith() was canceling the channel in all cases.  Josh had an RAII style object to do that.

The issue, however, was that the channel was canceled with NS_ERROR_ABORT_BINDING which docshell does not know about.

Therefore, I created a new NS_ERROR_INTERCEPTION_FAILED result and modified nsDocShell to show an error page when this happens.

The error page looks like this:

  https://www.dropbox.com/s/b8yce58v6e9d5z0/Screenshot%202015-07-02%2023.08.12.png?dl=0

I doubt users will really understand this, but I'm not sure what else to put here.  I'm welcome to suggestions for other text.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Do we translate messages like this?  Flag l10n to check.

Comment 5

[(no longer active)]

Comment on attachment 8629206 [details] [diff] [review]
Show a message if a docshell fails to load due to SW intercept failure. r=ehsan

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

It's great to be able to internally differentiate this kind of cancellation through NS_ERROR_INTERCEPTION_FAILED, and I think in nsDocShell::DisplayLoadError we should log a good error message to the console.  However, I don't think we should show anything about service workers to the user, since this will be entirely meaningless for them.  What do you think about reusing an existing error, such as corruptedContentError?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Well, in addition to that issue, these patches also don't work in e10s.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Show a message if a docshell fails to load due to SW intercept failure. r=ehsan r=jdm

Ok.  This now uses the corrupted content error page.  It also logs to the console for various types of errors we know about.

Of course, this logging is only done for navigations through docshell for now.  In theory we could add this everywhere network requests are performed.  Unfortunately we can't do it directly in ServiceWorkerEvents because we don't have the nsIDocument in order to find the console.  For now I put it in a separate nsContentUtils method so we can add it in other places easily.

Josh, can you review the nsIInterceptedChannel and InterceptedChannel bits?  Ehsan, can you review the rest?

Sorry I didn't have the foresight to split the patch up.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

I'll add a test as well.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Actually, I'd like to just consolidate the test for this in the test I'm writing for bug 1173912.

Comment 10

[(no longer active)]

Comment on attachment 8631112 [details] [diff] [review]
Show a message if a docshell fails to load due to SW intercept failure. r=ehsan r=jdm

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

::: dom/workers/ServiceWorkerEvents.cpp
@@ +83,3 @@
>  public:
> +  explicit CancelChannelRunnable(nsMainThreadPtrHandle<nsIInterceptedChannel>& aChannel,
> +                                 nsresult aStatus)

Nit: please drop explicit.

Comment 11

[(no longer active)]

Comment on attachment 8631112 [details] [diff] [review]
Show a message if a docshell fails to load due to SW intercept failure. r=ehsan r=jdm

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

::: dom/base/nsContentUtils.cpp
@@ +3428,5 @@
> +  } else if (aError == NS_ERROR_INTERCEPTED_ERROR_RESPONSE) {
> +    messageName = "InterceptedErrorResponse";
> +  } else if (aError == NS_ERROR_INTERCEPTED_USED_RESPONSE) {
> +    messageName = "InterceptedUsedResponse";
> +  }

One small additional nit: Do you mind adding a MOZ_ASSERT(messageName) here so that we catch the cases where we forget to update this function in the future?

Thanks!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/adf95d1cd0aa

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out (next to bug 1173934) for WPT4 and S4 oranges in https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e0899ac7c7 :

https://treeherder.mozilla.org/logviewer.html#?job_id=11578077&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=11577037&repo=mozilla-inbound

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Update wpt tests to expect failed window/frame on bad intercept. r=jgraham

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Also updated existing patch to pass nsresult to nsIInterceptedChannel.cancel() to fix the netwerk xpcshell test:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a194fb84d49b

Comment 16

[James Graham [:jgraham]]

Comment on attachment 8632861 [details] [diff] [review]
Update wpt tests to expect failed window/frame on bad intercept.  r=jgraham

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

::: testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-frame-resource.https.html
@@ +55,5 @@
>              });
>          })
>        .then(function(frame) {
>            assert_equals(
> +            frame.contentDocument,

If I have understood what's going on here, it's very unclear to me that either this or the previous behaviour are right. It seems like a CORS error should be something like a network error, which should end up at [1], which suggests that there should *be* a Document in the iframe, but that the contents of the DOM, if any are unspecified.

Possibly the only right thing to do here is assert that the document we didn't expect to load did not in fact load, rather than trying to make positive assertions about what did load?

[1] https://html.spec.whatwg.org/#read-ua-inline

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

James, what about this:

  assert_true(
    !frame.contentDocument || frame.contentDocument.textContent === '',
    /* ... */
  );

How else do you propose to assert the document didn't load?

Comment 18

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

https://html.spec.whatwg.org/#read-ua-inline

The spec doesn't define in which domain the synthetic document should be. Is it same-origin to ... well what? or is it always cross-origin to any web pages?

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

To be honest, if this becomes a big issue over how gecko handles about:neterror and frame.contentDocument, I'm inclined to just mark these tests expected fail and hash that out in a separate bug.

Comment 20

[James Graham [:jgraham]]

Yeah, given the spec is very unclear about what the origin of the error page should be here, I think that stating that the contentDocument is either null or is a document that doesn't contain the content of the document that would hae loaded if the load had succeeded is perfectly reasonable. I can't see any reason to specifically expect the empty string though.

Comment 21

[James Graham [:jgraham]]

Thinking about this some more, the *best* way to ensure that a cross-origin load doesn't happen is with a != reftest (the ref would be the same resource loaded same-origin), because the contentDocument === null check could be true even if the resource actually does load.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Update wpt tests to expect failed window/frame on bad intercept. r=jgraham

I'm using the check for empty string because I can't tell what the real resource is supposed to look like.  We're missing fetch-access-control.py.  I opened bug 1183677 for that.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Disable broken fetch-frame-resources.https.html wpt tests. r=jgraham

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f62985020fd9
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3874479724b

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f62985020fd9
https://hg.mozilla.org/mozilla-central/rev/e3874479724b