If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 38

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jld, Assigned: baku)

Tracking

Trunk
mozilla38
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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).
(Assignee)

Comment 1

3 years ago
Created attachment 8561522 [details] [diff] [review]
date.patch

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)
(Assignee)

Comment 2

3 years ago
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-
(Assignee)

Comment 4

3 years ago
Created attachment 8562107 [details] [diff] [review]
date.patch

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+
(Assignee)

Comment 6

3 years ago
Created attachment 8562193 [details] [diff] [review]
date.patch
Attachment #8562107 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed6136ac7ec6
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Blocks: 1131696
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0e720196653
Assignee: nobody → amarchesini
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f0e720196653
Status: NEW → RESOLVED
Last Resolved: 3 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.