Last Comment Bug 768842 - [BrowserAPI] Inform embedder when we show a Gecko error page
: [BrowserAPI] Inform embedder when we show a Gecko error page
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 769200
Blocks: browser-api 766437
  Show dependency treegraph
 
Reported: 2012-06-27 04:54 PDT by Justin Lebar (not reading bugmail)
Modified: 2013-04-04 13:53 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (9.53 KB, patch)
2012-06-28 03:21 PDT, Justin Lebar (not reading bugmail)
mounir: review+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2012-06-27 04:54:35 PDT
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) 2012-06-27 07:52:26 PDT
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) 2012-06-27 08:18:01 PDT
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 [:bz] (Out June 25-July 6) 2012-06-27 08:29:31 PDT
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) 2012-06-27 09:35:36 PDT
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) 2012-06-28 03:21:16 PDT
Created attachment 637442 [details] [diff] [review]
Patch v1
Comment 6 Mounir Lamouri (:mounir) 2012-06-28 09:27:12 PDT
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) 2012-06-28 09:41:42 PDT
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) 2012-07-02 10:26:58 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e248fe5d20f4
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-07-02 15:30:39 PDT
https://hg.mozilla.org/mozilla-central/rev/e248fe5d20f4
Comment 10 mgalli 2012-07-04 08:16:18 PDT
Do we have a bug like this for certs changes?
Comment 11 Justin Lebar (not reading bugmail) 2012-07-04 09:10:25 PDT
(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] 2012-08-01 05:23:49 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.