916985 - nsDSURIContentListener::DoContent does not handle CreateContentViewer failures correctly

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Deian Stefan]

Followup on https://bugzilla.mozilla.org/show_bug.cgi?id=886164#c18, nsDSURIContentListener::DoContent needs to either 
(see https://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsIURIContentListener.idl#53):
- set *aAbortProcess to true
- return a failure nsresult
- return a non-null *aContentHandler

1.) Howerver, if creating the content viewer fails, DoContent, does none of the above and just returns NS_OK:
https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDSURIContentListener.cpp#129

It seems like the right thing to do here is set *aContentHandler to nullptr since we don't know how to handle the content.

2.) The request is cancelled if the error is due to accessing a remote XUL document (that is not in the white-list):
 
https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDSURIContentListener.cpp#124

but it seems like we should also be setting *aAbortProcess here to true, since we're handling the load.

Comment 1

[Deian Stefan]

Attachment: Proposed fix

Comment 2

[Deian Stefan]

Attachment: Fix careless mistake in previous patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 808377 [details] [diff] [review]
Fix careless mistake in previous patch

>     if (NS_FAILED(rv)) {
>        // it's okay if we don't know how to handle the content   
>+        *aContentHandler = nullptr;
>         return NS_OK;

That doesn't look right...

Comment 4

[Deian Stefan]

(In reply to Boris Zbarsky [:bz] from comment #3)
> Comment on attachment 808377 [details] [diff] [review]
> Fix careless mistake in previous patch
> 
> >     if (NS_FAILED(rv)) {
> >        // it's okay if we don't know how to handle the content   
> >+        *aContentHandler = nullptr;
> >         return NS_OK;
> 
> That doesn't look right...

So we're not treating "don't know how to handle content" as "can't handle content type"?

https://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsIURIContentListener.idl#41

Comment 5

[Boris Zbarsky [:bzbarsky]]

The documentation there doesn't seem to match reality.  Reality is what happens in nsDocumentOpenInfo::TryContentListener and is what I said in bug 886164 comment 18:

  either set *aAbortProcess to true or return a failure nsresult or return a non-null
  *aContentHandler.

If you return success and don't set *aAbortProcess to true, you will hit this line in TryContentListener:

700   NS_ASSERTION(abort || m_targetStreamListener, "DoContent returned no listener?");

which will of course fail, and then and then the data will be silently dropped on the floor.

The way to indicate "can't handle" is to return a failure nsresult.  And fix the documentation, I guess.

Comment 6

[Deian Stefan]

Attachment: Address DoContent fail

Comment 7

[Deian Stefan]

Comment on attachment 813807 [details] [diff] [review]
Address DoContent fail

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

Thanks for looking at the patch. I fixed the comment and am now just failing (+setting the aContentHolder to nullptr, despite the callee doing the same) in the case of "can't handle content".

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 813807 [details] [diff] [review]
Address DoContent fail

r=m

Comment 9

[Deian Stefan]

Try: https://tbpl.mozilla.org/?tree=Try&rev=bc66ff955d95

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e66a625c971

Friendly reminder that your commit message should be giving a brief description of what your patch is actually doing rather than just restating the bug title. Thanks! :)

Comment 11

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/2e66a625c971