Closed Bug 782342 Opened 7 years ago Closed 7 years ago

blob: protocol no Content-Length header

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: benjamin.bernard, Assigned: baku)

References

()

Details

Attachments

(2 files, 2 obsolete files)

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".
Andrea: Could you have a look at this bug?
Component: Untriaged → DOM
Product: Firefox → Core
Version: 14 Branch → Trunk
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] (khuey@mozilla.com) from comment #3)
> Of Bug 730765 and the general case of Bug 583444, I believe.
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.
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
We probably need to call SetContentLength there, among other things.
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).
If nobody is working on this bug, I take care of it :)
Assignee: nobody → amarchesini
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
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.
Attachment #651804 - Flags: feedback?
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?
> 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.
> > 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.
Let's fix the content-type issue in a separated bug.
This patch is about the content-length.
Attachment #651804 - Attachment is obsolete: true
Attachment #651804 - Flags: feedback?
Attachment #652347 - Flags: review?(jonas)
Comment on attachment 652347 [details] [diff] [review]
Bug 782342 - blob: protocol no Content-Length header

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

Sweet!
Attachment #652347 - Flags: review?(jonas) → review+
Attachment #652347 - Flags: checkin?
title changed.
Attachment #652347 - Attachment is obsolete: true
Attachment #652347 - Flags: checkin?
Attachment #654655 - Flags: checkin?
Keywords: checkin-needed
Attachment #654655 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/3854823c96e2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.