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]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 669049
Blocks: 649133
  Show dependency treegraph
 
Reported: 2011-06-02 10:51 PDT by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
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 | Splinter Review
patch v2 (5.05 KB, patch)
2011-06-03 20:58 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
patch v2 (3.00 KB, patch)
2011-06-03 21:00 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
patch v2 (9.24 KB, patch)
2011-06-03 21:01 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
patch v2.1 (9.26 KB, patch)
2011-06-03 21:57 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
patch v3 (8.96 KB, patch)
2011-06-19 11:43 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Bandaid patch (3.80 KB, patch)
2011-06-30 13:42 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
jonas: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 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 User image Masatoshi Kimura [:emk] 2011-06-03 09:32:02 PDT
Created attachment 537160 [details] [diff] [review]
patch
Comment 4 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 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 User image 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 User image Masatoshi Kimura [:emk] 2011-06-03 21:00:04 PDT
Created attachment 537302 [details] [diff] [review]
patch v2

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

Sorry for the spam...
Comment 8 User image 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 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 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 User image 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 User image 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image 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 User image 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-01 19:37:02 PDT
http://hg.mozilla.org/mozilla-central/rev/84bbc200e89a
Comment 19 User image 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 User image Masatoshi Kimura [:emk] 2011-07-03 00:45:54 PDT
Created attachment 543658 [details]
64K dummy file
Comment 21 User image 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 User image 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 User image 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.