The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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".
(Reporter)

Updated

5 years ago
Blocks: 693515
(Reporter)

Comment 1

5 years ago
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.
(Reporter)

Comment 2

5 years ago
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.
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!
(Reporter)

Comment 4

5 years ago
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
(Reporter)

Updated

5 years ago
Depends on: 769200
(Reporter)

Comment 5

5 years ago
Created attachment 637442 [details] [diff] [review]
Patch v1
Attachment #637442 - Flags: review?(mounir)
(Reporter)

Updated

5 years ago
Blocks: 766437
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.
Attachment #637442 - Flags: review?(mounir) → review+
(Reporter)

Comment 7

5 years ago
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.
(Reporter)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e248fe5d20f4
Assignee: nobody → justin.lebar+bug
(Reporter)

Updated

5 years ago
Assignee: justin.lebar+bug → nobody
Component: General → DOM: Mozilla Extensions
OS: Mac OS X → All
Product: Boot2Gecko → Core
QA Contact: general → general
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/e248fe5d20f4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16

Comment 10

5 years ago
Do we have a bug like this for certs changes?
(Reporter)

Comment 11

5 years ago
(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.
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.
(Assignee)

Updated

4 years ago
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.