1025330 - expose the failed channel from the docshell so error pages can have access to it

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Unless I'm misunderstanding how the docshell works, when an error page (in particular an SSL error page) has been loaded, the original channel that failed to load is not available. However, it seems the docshell does know what that failed channel is. It would be very useful to have access to it. (This applies to the SSL error reporting project as well as properly enabling certificate overrides so we don't have to use the inherently broken recent bad certs service.)

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch

jst - looks like you're the docshell peer with the fewest review requests at the moment - would you mind having a look?

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

To see how this is going to be used, see the patch in bug 1025332.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 8440172 [details] [diff] [review]
patch

This looks correct to me, but requesting review from bz as well to ensure this doesn't leave mFailedChannel set to some old failed channel forever or somesuch. AFAICT Stop() is always called between loads, and that sets mFailedChannel to null if the load type is for an error page, which is the only case where mFailedChannel is set for. But again, I'd like bz to look this over as well.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8440172 [details] [diff] [review]
patch

Hmm.  Relying on the Stop(args) behavior is really fragile.  As one simple example, if a javascript: URI is loaded, it won't call Stop(args) in the docshell (relying on the JS protocol handler to do it), but _will_ set mLoadType, so by the time Stop(args) is called the load type is not LOAD_ERROR_PAGE.  I'm pretty sure there are more convoluted cases like that if we look hard enough.

I would be happier if we just put the failed channel on the error page document instead.  In the code in CreateContentViewer(), we should be able to get a document from the new content viewer, right?  That would ensure that the lifetime of that channel won't exceed that of the document in question.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v2

Thanks for the reviews, jst and bz.
bz - I'm not sure I understood what you were saying to do. Is this on the right track?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8440966 [details] [diff] [review]
patch v2

>+#include "nsIChannel.h"                  // for member

Please just put that in nsDocument.h (where it's already included) and make this a virtual getter/setter like GetChannel() is.  You'll need to change the nsIDocument IID.

>+    nsCOMPtr<nsIDocument> doc(GetDocument());

No need for the strong ref.

>+    NS_IF_ADDREF(*aFailedChannel = doc->GetFailedChannel());

On the other hand, doc can be null, so you need to check that.

>+        viewer->GetDOMDocument(getter_AddRefs(domDoc));

If you use viewer->GetDocument() here, you can get the nsIDocument* directly.

>+  readonly attribute nsIChannel failedChannel;

Needs documentation.  And an IID change for nsIDocShell.

r=me with those fixed.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v2.1

Thanks for the reviews! Carrying over r+.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d906d4e1acbe

Comment 8

[Ed Morley [:emorley]]

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