767087 - Add a flag to force content sniffing on nsHttpChannel and nsBaseChannel.

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Paul Adenot (:padenot)]

For bug 567077, we need a flag to force the type sniffers to sniff for the type in the case where the Content-Type header is "application/octet-stream". It has been proposed that the change would be made in necko.

Comment 1

[Paul Adenot (:padenot)]

Attachment: Patch v0: add a flag for force content type sniffing for nsHttpChannel and nsBaseChannel.

Here is a possible patch, works great in combination with patches for bug 567077.

I'm new in that module, feel free to pass the review to another person if you feel you're not the right peer to ask for review here (or if you are too busy).

Comment 2

[Jason Duell]

Comment on attachment 635815 [details] [diff] [review]
Patch v0: add a flag for force content type sniffing for nsHttpChannel and nsBaseChannel.

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

Looks good.  I assume you've tested this manually?

bz: do you think this needs an xpcshell test, or is it simple enough that we're ok w/o one?

::: netwerk/base/public/nsIChannel.idl
@@ +242,5 @@
>      const unsigned long LOAD_CLASSIFY_URI = 1 << 22;
>  
>      /**
> +     * If this flag is set, the channel should call the content sniffers
> +     * registered in the "content-sniffing-services" category, if the

Let's go with this instead:

If this flag is set and a server's response is Content-Type application/octet-steam, the server's Content-Type will be ignored and the channel content will be sniffed as though no Content-Type had been passed.

::: netwerk/base/src/nsBaseChannel.cpp
@@ +688,5 @@
> +  // going as-is.
> +  bool shouldSniff = mContentType.EqualsLiteral(UNKNOWN_CONTENT_TYPE) ||
> +            (mLoadFlags & LOAD_TREAT_APPLICATION_OCTET_STREAM_AS_UNKNOWN) &&
> +            mContentType.EqualsLiteral(APPLICATION_OCTET_STREAM);
> +

Everyone should know that && binds closer than ||: heck, we could do w/o any *any* parentheses in the whole expression.   But just to be clear, let's put another level of parenthesis around the whole || expression:

bool shouldSniff = mContentType.EqualsLiteral(UNKNOWN_CONTENT_TYPE) ||
    ((mLoadFlags & LOAD_TREAT_APPLICATION_OCTET_STREAM_AS_UNKNOWN) &&
        mContentType.EqualsLiteral(APPLICATION_OCTET_STREAM));

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +881,5 @@
>      mTracingEnabled = false;
>  
> +    if (mResponseHead && (mResponseHead->ContentType().IsEmpty() ||
> +        mResponseHead->ContentType().EqualsLiteral(APPLICATION_OCTET_STREAM) &&
> +        (mLoadFlags & LOAD_TREAT_APPLICATION_OCTET_STREAM_AS_UNKNOWN))) {

To keep this more legible, let's declare 'bool shouldSniff' above this if statement, like you do for nsBaseChannel (and add extra parens there too)

Comment 3

[Paul Adenot (:padenot)]

> Looks good.  I assume you've tested this manually?

Yes. Plus it is going to be tested by the tests I've written for bug 567077 (which is the reason why we need this feature in the first place). I could probably write another test for that feature if needed.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 635815 [details] [diff] [review]
Patch v0: add a flag for force content type sniffing for nsHttpChannel and nsBaseChannel.

This looks fine, though an xpcshell test can't hurt.  Shouldn't be hard to do....

Comment 5

[Paul Adenot (:padenot)]

Attachment: Addressed jduell's comments.

Carrying r+ forward.

Comment 6

[Paul Adenot (:padenot)]

Attachment: XPCShell test for the feature.

Here is a test for that feature, green on try [1].

[1]: https://tbpl.mozilla.org/?tree=Try&rev=af53f8131c39

Comment 7

[Jason Duell]

Comment on attachment 636778 [details] [diff] [review]
XPCShell test for the feature.

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

Thanks Paul!

http://hg.mozilla.org/integration/mozilla-inbound/rev/0a46999bfecb
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d4d8153d27d

::: netwerk/test/unit/test_force_sniffing.js
@@ +1,1 @@
> +// This file tests the flat LOAD_TREAT_APPLICATION_OCTET_STREAM_AS_UNKNOWN.

s/flat/flag

@@ +70,5 @@
> +  httpserv = new nsHttpServer();
> +  httpserv.registerPathHandler("/test", handler);
> +  httpserv.start(4444);
> +
> +  // Register our fake sniffer that always return the content-type we want.

returns

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0a46999bfecb
https://hg.mozilla.org/mozilla-central/rev/2d4d8153d27d