782342 - blob: protocol no Content-Length header

Status: RESOLVED FIXED

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

Description

[Benjamin BERNARD]

Attachment: A simple script to reproduce the bug (you will need an ogg file named demo.ogg in the same folder)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713224749

Steps to reproduce:

I load an ogg blob using his blob URL into an <audio></audio> player.


Actual results:

I can't get the duration of the audio file, it's played like a streamed media.
I think it's beause there is no "Content-Length" header in the Firefox implementation of the blob: protocol.

Test it here : http://experiments.benvii.com/blob_content_length/

Moreover you will also noticed the "Content-Type" header is not the right one :)

Same problem on "firefox for Android Beta" (v15).


Expected results:

I should have got the duration of the audio file, a "Content-Length" header and a "Content-Type: audio/ogg".

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Andrea: Could you have a look at this bug?

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is a dup.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Of Bug 730765 and the general case of Bug 583444, I believe.

Comment 4

[Benjamin BERNARD]

It's not really a duplicate.
Firefox will need to add a Content-Length header as it is going to be in the FileAPI spec.

By the way I think it will also solve Bug 583444 and solve the same problem with video.

You can see the bug for video here : http://experiments.benvii.com/blob_content_length/video.html

See this : https://www.w3.org/Bugs/Public/show_bug.cgi?id=18548#c0
And this discussion : http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0449.html
(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #3)
> Of Bug 730765 and the general case of Bug 583444, I believe.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

No, we need to behave like we received a Content-Length header.  We're not really serving blobs over HTTP on the local machine.

I've been following the discussions.

Comment 6

[Benjamin BERNARD]

Ok, because it's HTTP Channels.
I'm not good at C++ but is it concerning this part of the source code ? http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsBlobProtocolHandler.cpp#178

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We probably need to call SetContentLength there, among other things.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We also need to fake a Content-Length header in somewhere in the XMLHttpRequest code (calling getResponseHeaders on a blob URI load should have a Content-Length header).

Comment 9

[Andrea Marchesini [:baku]]

If nobody is working on this bug, I take care of it :)

Comment 10

[Andrea Marchesini [:baku]]

Attachment: Bug 782342 - blob: protocol no Content-Length header

Thank you for the script. I converted it in a mochi test :)
This patch has not been tested on try so I'm not asking for any review yet.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 651804 [details] [diff] [review]
Bug 782342 - blob: protocol no Content-Length header

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

::: content/base/src/nsXMLHttpRequest.cpp
@@ +1465,5 @@
> +  PRInt32 length;
> +  if (NS_SUCCEEDED(mChannel->GetContentLength(&length))) {
> +    aResponseHeaders.AppendLiteral("Content-Length: ");
> +    aResponseHeaders.AppendInt(length);
> +    aResponseHeaders.AppendLiteral("\r\n");

You need to make it work with GetResponseHeader as well.

And you probably don't want to do this for http channels since then there's a risk that the Content-Length header shows up twice. So only do this for non-http channels. (Same in GetResonseHeader).

@@ +3083,5 @@
>    // ignoring return value, as this is not critical
> +  nsCAutoString contentType;
> +  if (NS_FAILED(mChannel->GetContentType(contentType)) ||
> +      contentType.IsEmpty())
> +    mChannel->SetContentType(NS_LITERAL_CSTRING("application/xml"));

This doesn't look correct. GetContentType will likely always return an empty string here. What are you trying to fix here?

Comment 12

[Andrea Marchesini [:baku]]

> You need to make it work with GetResponseHeader as well.

Right.

> And you probably don't want to do this for http channels since then there's
> a risk that the Content-Length header shows up twice. So only do this for
> non-http channels. (Same in GetResonseHeader).

This code should not run for http channels.

> >    // ignoring return value, as this is not critical
> > +  nsCAutoString contentType;
> > +  if (NS_FAILED(mChannel->GetContentType(contentType)) ||
> > +      contentType.IsEmpty())
> > +    mChannel->SetContentType(NS_LITERAL_CSTRING("application/xml"));

The previous implementation forces 'application/xml' as content type. This overwrites what we set in the blob channel. So we want to apply the default content-type ONLY if this is not already set.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > And you probably don't want to do this for http channels since then there's
> > a risk that the Content-Length header shows up twice. So only do this for
> > non-http channels. (Same in GetResonseHeader).
> 
> This code should not run for http channels.

You're correct. My bad.

> > >    // ignoring return value, as this is not critical
> > > +  nsCAutoString contentType;
> > > +  if (NS_FAILED(mChannel->GetContentType(contentType)) ||
> > > +      contentType.IsEmpty())
> > > +    mChannel->SetContentType(NS_LITERAL_CSTRING("application/xml"));
> 
> The previous implementation forces 'application/xml' as content type. This
> overwrites what we set in the blob channel. So we want to apply the default
> content-type ONLY if this is not already set.

Ah. Makes sense for channels which have a contenttype even before they are opened. Please add a comment to that effect.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: Bug 782342 - blob: protocol no Content-Length header

Let's fix the content-type issue in a separated bug.
This patch is about the content-length.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 652347 [details] [diff] [review]
Bug 782342 - blob: protocol no Content-Length header

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

Sweet!

Comment 16

[Andrea Marchesini [:baku]]

Attachment: Bug 782342 - blob: protocol no Content-Length header, r=sicking

title changed.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=3eefa652b5f9

https://hg.mozilla.org/integration/mozilla-inbound/rev/3854823c96e2

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3854823c96e2