399876 - Improve ability to detect whether an error can be shown as an error page

Status: NEW

(Core :: DOM: Navigation, defect)

Description

[Kai Engert (:KaiE:)]

With bug 107491 I introduced the possibility to display errors during SSL connections as an error page. With bug 327181 we converted more scenarios to bring up such an error page, so lately, users see that page more frequently.

Whenever the current SSL connection does not happen inside a document pane (having room for an error page) then errors will be reported as a dialog.

When I first tried to implement this, I considered to make use of the nsIDocShell::useErrorPages attribute.

As I explain in bug 370875 comment 6, this attribute is not appropriate for platform wide detection of the error page ability, because it's giving false/inappropriate information in mail context.

So, I had used a different variable to make the "error pages vs error dialog" decision:

+  nsCOMPtr<nsIDocShell> docshell(do_GetInterface(mCallbacks));
+  if (docshell)
+  {
+    nsCOMPtr<nsISecureBrowserUI> secureUI;
+    docshell->GetSecurityUI(getter_AddRefs(secureUI));
+    if (secureUI)
+    {
+      mExternalErrorReporting = PR_TRUE;
+    }



What is this bug about?

There are scenarios where we could report an error page, but don't do it.
This bug proposes we enhance the docshell-to-networking interaction, so we can judge better when an error page is possible.


Example 1: 
  Frames. See bug 399760 comment 0, "Problem 3".

Example 2:
  AJAX - or - first ssl connection in a web page
              triggered using JavaScript

To try the second example, go to
  http://sonyrewards.com/en/home/base/
click the "please login" at the top of the page,
enter anything for user/password, and you'll get an error dialog (not a page).

Comment 1

[Boris Zbarsky [:bzbarsky]]

For example 1, you should be getting the root same-type docshell, right?

For example 2... I don't see why that should happen.  I'll take a look.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Ah, the second case is just a subframe.  So it's really the same thing as the first case.

Not that I'm against a better API here, mind you.  Just saying that the existing code can handle the two listed cases with a minor tweak.

Comment 3

[Kai Engert (:KaiE:)]

Boris, if you could explain how the tweak should be done, that would be much appreciated! Thanks.

Comment 4

[Boris Zbarsky [:bzbarsky]]

  nsCOMPtr<nsIDocShellTreeItem> item(do_GetInterface(mCallbacks));
  if (item) {
    nsCOMPtr<nsIDocShellTreeItem> rootItem;
    item->GetSameTypeRootTreeItem(getter_AddRefs(rootItem));
    nsCOMPtr<nsIDocShell> docshell(do_QueryInterface(rootItem));
    NS_ASSERTION(docshell, "Something is _very_ wrong");
    
  }

Then get the security UI off of |docshell|.

Comment 5

[Kai Engert (:KaiE:)]

Attachment: patch for testing bz's proposal

Boris, thanks a lot for your quick proposal.
This is a patch that implements it.
It helps, but I think it's not yet the solution to the real problem.

When using this patch, I no longer get an error dialog.
This means, we are able to find a docshell that carries a security info.
So, PSM assumes that docshell will display the error, and PSM will remain silent.

But that doesn't happen.
With this patch, I no longer get any error message.
Not as a dialog, not as a page.

I presume, when loading a sub element of a page, the SSL/network error from that element is not handled by nsDocShell::DisplayLoadError(). The error is probably handled somewhere else, where it's being ignored.


But even if the docshell displayed an error page, would that really help us?


When we can not load a sub element of a page, should we really replace all page contents with the error page?


I think for now we should continue to display the error dialog.

I think this bug asks for a solution to our "mixed content" and "block insecure content by default" problems.

As soon as we have an indicator in the UI (like a notification bar), that would notify the user
  "there was some insecure content that was blocked - 
   click here to learn more"
then we could stop displaying the error dialog.

(And something similar to Boris patch would help us to know: this docshell is able to display such a notification bar)

Comment 6

[Boris Zbarsky [:bzbarsky]]

> not handled by nsDocShell::DisplayLoadError().

Uh... yes.  That will only handle loads that are happening in that docshell itself.  It will not handle errors from images, <object>s, <script>, <style>, XMLHttpRequest, etc, etc.

Which of those do you need to show SSL errors for, exactly?

> When we can not load a sub element of a page, should we really replace all
> page contents with the error page?

No, but I also don't think we should put up an error dialog in that case...  We don't give error feedback if a script on the page times out; why is SSL any different?

Comment 7

[Kai Engert (:KaiE:)]

(In reply to comment #6)
> > not handled by nsDocShell::DisplayLoadError().
> 
> Uh... yes.  That will only handle loads that are happening in that docshell
> itself.  It will not handle errors from images, <object>s, <script>, <style>,
> XMLHttpRequest, etc, etc.
> 
> Which of those do you need to show SSL errors for, exactly?

I'm not sure in which scenarios SSL errors should be shown and in which scenarios the content shall be silently blocked.

If we didn't show an error message, diagnosing broken sites like the login on sonyrewards would be impossible.


The tough question is: Should we try to identify scenarios where it's reasonable to show SSL as an error page.

Maybe this bug should be resolved invalid and we should simply continue to display an error dialog for sub elements.


> > When we can not load a sub element of a page, should we really replace all
> > page contents with the error page?
> 
> No, but I also don't think we should put up an error dialog in that case...  We
> don't give error feedback if a script on the page times out; why is SSL any
> different?

The difference is with us deliberately blocking the content, because of an untrusted cert, and because of that our rendering behavior might be different from other browsers, so we should at least notify the user.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I'm not sure in which scenarios SSL errors should be shown and in which
> scenarios the content shall be silently blocked.

Right.  That's the first thing to decide on.

> If we didn't show an error message, diagnosing broken sites like the login on
> sonyrewards would be impossible.

Does the error message need to be in the user-visible UI?  Or is the error console ok?  I would think that except for an actual page being loaded in a docshell (which gets an error page), everything else should go into the error console.

> Maybe this bug should be resolved invalid and we should simply continue to
> display an error dialog for sub elements.

I personally don't think that's the way to go.

Note that we also don't give error feedback, except in the error console when CheckLoadURI blocks a load.  And I think that's fine.

Comment 9

[Johnathan Nightingale [:johnath]]

So whether we silently block things like images/scripts/etc. is one question to answer, but it feels like the bigger problem is that we don't currently show error pages in frames.  E.g.:

data:text/html,<html><frameset%20cols="50%,50%"><frame%20src="http://google.com"/><frame%20src="https://mur.at"/></frameset></html>

Is it possible for us to at least do the right thing when we *do* have the ability to show UI first, and then tackle the question of what to do when we can't?  Because that one seems sort of glaring, to me, and is contrary to what we do with regular error pages, E.g.:

data:text/html,<html><frameset%20cols="50%,50%"><frame%20src="http://google.com"/><frame%20src="http://foob.foob"/></frameset></html>

Comment 10

[Boris Zbarsky [:bzbarsky]]

That example should get fixed by the patch in attachment 284998 [details] [diff] [review], no?

Comment 11

[Johnathan Nightingale [:johnath]]

(In reply to comment #10)
> That example should get fixed by the patch in attachment 284998 [details] [diff] [review], no?

It does!  I might humbly suggest that whether we decide to tackle other cases or not, this patch is a big improvement on its own.  I can file a new bug for this specific case, or we can morph this bug, but Kai/bz: does it make sense to just get this one reviewed/landed?

Comment 12

[Kai Engert (:KaiE:)]

Johnathan, I'm worried the current patch makes some scenarios worse.

Did you try example 2 from comment 0 ?

Current state:
  user gets error dialog, indicating the problem

With the patch:
  silence

Comment 13

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I think that the patch is in fact the right behavior, giving us consistency between SSL errors and other sorts of network errors...

Comment 14

[Kai Engert (:KaiE:)]

Would it be easily possible to produce a notice on the error console each time the docshell throws away such a connection failure?

Comment 15

[Johnathan Nightingale [:johnath]]

Ah, no.  I forgot to retest that scenario with the patch.  I see why you were reluctant to land it.

On the other hand, the patch doesn't actually make the experience much worse for end users - they weren't able to proceed at sonyrewards either way - it just changes from a dialog they can do nothing about, to a silent failure.  It does make it harder to diagnose the problem in that situation, I agree, and I think we should be more helpful there. By not landing it though, we continue to not let them log in at sonyrewards, but also prevent them receiving information and override options in the comment 9 case that, with the patch, they would have.

I would be in favour of landing a version of this fix, with errors logged to the console as bz suggests to assist diagnosis by developers, and taking a follow-up to notify-bar in cases where we block content that can't present an error page.  I wonder if how you feel about that tradeoff, Kai?

Comment 16

[Kai Engert (:KaiE:)]

sounds good to me. it would be great to land the error-console thing at the same time as this bug.

Comment 17

[Johnathan Nightingale [:johnath]]

(In reply to comment #16)
> sounds good to me. it would be great to land the error-console thing at the
> same time as this bug.

Well then let's wait for bz to tell us that there's a oneliner way to log to the console from your patch.  :)

Comment 18

[Kai Engert (:KaiE:)]

My patch to security merely says "ok, this is happening somewhere inside a docshell, that docshell will deal with the error and report it".

The patch to enable error reporting would have to happen where the errors are consumed and currently silently discarded.

Comment 19

[Kai Engert (:KaiE:)]

I read bz's comments again and looked at the page in more detail and traced a bit.

I now understand this page uses an invisible iframe.
It appears, we DO load an error page, but it will be shown in the hidden iframe only.

If that site's navigation depends on events in a hidden iframe, it's probably the site's responsibility to catch failures and report them in a way visible to the user.


Given that we DO show an error page (hidden iframe), my proposal wouldn't help.
My proposal was "let's show something on the console if the errors are silently eaten".

But we don't eat the error silently in the Sony example.


So, if we wanted to provide a workaround for the bad site, we would have to report the error twice. Once on the page, once in the error console.

I'm not sure if that's what we want.

Comment 20

[Kai Engert (:KaiE:)]

I think I'm OK to land the attached patch.


But let's discuss what should happen in the following scenario:
  http://kuix.de/misc/test399876/

That page attempts to load from a bad cert location using 4 different mechanisms:
- css
- js
- image
- xmlhttprequest

In which scenarios should an error be shown on the console?

Comment 21

[Boris Zbarsky [:bzbarsky]]

The code that gets called to handle errors is in the clause at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/base/nsDocShell.cpp&rev=1.876#4922

You could add another clause: when there is a STATE_STOP for this progress that is not DOCUMENT or NETWORK and aStatus is an error (or has a certain set of values), log something.

Comment 22

[Kai Engert (:KaiE:)]

Attachment: test 2

Boris, is this what you had in mind?

It seems this condition is never true in my tests.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Yes, I meant something like that.  I'd expect it to be true sometimes... do you hit nsDocLoader::FireOnStateChange with STATE_STOP but neither of the other flags?

Comment 25

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.