nsDSURIContentListener::DoContent does not handle CreateContentViewer failures correctly

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: deian, Assigned: deian)

Tracking

Trunk
mozilla27
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Summary: nsDSURIContentListener::DoContent does not handler CreateContentViewer failures correctly → nsDSURIContentListener::DoContent does not handle CreateContentViewer failures correctly
Posted patch Proposed fix (obsolete) — Splinter Review
Attachment #808376 - Attachment is obsolete: true
Attachment #808377 - Flags: review?(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...
Attachment #808377 - Flags: review?(bzbarsky) → review-
(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
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.
Attachment #808377 - Attachment is obsolete: true
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".
Attachment #813807 - Flags: review?(bzbarsky)
Comment on attachment 813807 [details] [diff] [review]
Address DoContent fail

r=m
Attachment #813807 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
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! :)
Assignee: nobody → deian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2e66a625c971
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.