Last Comment Bug 767087 - Add a flag to force content sniffing on nsHttpChannel and nsBaseChannel.
: Add a flag to force content sniffing on nsHttpChannel and nsBaseChannel.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: 16 Branch
: All All
: -- normal (vote)
: mozilla16
Assigned To: Paul Adenot (:padenot) (in PTO until early September)
:
Mentors:
Depends on:
Blocks: 567077
  Show dependency treegraph
 
Reported: 2012-06-21 12:13 PDT by Paul Adenot (:padenot) (in PTO until early September)
Modified: 2012-07-03 02:15 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v0: add a flag for force content type sniffing for nsHttpChannel and nsBaseChannel. (3.92 KB, patch)
2012-06-22 11:08 PDT, Paul Adenot (:padenot) (in PTO until early September)
jduell.mcbugs: review+
bzbarsky: review+
Details | Diff | Splinter Review
Addressed jduell's comments. (4.02 KB, patch)
2012-06-26 10:33 PDT, Paul Adenot (:padenot) (in PTO until early September)
padenot: review+
Details | Diff | Splinter Review
XPCShell test for the feature. (4.00 KB, patch)
2012-06-26 10:35 PDT, Paul Adenot (:padenot) (in PTO until early September)
jduell.mcbugs: review+
Details | Diff | Splinter Review

Description Paul Adenot (:padenot) (in PTO until early September) 2012-06-21 12:13:52 PDT
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) (in PTO until early September) 2012-06-22 11:08:54 PDT
Created attachment 635815 [details] [diff] [review]
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 [:jduell] (needinfo me) 2012-06-24 21:49:57 PDT
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) (in PTO until early September) 2012-06-24 23:11:40 PDT
> 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 [:bz] 2012-06-25 11:20:08 PDT
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) (in PTO until early September) 2012-06-26 10:33:00 PDT
Created attachment 636773 [details] [diff] [review]
Addressed jduell's comments.

Carrying r+ forward.
Comment 6 Paul Adenot (:padenot) (in PTO until early September) 2012-06-26 10:35:35 PDT
Created attachment 636778 [details] [diff] [review]
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 [:jduell] (needinfo me) 2012-07-02 16:45:58 PDT
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

Note You need to log in before you can comment on or make changes to this bug.