661582 - xhr.response should return a Blob, not a File, when appropriate

Status: RESOLVED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

xhr.responseType = "blob";
xhr.response instanceof Blob; // Should be true, is true
xhr.response instanceof File; // Should be false, is true

We need to set the mIsFullFile flag on nsDOMFile to false.  This might require a slightly different constructor.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This also means we leak the filename from the cache to the web page.  I don't think that's a security/privacy problem, but it's not ideal.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is a *potential* security/privacy leak. I can't think of any attacks either, but it's making me a bit nervous and should be easy to fix.

Masatoshi: Do you think you'd have time to fix this soonish? If not I should be able to.

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: patch

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 537160 [details] [diff] [review]
patch

It seems to me that a better solution would be to introduce a new constructor which sets the mIsFile flag to false. Both to nsDOMMemoryFile and nsDOMFile.

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: patch v2

Added a constructor to nsDOMFile and nsDOMMemoryFile.

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: patch v2

Sorry, wrong file.

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: patch v2

Sorry for the spam...

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: patch v2.1

Unreferencing mFile after bResponseBlob has been created.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 537312 [details] [diff] [review]
patch v2.1

>diff --git a/content/base/public/nsDOMFile.h b/content/base/public/nsDOMFile.h
>--- a/content/base/public/nsDOMFile.h
>+++ b/content/base/public/nsDOMFile.h
>@@ -79,16 +79,29 @@ public:
>       mIsFullFile(true)
>   {}
> 
>   nsDOMFile(nsIFile *aFile)
>     : mFile(aFile),
>       mIsFullFile(true)
>   {}
> 
>+  nsDOMFile(nsIFile *aFile, PRUint64 aStart, PRUint64 aLength,
>+            const nsAString& aContentType)
>+    : mFile(aFile),
>+      mStart(aStart),
>+      mLength(aLength),
>+      mContentType(aContentType),
>+      mIsFullFile(false)
>+  {

I think you can do this even simpler. Give the current

nsDOMFile(nsIFile *aFile, const nsAString& aContentType)

constructor another bool argument for which defaults to true and set mIsFullFile to the value of that argument.

Comment 10

[Masatoshi Kimura [:emk]]

I had to give a length explicitly for non-full file blobs, otherwise nsDOMFile::GetSize will not work correctly.
I don't think it's a good idea calling nsIFile::GetFileSize inside the constructor. Is it better to rewrite nsDOMFile::GetSize to support constructing non-full file blobs without a length?

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: patch v3

Added a boolean flag to indicate whether the filename sould be hidden or not.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Bandaid patch

This is hacky, but we need a bandaid for 6.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 543234 [details] [diff] [review]
Bandaid patch

Drivers, this patch fixes a correctness issue and a potential security/privacy leak (of a cache filename, N.B. comments 1 and 2) with a new feature introduced in Firefox 6.  This patch is a bandaid because the "correct" fix here requires significant refactoring that is out of scope for Aurora.  I believe the risk in this patch is low (and in well-tested code).

Comment 14

[LegNeato]

Comment on attachment 543234 [details] [diff] [review]
Bandaid patch

Approved for releases/mozilla-aurora. Please land asap before 2011-07-05 @ 9:00 am PDT.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/42f2aca1fde9
http://hg.mozilla.org/releases/mozilla-aurora/rev/4afee8a13b96

I think the real fix here is to refactor nsDOMFile to be cleaner, which Jonas has plans to do, so I'm going to close this out.

Comment 16

[Masatoshi Kimura [:emk]]

Comment on attachment 540335 [details] [diff] [review]
patch v3

Canceling review per comment #15.

Comment 17

[Matt Brubeck (:mbrubeck)]

Backed out from mozilla-central during investigation of Android browser-chrome test failures:
http://hg.mozilla.org/mozilla-central/rev/00bb08972e46

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/84bbc200e89a

Comment 19

[Masatoshi Kimura [:emk]]

Comment on attachment 543234 [details] [diff] [review]
Bandaid patch

This patch doesn't work for larger remote file because the cache file size is 0 bytes at the time of CreateResponseBlob unless the file is already stored in the cache. And the blob size will be "frozen" by mozSlice call.
It's the reason I moved |new nsDOMFile| call from CreateResponseBlob in my v2 patch.
Mochitest couldn't catch this because smaller files (< 8K) will not be written in the cache even if cacheAsFile flag is set.
Sorry, I should have added a test to catch this case.

Comment 20

[Masatoshi Kimura [:emk]]

Attachment: 64K dummy file

Comment 21

[Masatoshi Kimura [:emk]]

Attachment: testcase

This test will fail with latest Nightly/Aurora on first load.
It will succeed on reload.

Comment 22

[Masatoshi Kimura [:emk]]

> smaller files (< 8K) will not be written in the cache
smaller files (< 8K) will not be written as separate cache files

Comment 23

[Masatoshi Kimura [:emk]]

I've filed a new bug because the cache issue not directly related to this bug (xhr.response should return a Blob).