790132 - Need a test to make sure we only sniff when we intend to.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Paul Adenot (:padenot)]

Attachment: Patch that passes, which a questionnable fifth case.

To avoid regressions like in bug 789077, here is a test that makes sure the media sniffer only modifies the content type if:

- it is not present in the response
- if it is application/octet-stream and we set the flag |LOAD_TREAT_APPLICATION_OCTET_STREAM_AS_UNKNOWN| on the channel

However, in my test, when the content-type is not present and the flag |LOAD_CALL_CONTENT_SNIFFERS| is set on the channel (fifth entry in the |tests| array, in the test), the following happens:

- CallOnStartRequest is called on the nsHttpChannel.
- We assign an nsUnknownDecoder on mListener, because the content type is empty.
- We sniff using the nsMediaSniffer because |LOAD_CALL_CONTENT_SNIFFERS| is set, it finds a media as expected.
- We call mListener->OnStartRequest, the nsUnknownDecoder does _not_ call the remaining listeners (I tried to blame to know why, but it's way too old, and the commit message is a bit evasive).
- When OnStopRequest happens, the nsUnknownDecoder sniffs, and sets the content type to application/octet-stream, in LastDitchSniff, because everything else failed. Then the other listeners are called (including the JS listener I use in the xpcshell test to check), but at that point, the content type is wrong, i.e. is application/octet-stream.

I'm not sure if it's something we want (I certainly wasn't expect that). Maybe we could change the sniffing logic a bit to avoid overwriting the content type when it has already been sniffed? Or I'm missing something, and the content-type should be application/octet-stream in that case.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> - We sniff using the nsMediaSniffer because |LOAD_CALL_CONTENT_SNIFFERS| is set, it
> finds a media as expected.

Hmm.  So this is broken.  We really shouldn't be triggering nsUnknownDecoder if the LOAD_CALL_CONTENT_SNIFFERS thing set a type.  Please file a bug on the HTTP code?

That said, I'm reading nsHttpChannel::OnStartRequest and trying to understand why nsMediaSniffer is set up as a content sniffer at all.  Don't the media loads pass the LOAD_TREAT_APPLICATION_OCTET_STREAM_AS_UNKNOWN flag?  And if so, shouldn't nsUnknownDecoder just end up sniffing the data?  Does nsUnknownDecoder not do media sniffing?  It so, maybe it should?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 659958 [details] [diff] [review]
Patch that passes, which a questionnable fifth case.

Please take out that printf, but otherwise looks decent.

Comment 3

[Paul Adenot (:padenot)]

(In reply to Boris Zbarsky (:bz) from comment #1)
> > - We sniff using the nsMediaSniffer because |LOAD_CALL_CONTENT_SNIFFERS| is set, it
> > finds a media as expected.
> 
> Hmm.  So this is broken.  We really shouldn't be triggering nsUnknownDecoder
> if the LOAD_CALL_CONTENT_SNIFFERS thing set a type.  Please file a bug on
> the HTTP code?

Bug 790524.

> That said, I'm reading nsHttpChannel::OnStartRequest and trying to
> understand why nsMediaSniffer is set up as a content sniffer at all.  Don't
> the media loads pass the LOAD_TREAT_APPLICATION_OCTET_STREAM_AS_UNKNOWN
> flag?  And if so, shouldn't nsUnknownDecoder just end up sniffing the data? 
> Does nsUnknownDecoder not do media sniffing?  It so, maybe it should?

To be clear, here, sniffing works just fine when we are loading things in a media element, that is passing the flag LOAD_TREAT_APPLICATION_OCTET_STREAM_AS_UNKNOWN.

The nsMediaSniffer is set as a content sniffer because we want to sniff when a video or an audio file is directly loaded (and not using a media element, so LOAD_TREAT_APPLICATION_OCTET_STREAM_AS_UNKNOWN is not set, but LOAD_CALL_CONTENT_SNIFFER is set). Although I just spoke to Chris Pearce, and he does not feel too strongly about sniffing in top level document for media.

nsUnknownDecoder does media sniffing in OnDataAvailable (because it calls TryContentSniffer), but at that point, the content-type is already known (because we just sniffed it using nsMediaSniffer in CallOnStartRequest), so it bails out (as per bug 789077), returning NS_ERROR_NOT_AVAILABLE. nsUnknownDecoder considers that the sniffing failed, and when calling LastDitchSniff at the end, sets the content-type to application/octet-stream.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, we don't set OCTET_STREAM_AS_UNKNOWN for toplevel loads, right.  OK.  But we do want to sniff for media there?

I hate the web sometimes.  :(

So yeah, we should just fix bug 790524.

Comment 5

[Paul Adenot (:padenot)]

Attachment: Add a test to make sure media sniffing occurs only when we want.

Remove the printfs.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2ff3ec4dbcb2 (this patch
on the top of the queue).

Comment 6

[Paul Adenot (:padenot)]

It's green, I guess we can land.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

(In reply to Paul ADENOT (:padenot) from comment #5)
> Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2ff3ec4dbcb2

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/432bdfd10aaa

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/432bdfd10aaa