Last Comment Bug 661582 - xhr.response should return a Blob, not a File, when appropriate
: xhr.response should return a Blob, not a File, when appropriate
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla6
Assigned To: Masatoshi Kimura [:emk]
:
Mentors:
Depends on: 669049
Blocks: 649133
  Show dependency treegraph
 
Reported: 2011-06-02 10:51 PDT by Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
Modified: 2011-07-03 00:57 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
affected


Attachments
patch (4.22 KB, patch)
2011-06-03 09:32 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
patch v2 (5.05 KB, patch)
2011-06-03 20:58 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
patch v2 (3.00 KB, patch)
2011-06-03 21:00 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
patch v2 (9.24 KB, patch)
2011-06-03 21:01 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
patch v2.1 (9.26 KB, patch)
2011-06-03 21:57 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
patch v3 (8.96 KB, patch)
2011-06-19 11:43 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
Bandaid patch (3.80 KB, patch)
2011-06-30 13:42 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
jonas: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Review
64K dummy file (64.00 KB, application/octet-stream)
2011-07-03 00:45 PDT, Masatoshi Kimura [:emk]
no flags Details
testcase (262 bytes, text/html)
2011-07-03 00:48 PDT, Masatoshi Kimura [:emk]
no flags Details

Description Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-06-02 10:51:30 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-06-02 10:52:09 PDT
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) 2011-06-02 11:20:50 PDT
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] 2011-06-03 09:32:02 PDT
Created attachment 537160 [details] [diff] [review]
patch
Comment 4 Jonas Sicking (:sicking) 2011-06-03 11:10:34 PDT
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] 2011-06-03 20:58:30 PDT
Created attachment 537300 [details] [diff] [review]
patch v2

Added a constructor to nsDOMFile and nsDOMMemoryFile.
Comment 6 Masatoshi Kimura [:emk] 2011-06-03 21:00:04 PDT
Created attachment 537302 [details] [diff] [review]
patch v2

Sorry, wrong file.
Comment 7 Masatoshi Kimura [:emk] 2011-06-03 21:01:42 PDT
Created attachment 537304 [details] [diff] [review]
patch v2

Sorry for the spam...
Comment 8 Masatoshi Kimura [:emk] 2011-06-03 21:57:43 PDT
Created attachment 537312 [details] [diff] [review]
patch v2.1

Unreferencing mFile after bResponseBlob has been created.
Comment 9 Jonas Sicking (:sicking) 2011-06-09 17:41:01 PDT
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] 2011-06-10 06:32:47 PDT
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] 2011-06-19 11:43:16 PDT
Created attachment 540335 [details] [diff] [review]
patch v3

Added a boolean flag to indicate whether the filename sould be hidden or not.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-06-30 13:42:55 PDT
Created attachment 543234 [details] [diff] [review]
Bandaid patch

This is hacky, but we need a bandaid for 6.
Comment 13 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-06-30 13:50:04 PDT
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 christian 2011-06-30 14:36:36 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-06-30 18:19:26 PDT
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] 2011-07-01 06:16:45 PDT
Comment on attachment 540335 [details] [diff] [review]
patch v3

Canceling review per comment #15.
Comment 17 Matt Brubeck (:mbrubeck) 2011-07-01 12:05:42 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-07-01 19:37:02 PDT
http://hg.mozilla.org/mozilla-central/rev/84bbc200e89a
Comment 19 Masatoshi Kimura [:emk] 2011-07-03 00:35:45 PDT
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] 2011-07-03 00:45:54 PDT
Created attachment 543658 [details]
64K dummy file
Comment 21 Masatoshi Kimura [:emk] 2011-07-03 00:48:30 PDT
Created attachment 543659 [details]
testcase

This test will fail with latest Nightly/Aurora on first load.
It will succeed on reload.
Comment 22 Masatoshi Kimura [:emk] 2011-07-03 00:51:36 PDT
> 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] 2011-07-03 00:57:04 PDT
I've filed a new bug because the cache issue not directly related to this bug (xhr.response should return a Blob).

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