907575 - mResniffMimeType not properly reset

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alice0775 White]

This is spun off from Bug 907490.

Steps to reproduce:
1. Visited http://plover.com/~mjd/swatch.html.

Actual Results:
The clock does not work properly. It displays a broken image icon alternately.

Expected Results:
The clock whould work properly. No broken image icon should not display.

Comment 1

[Alice0775 White]

s/whould/should/

Comment 2

[Alice0775 White]

There is 2 regression.


#1 Regression window
Good:
http://hg.mozilla.org/mozilla-central/rev/f16bcf1fbf73
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120519182851
Not update:
http://hg.mozilla.org/mozilla-central/rev/0e2cc686b07b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120519211152
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f16bcf1fbf73&tochange=0e2cc686b07b

Regressed by:Bug 733553


#2 Regression window
Not update:
http://hg.mozilla.org/mozilla-central/rev/9cfb80a82883
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120922003414
Broken Image Icon:
http://hg.mozilla.org/mozilla-central/rev/b461a7cd250e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120922052915
Pushlog
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9cfb80a82883&tochange=b461a7cd250e

Regressed by Bug 787899

Comment 3

[Adam]

MIME/Content-type discovery problem, mixed with a lying server. The first part correctly has the header "Content-type: image/jpeg" while all subsequent parts erroneously have headers "Content-type: image/gif" (all parts are JPEG).

Each part is sniffed (imgRequest::OnDataAvailable) for its real type, but for some reason alternating parts fail, and get their type from the channel (= "image/gif").

The bug looks to be in which data are handed to type sniffing.

Comment 4

[Adam]

Attachment: Reset mResniffMimeType after the first part

The mResniffMimeType flag was previously only reset (set false) if we changed types (or were SVG). But we should have been resetting every time and relying on imgRequest::OnStartRequest to enable it.

The old way means that pieces of parts would be resniffed for mime type; that only didn't cause problems because most servers don't lie about their types.

Old way:
1. Part A piece 1 comes
   a. Sniff and find image/jpeg
   b. Ignore the erroneous part header ("Content-type: image/gif")
   c. Set mResniff=false (because mContentType wasn't set before)
2. Part A piece 2 comes, but mResniff=false
3. [Rest of Part A comes, successfully decoded as JPEG]
4. OnStartRequest sets mResniff=true
5. Part B piece 1 comes
   a. Sniff and find image/jpeg
   b. Ignore the part header
   c. Don't touch mResniff(=true) (because the content type didn't change)
6. Part B piece 2 comes
   a. Sniff and fail
   b. Fall back to erroneous image/gif
   c. Set mResniff=false (because mContentType changed from jpeg to gif)
7. [Rest of Part B comes, fails to decode as GIF]
8. OnStartRequest sets mResniff=true
9. Repeat from [1], except [1c] explanation becomes "because we switch from image/gif to image/jpeg"

New way:
At [5c], we instead set mResniff=false. At [6] we don't sniff (or ever for intermediate pieces).

Comment 5

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 794408 [details] [diff] [review]
Reset mResniffMimeType after the first part

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

Looks good! r+'d with the issue below addressed. Adam, are you familiar with Mozilla tests enough to write a test to go with this?

::: image/src/imgRequest.cpp
@@ +774,5 @@
>  
>        if (mDecodeRequested)
>          mImage->StartDecoding();
>      }
> +    mResniffMimeType = false;

I'd prefer if you placed this right after the |if (mResniffMimeType)| block, so that the behavior doesn't differ if the image has an error and we take the early return path. Although I don't see any reason why that'd make a difference in this case, I prefer that we have a clear invariant that mResniffMimeType always gets reset to false after OnDataAvailable runs.

Comment 6

[Adam]

Attachment: Maintain reset mResniff invariant early, add a test

Thanks, Seth!

This patch deviates from what you suggest. A skeleton of the relevant code:

> if (!mGotData || mResniffMimeType) {
>   if (mContentType != newType || newType.EqualsLiteral(IMAGE_SVG_XML)) {
>     if (mResniffMimeType) {
>     }
>   }
> }

The reset needs to occur in the outermost block (it can additionally occur in other blocks) because we hit cases where we don't change types (and aren't using SVG) where mResniff is true, which means we will resniff on the pieces of those parts.

This patch uses stores and resets mResniff early:

> if (!mGotData || mResniffMimeType) {
>   // Store and reset this for the invariant that it's always false after
>   // calls to OnDataAvailable (see bug 907575)
>   bool resniffMimeType = mResniffMimeType;
>   mResniffMimeType = false;
>   if (mContentType != newType || newType.EqualsLiteral(IMAGE_SVG_XML)) {
>     if (resniffMimeType) {

This maintains the invariant if we return early from error, while avoiding multiple resets. If you'd prefer, we could reset mResniffMimeType in both places, but this version feels cleaner to me.

The patch adds a test to the existing test from bug 733553. I'd rather not add a standalone test for a relatively small test case, given the testing overhead for multipart tests. Success/failure is determined by checking the image width after loading each part.

For the test, we send a part with the wrong type. The part must also exceed a single TCP frame (hence the image chosen for the test case). If it fit a single frame, there would be no next piece to be resniffed if the invariant failed.

Unfortunately, failure shows up on the part following our faulty part. My guess is that the first piece is sufficient to set the width, so it passes. But between bailing out mid-decode of that part and attempting a new decode of an erroneous gif (on the remaining pieces), the next part fails. 

Whatever the cause, I added comments to the test files to clarify that behavior for future investigations of test failure.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 796298 [details] [diff] [review]
Maintain reset mResniff invariant early, add a test

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

Looks good!

Comment 8

[Adam]

Attachment: Prepared for checkin

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=dc30fbd7f6bf

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86c7b240f660

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/86c7b240f660