Closed Bug 1121722 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: jld, Assigned: baku)

References

Details

Attachments

(1 file, 2 obsolete files)

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).
Attached patch date.patch (obsolete) — Splinter 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)
I can add a mochitest, maybe :)
Comment on attachment 8561522 [details] [diff] [review]
date.patch

> 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-
Attached patch date.patch (obsolete) — Splinter 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]
date.patch

>+      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.

r=me
Attachment #8562107 - Flags: review?(bzbarsky) → review+
Attached patch date.patchSplinter Review
Attachment #8562107 - Attachment is obsolete: true
Blocks: 1131696
https://hg.mozilla.org/mozilla-central/rev/f0e720196653
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.