171025 - [FIX]Handle content conversion failure in nsURILoader.cpp

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect, P1)

Description

[David Epstein]

If the content-type is set in nsIURIContentListener:IsPreferred() to something
other than the original content type, and the conversion isn't carried out,
content listener notifications are sent, but not from the stream listener. This
is as expected because the stream conversion didn't take place. However, the
failure is not returned to the caller. Or another option would be to keep
looking for someone to handle the original content rather than just abort the load.

Example:
TestEmbed, Mozilla 1.2b Gecko 20020925. Build instructions located at:
http://lxr.mozilla.org/seamonkey/source/embedding/qa/testembed/README.TXT
1. Implement nsIURIContentListener methods.
2. Set nsIURIContentListener::IsPreferred() preferred content type to image/gif.
(In TestEmbed, go to:
http://lxr.mozilla.org/seamonkey/source/embedding/qa/testembed/BrowserImpl.cpp#577)
3. Pass a url pointing to text/html content type into nsIURILoader->OpenURI().
(In TestEmbed, Tests > urIContentListener > OpenURI menu. Enter url into dlog).
4. Track nsIURIContentListener callbacks. They occur as expected. (TestEmbed
logfile located in C://Temp/TestOutput.txt).
Result: Aborted load, but no handling of failure. See above url for
nsURILoader.cpp where rv = DispatchContent(request, aCtxt); 
No NS_SUCCEEDED or NS_FAILED is done on rv.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Taking.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Didn't make 1.4.  I'll try to get this done for 1.5a....

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

The basic idea here is that we move nsURILoader::DispatchContent code into
nsDocumentOpenInfo::DispatchContent and nsDocumentOpenInfo::TryContentListener.
 This allows us to catch conversion failures when impossible conversions are
requested and to continue trying other content listeners in hopes of finding
one that works.

Most of this is cut-and-paste....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132650 [details] [diff] [review]
Proposed patch

er, this has a slight issue

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Same, but with the retargeted load flag done right

This just makes sure that we set the retargeted load flag if the nsURIListener
we end up with does not correspond to our original window context.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132652 [details] [diff] [review]
Same, but with the retargeted load flag done right

Thoughts?  I'll update the documentation accordingly, of course...

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 132652 [details] [diff] [review]
Same, but with the retargeted load flag done right

in TryContentListener:

+  // At this point, aListener wants data of type aContentType.  Let'em
+  // have it.	

There is no aContentType... 
And shouldn't there be a space before 'em?

But first, if we are retargeting, so set an appropriate
+  // flag on the channel

s/so// ?

+  nsresult rv = aListener->DoContent(mTargetType.get(),
+					mIsContentPreferred,

wrong indentation, also in the next few lines

also... |nsresult rv| is declared twice in this function... maybe declare it
near the top of the function once.

+  if (NS_FAILED(rv)) {
you should probably unset LOAD_RETARGETED if DoContent failed...


Question: Should DoContent returning abort=TRUE mean that the channel should be
Cancel()ed?

+  // Fifth step:  If no listener prefers this type, see if any stream
+  //		   decoders [...]

s/decoder/converter/ ?


r=me with that changes

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Question: Should DoContent returning abort=TRUE 

I'm not quite sure what that's intended to mean, actually (when the callee
aborts like that).  Darin?  Do you know?  I basically left the old behavior, but
that could well be wrong....

Fixed the other issues, except the rv thing -- I see no problem with declaring
it twice in the blocks where it's actually used.

Comment 9

[Darin Fisher]

Comment on attachment 132652 [details] [diff] [review]
Same, but with the retargeted load flag done right

>Index: uriloader/base/nsURILoader.cpp

>+  /**
>+   * A pointer to the entity that originated the load.  This should
>+   * implement nsIInterfaceRequestor; we depend on getting things like
>+   * nsIURIContentListeners, nsIDOMWindows, etc off of it.
>+   */
>   nsCOMPtr<nsISupports> m_originalContext;

what if it doesn't implement nsIInterfaceRequestor?  is there any value
in holding onto it if it does not?  is it a programming error if it does 
not?  the reason why i am asking is that i'm really wondering if we can
just store nsCOMPtr<nsIInterfaceRequestor> instead as a minor optimization.
i.e., do the QI to nsIInterfaceRequestor only once.


>+  /**
>+   * Boolean to Pass to CanHandleContent (also determines whether we
>+   * use CanHandleContent or IsPreferred).
>+   */
>   PRBool mIsContentPreferred;

s/Pass/pass/ i presume.


otherwise, this patch looks really good to me.	i don't know much about
the |abort| parameter.	to answer biesi's question, i would look at who
sets it to TRUE, and see if any of them would care if the channel was
canceled or not.


sr=darin

Comment 10

[Boris Zbarsky [:bzbarsky]]

> what if it doesn't implement nsIInterfaceRequestor?

Some of our internal stuff fails to work right... but we pass this pointer out
to the embedding app in various ways (eg if it overrides our external helper app
handler), so they may want it back even if they don't implement
nsIInterfaceRequestor...

The problem with the abort arg is that embeddors use it, again.  None of the
callees in Mozilla proper ever set it to true.

ccing mscott's real mail address in case he has any insight into that end of
things, and checked in.  If we need to do something with the abort bool, I'd
rather do it in a separate patch...