309525 - <object> loading doesn't do stream conversion

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

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

Loading stuff via <object> doesn't do any stream conversions, because it
directly calls nsIURIContentListener::DoContent.

This includes making nsObjectLoadingContent recognize types only supported by
stream converters as valid document types.

Comment 1

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

Attachment: eml file for testcase

Comment 2

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

Attachment: netwerk/ part

Comment 3

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

Attachment: uriloader/ part

Includes a necessary patch for rdf/chrome, so that IsPending works correctly (this is already fixed in chrome/).

Comment 4

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

Attachment: content/base/src part

I wonder if I should expose this on nsIWebNavigation, and call that...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 202911 [details] [diff] [review]
netwerk/ part

>Index: netwerk/streamconv/src/nsStreamConverterService.cpp
>+nsStreamConverterService::CanConvert(const char* 

>+    *_retval = NS_SUCCEEDED(rv) != PR_FALSE;

Isn't that the same as:

  *_retval = NS_SUCCEEDED(rv);

?

sr=bzbarsky

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 202912 [details] [diff] [review]
uriloader/ part

>Index: uriloader/base/nsIURILoader.idl
>+  /* @{ */

I assume that indicates to something that these two constants belong together?

>+  /**
>+   * Loads data from a channel. This differs from openURI in that the channel
>+   * may already be opened, and that it returns a stream listener into which the
>+   * caller should pump data.

I'm not sure I follow this.  What does it mean for this to return a stream listener?  What will this do if the channel is _not_ opened?  Still return a stream listener?  What data is the caller expected to pump into the listener this returns?

>+   * Note: If the channel already has a loadgroup, it will be replaced with a
>+   * different one.

Which one?  Or can we not document that in a reasonable way?

>+   * If the current listener refuses the load immediately

Which one is the "current listener"?

What does it mean to refuse the load immediately?

>, this method will
>+   * return NS_ERROR_WONT_HANDLE_CONTENT. If desired, the caller should cancel
>+   * the channel (this method won't).

s/If desired etc/At that point, the caller should probably cancel the channel if it's already open (this method will not cancel the channel)./

Or something like that, perhaps?

>+   * If flags include DONT_RETARGET, but the current listener refuses the load,

Again, what is "current listener", and how does "refuses the load" differ from "refuses the load immediately"?

>+   * @param aChannel
>+   *        The channel that should be loaded. The channel may already be
>+   *        opened.

May it also already be closed?  eg. a channel that's done loading?

>+   * @param aIsContentPreferred

aFlags, you mean?

>+   *        Should the content be displayed in a container that prefers the
>+   *        content-type, or will any container do.

And fix this comment, please?

>Index: uriloader/base/nsURILoader.cpp

I think the diff to this file would be a lot more readable as a diff -b (or diff -w).  Could you post a diff like that, please?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 202913 [details] [diff] [review]
content/base/src part

I need to think about this a bit more, but comments off top of my head:

1)  We don't really want to change nsIWebNavigation -- we want to freeze it more or less as-is.

2)
  
>@@ -1007,15 +1003,30 @@ nsObjectLoadingContent::IsSupportedDocum

>+    if (supported == nsIWebNavigationInfo::UNSUPPORTED) ...
>+        rv = convServ->CanConvert(aMimeType.get(), "*/*", &canConvert);

>+      return NS_SUCCEEDED(rv) && canConvert;

I'm not sure I buy that... Just because we can convert the type to something doesn't mean the something will be a supported document, does it?  Consider, eg, the case when the type is application/x-unknown-content-type and the unknown decoder detects it as PDF.

If there's something I'm missing, it should probably be documented here with this code... ;)

Comment 8

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

(In reply to comment #5)
> Isn't that the same as:
> 
>   *_retval = NS_SUCCEEDED(rv);

Hm, yes, looks like it is for NS_SUCCEEDED (not for NS_FAILED though)

(In reply to comment #6)
> I assume that indicates to something that these two constants belong together?

Yeah, doxygen will group them together with a "Flags for opening URIs." header.

> What data is the caller expected to pump into the listener
> this returns?

Well, the point is that this stream listener will receive the data from the channel. I wanted to allow both doing something like:
  var chan = newChannel(...);
  var listener = uriLoader.openChannel(chan);
  return chan.asyncOpen(listener, nsnull);
(which openURI now does)

And the other approach, calling it inside a listener's onStartRequest method.

I should probably clarify the comment.

> Which one?  Or can we not document that in a reasonable way?

does anyone care which one it is? :-)

> Which one is the "current listener"?

The one from aWindowContext... I should document this better

> What does it mean to refuse the load immediately?

You mean I should document that this calls onStartURIOpen?

>Again, what is "current listener", and how does "refuses the load" differ from
>"refuses the load immediately"?

It differs in that the caller only refuses it in onStartRequest :-) (canHandleContent etc)... hm.. probably needs rewording too.

> May it also already be closed?  eg. a channel that's done loading?

ohh, that's an interesting question. I'm pretty sure the impl would screw this up, with that isPending check. so, no :-)




(In reply to comment #7)
> 1)  We don't really want to change nsIWebNavigation -- we want to freeze it
> more or less as-is.

Yes, I meant as an nsIWebNavigation2 method or so.

> I'm not sure I buy that... Just because we can convert the type to something
> doesn't mean the something will be a supported document, does it?  Consider,
> eg, the case when the type is application/x-unknown-content-type and the
> unknown decoder detects it as PDF.

Yeah, hm, but we can only detect that in onStartRequest. At that point, uriloader will call canHandleContent, which fails, uriloader returns NS_ERROR_WONT_HANDLE_CONTENT, which means onStartRequest fails, which means we fall back. So I think we're ok here. I should probably test this.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I should probably clarify the comment.

Yes, exactly.

> does anyone care which one it is? :-)

I do, if we're putting this in an api comment.  Why bother mentioning it at all otherwise?

> The one from aWindowContext... I should document this better

Yes, please.

> You mean I should document that this calls onStartURIOpen?

For example, yes.  :)  But seriously, I didn't have any idea what that comment was talking about.  Note that "listener" in this comment can mean either nsIStreamListener or nsIURIContentListener depending on the use; it would be good to disambiguate...

> probably needs rewording too.

Right.

> Yeah, hm, but we can only detect that in onStartRequest. At that point,
> uriloader will call canHandleContent, which fails, uriloader returns
> NS_ERROR_WONT_HANDLE_CONTENT, which means onStartRequest fails, which means we
> fall back. So I think we're ok here. I should probably test this.

Please.  And document.

Comment 10

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

> Why bother mentioning it at all otherwise?

The reason I mentioned is that the caller might assume that the loadgroup he set might be the one that's used. I wanted to make it clear that it will get replaced.

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK.  But then I think it's worth saying what it gets replaced with...

Comment 12

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

Attachment: netwerk/ part, v2

Comment 13

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

Attachment: uriloader/ part, v2

Comment 14

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

Attachment: uriloader/ part, v2, as diff -b

Comment 15

[Darin Fisher]

Comment on attachment 203663 [details] [diff] [review]
netwerk/ part, v2

r=darin

Comment 16

[Darin Fisher]

Comment on attachment 203672 [details] [diff] [review]
uriloader/ part, v2

I'll sr this after boris has finished his review of it.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 203672 [details] [diff] [review]
uriloader/ part, v2

Thanks for the comment improvements!  r=bzbarsky

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 202913 [details] [diff] [review]
content/base/src part

>Index: content/base/src/nsObjectLoadingContent.cpp
>+      rv = uriLoader->OpenChannel(chan, nsIURILoader::IS_CONTENT_PREFERRED |
>+                                  nsIURILoader::DONT_RETARGET, req,

Why nsIURILoader::IS_CONTENT_PREFERRED ?  That should only be set for link clicks, no?  That's certainly what docshell does it passes mLoadType == LOAD_LINK for the aIsContentPreferred arg of nsIURILoader::LoadURI.

r+sr=bzbarsky with that fixed.

Comment 19

[Darin Fisher]

Comment on attachment 203673 [details] [diff] [review]
uriloader/ part, v2, as diff -b

sr=darin

Comment 20

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

hm... I did use to pass true to the isContentPreferred argument:
-      rv = listener->DoContent(mContentType.get(), PR_TRUE, aRequest,

I guess that was wrong. I'll remove that flag before checking in. (note to self: still need to do aforementioned testing)

Comment 21

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

Testcase for a stream converter that converts to something unsupported:
data:text/html,<object%20data="data:application/x-unknown-content-type,FAIL%2500">PASS</object>

(Verification that this does what one would expect:
data:text/html,<object%20data="data:application/x-unknown-content-type,Text%20content">Alternate</object>
)

Works fine with the patch: The first one shows "PASS", the second one "Text content" (as expected)

Comment 22

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

Attachment: content/base/src part, v2

Comment 23

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

netwerk part:
Checking in netwerk/streamconv/public/nsIStreamConverterService.idl;
/cvsroot/mozilla/netwerk/streamconv/public/nsIStreamConverterService.idl,v  <--  nsIStreamConverterService.idl
new revision: 1.11; previous revision: 1.10
done
Checking in netwerk/streamconv/src/nsStreamConverterService.cpp;
/cvsroot/mozilla/netwerk/streamconv/src/nsStreamConverterService.cpp,v  <--  nsStreamConverterService.cpp
new revision: 1.67; previous revision: 1.66
done

uriloader part:
Checking in rdf/chrome/src/nsChromeProtocolHandler.cpp;
/cvsroot/mozilla/rdf/chrome/src/nsChromeProtocolHandler.cpp,v  <--  nsChromeProtocolHandler.cpp
new revision: 1.108; previous revision: 1.107
done
Checking in uriloader/base/nsIURILoader.idl;
/cvsroot/mozilla/uriloader/base/nsIURILoader.idl,v  <--  nsIURILoader.idl
new revision: 1.34; previous revision: 1.33
done
Checking in uriloader/base/nsURILoader.cpp;
/cvsroot/mozilla/uriloader/base/nsURILoader.cpp,v  <--  nsURILoader.cpp
new revision: 1.138; previous revision: 1.137
done

Comment 24

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

Attachment: content/base/src part, v2.1

the comment change in v2 was wrong... this one is right (also has the correct diff parameters)

Comment 25

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

content part:
Checking in content/base/src/Makefile.in;
/cvsroot/mozilla/content/base/src/Makefile.in,v  <--  Makefile.in
new revision: 1.84; previous revision: 1.83
done
Checking in content/base/src/nsObjectLoadingContent.cpp;
/cvsroot/mozilla/content/base/src/nsObjectLoadingContent.cpp,v  <--  nsObjectLoadingContent.cpp
new revision: 1.18; previous revision: 1.17
done

Comment 26

[georgi - hopefully not receiving bugspam]

this seems interesting functionality.

what it is supposed the eml testcase to do (i get FAIL on latest trunk)?

Comment 27

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

requires seamonkey with mailnews. you should see the mail message in the <object>.

Comment 28

[David :Bienvenu]

I think this fix caused bug 320732 - perhaps mailnews is doing something wrong...investigating