Chrome-only DOM File constructors don't use the file's modification time

RESOLVED FIXED in Firefox 38



4 years ago
4 years ago


(Reporter: jld, Assigned: baku)



Firefox Tracking Flags

(firefox38 fixed)



(1 attachment, 2 obsolete attachments)

The forms of File::CreateFromFile that take an nsIFile or path will use the actual file's last-modified time for the created DOM File, as one might expect.  (This is what happens when chrome script sets the |value| property of a file input element to a non-empty string currently, but this has e10s/sandboxing problems; see bug 1068838.)

However, CreateFromFile isn't exposed to script directly, and the chrome-exposed File constructors add an extra layer of MultipartFileImpl in which the mtime gets lost and replaced with the current time.  In principle, the caller could use the |lastModified| option to the File constructor to work around this, but that's not implemented yet (bug 939509).

Comment 1

4 years ago
Created attachment 8561522 [details] [diff] [review]

Investigating this issue a bit more, it seems that it's not a big problem to support lastModified attribute in the File ChromeOnly constructors.

We cannot take the modification time of the nsIFile for security reasons: bug 403852. Maybe this is not valid anymore... but I don't want to break existing tests.
Attachment #8561522 - Flags: review?(bzbarsky)

Comment 2

4 years ago
I can add a mochitest, maybe :)
Comment on attachment 8561522 [details] [diff] [review]

> We cannot take the modification time of the nsIFile for security reasons:
> bug 403852

That bug has no security anything involved.  What's the security issue?

This patch is not removing the todo() in dom/base/test/test_bug403852.html that references this bug number.  Why not?
Attachment #8561522 - Flags: review?(bzbarsky) → review-

Comment 4

4 years ago
Created attachment 8562107 [details] [diff] [review]

Sorry bz, you are totally right. Here a new patch, with a mochitest.
Attachment #8561522 - Attachment is obsolete: true
Attachment #8562107 - Flags: review?(bzbarsky)
Comment on attachment 8562107 [details] [diff] [review]

>+      uint64_t subLastModified = blob->GetLastModified(error);

Maybe call it "partLastModified"?

>+    mLastModificationDate =
>+      lastModified ? lastModified * PR_USEC_PER_MSEC : JS_Now();

Isn't 0 a fairly valid value?  Just means a 40-year-old file....

Speaking of which, shouldn't our last-modified things be signed, so they can represent times before the epoch?  Followup ok here.

Attachment #8562107 - Flags: review?(bzbarsky) → review+

Comment 6

4 years ago
Created attachment 8562193 [details] [diff] [review]
Attachment #8562107 - Attachment is obsolete: true


4 years ago
Blocks: 1131696
Assignee: nobody → amarchesini
Keywords: checkin-needed
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.