Closed
Bug 916985
Opened 11 years ago
Closed 11 years ago
nsDSURIContentListener::DoContent does not handle CreateContentViewer failures correctly
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: deian, Assigned: deian)
Details
Attachments
(1 file, 2 obsolete files)
2.30 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•11 years ago
|
Summary: nsDSURIContentListener::DoContent does not handler CreateContentViewer failures correctly → nsDSURIContentListener::DoContent does not handle CreateContentViewer failures correctly
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #808376 -
Attachment is obsolete: true
Attachment #808377 -
Flags: review?(bzbarsky)
Comment 3•11 years ago
|
||
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-
Assignee | ||
Comment 4•11 years ago
|
||
(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•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #808377 -
Attachment is obsolete: true
Assignee | ||
Comment 7•11 years ago
|
||
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 8•11 years ago
|
||
Comment on attachment 813807 [details] [diff] [review] Address DoContent fail r=m
Attachment #813807 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 9•11 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=bc66ff955d95
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 10•11 years ago
|
||
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
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2e66a625c971
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•