768842 - [BrowserAPI] Inform embedder when we show a Gecko error page

Status: RESOLVED FIXED

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

Description

[Justin Lebar (not reading bugmail)]

The embedder should be informed when we show an error page (http error, certificate error, etc.).

This is particularly important for apps.  If the system app tries to load an app and gets a 404, we should do something more meaningful than display a 404 page to the user.

Things like cert errors are kind of complex; I don't think we need to handle the full flow for allowing cert exceptions.  It should be good enough for now if we just say "cert error, can't load page".

Comment 1

[Justin Lebar (not reading bugmail)]

Ha, we actually *should* have been sending locationchange for "about:certerror" and friends, except there's a bug (?) in docshell where we don't send a locationchange for an iframe's initial load, even if the initial load is an error page.

Boris, do you think it would break anything if we fired a locationchange for error pages, in an iframe's initial load?  Otherwise we can restrict this change to browser frames only.

Comment 2

[Justin Lebar (not reading bugmail)]

Oh, I see; I should be looking at the statechange event for the error code.  It's still weird that we don't send a locationchange for the first load, if it's an error (per comment 1), but that's not important.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The "don't send locationchange for initial load" thing is restricted to subframes because it confused the UI when we did that.

But browser frames maybe shouldn't test true for the "is a subframe" check!

Comment 4

[Justin Lebar (not reading bugmail)]

Chris, I think we drastically underestimated the complexity of informing the embedder of exactly what error occurred.  See [1] for a list of all the errors which can occur.

Informing the embedder /that/ an error occurred is simple, and I'll spin that up.

[1] http://hg.mozilla.org/mozilla-central/file/5b3bf49ce3cf/docshell/base/nsDocShell.cpp#l3930

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 6

[Mounir Lamouri (:mounir)]

Comment on attachment 637442 [details] [diff] [review]
Patch v1

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

::: dom/browser-element/BrowserElementChild.js
@@ +415,5 @@
> +        if (status == Cr.NS_OK) {
> +          return;
> +        }
> +
> +        // XXX See nsDocShell::DisplayLoadError for a list of all the errors we

XXX comments shouldn't be added. This should be a NOTE or a TODO with a follow-up bug filed and the number in the comment.

@@ +417,5 @@
> +        }
> +
> +        // XXX See nsDocShell::DisplayLoadError for a list of all the errors we
> +        // should handle here.
> +        sendAsyncMsg('error', {type: 'other'});

Are you going to give more details about the error in a follow-up?

::: dom/browser-element/mochitest/browserElement_Error.js
@@ +35,5 @@
> +    });
> +
> +  });
> +
> +  iframe.src = "http://does_not_exist_so_is_a_404_error1234598239238497239487.com";

Why not:
http://example..org/does_not_exist_so_is_a_404_error.html

Otherwise, I believe this is really going to try to access the domain while example.org will be redirected locally.

Comment 7

[Justin Lebar (not reading bugmail)]

I'm happy to s/XXX/TODO, but I don't like filing follow-ups for issues we haven't encountered yet.  The follow-up would be vague "support more error codes", but then I'd have to file /another/ bug for "support /this specific/ error code" when we actually needed it.  It's really not a net gain.

> Are you going to give more details about the error in a follow-up?

As needed, yes.

> Why not:
> http://example..org/does_not_exist_so_is_a_404_error.html

That doesn't work because mochitest returns a 404 page, and we only do the gecko error page when the server doesn't send an error page itself.

Since we already have one test for this (invalid cert) and there are 25 different ways to generate a Gecko error page, I'm happy simply deleting this test wholesale.

Comment 8

[Justin Lebar (not reading bugmail)]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e248fe5d20f4

Comment 10

[mgalli]

Do we have a bug like this for certs changes?

Comment 11

[Justin Lebar (not reading bugmail)]

(In reply to mgalli from comment #10)
> Do we have a bug like this for certs changes?

I don't know what you mean by "certs changes", but you will get this error event if you visit a page with an invalid SSL certificate, if that answers your question.

Comment 12

[Ben Francis [:benfrancis]]

Justin, can you document this API somewhere so we know what information it gives us exactly? This would really help with a discussion we're having about what the UI should do when an app encounters an error.