Bug 86811 - REGRESSION: We should allow null modificationTime when snapshot metadata is given
: REGRESSION: We should allow null modificationTime when snapshot metadata is g...
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-05-17 22:53 PST by
Modified: 2012-05-30 02:09 PST (History)


Attachments
Patch (1.58 KB, patch)
2012-05-17 22:57 PST, Kinuko Yasuda
no flags Review Patch | Details | Formatted Diff | Diff
Patch (3.81 KB, patch)
2012-05-23 01:44 PST, Kinuko Yasuda
no flags Review Patch | Details | Formatted Diff | Diff
Patch (3.80 KB, patch)
2012-05-23 21:04 PST, Kinuko Yasuda
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.93 KB, patch)
2012-05-28 07:11 PST, Kinuko Yasuda
jianli: review+
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2012-05-17 22:53:25 PST
[REGRESSION] we should not force file modificationTime check on (non-sliced) files even if snapshot time is given

This is a regression introduced by r117432.
http://trac.webkit.org/changeset/117432
------- Comment #1 From 2012-05-17 22:57:49 PST -------
Created an attachment (id=142637) [details]
Patch
------- Comment #2 From 2012-05-17 23:11:55 PST -------
A regression is resported by chrome os folks... Jian, could you take a look at this again?

This patch is to avoid assigning the given snapshot modificationTime to BlobDataItem's expectedModificationTime, as assigning a non-null value there forces modificationTime check, and we wouldn't want to do that at least for non-sliced files.  (This change would not look truly consistent but I have no better idea for now.)
------- Comment #3 From 2012-05-18 01:15:36 PST -------
Clearing the r flag for now until we figure out what to do.
------- Comment #4 From 2012-05-18 12:58:41 PST -------
Please do post a rationale when making a final patch.
------- Comment #5 From 2012-05-18 17:40:48 PST -------
(From update of attachment 142637 [details])
Per offline conversation this fix seems to work and chromeos team will need this.   Jian, could you take a look?  Thanks!
------- Comment #6 From 2012-05-18 17:53:27 PST -------
(In reply to comment #4)
> Please do post a rationale when making a final patch.

Non-null expectedModificationTime field must be given to the BlobDataItem only when we want to make sure the file is not modified since the Blob is made, and we do that only for sliced files.
I wrongly set the value in the previous patch which caused undesirable modificationTime verification even on non-sliced files (when they are given by an external filesystem).  This patch is trying to fix the regression issue.
------- Comment #7 From 2012-05-19 08:44:32 PST -------
> only when we want to make sure the file is not modified since the Blob is made, and we do that only for sliced files.

Why are sliced files special? It appears that reader would always care when the file changed beneath it, wouldn't it?
------- Comment #8 From 2012-05-19 09:23:19 PST -------
(In reply to comment #7)
> > only when we want to make sure the file is not modified since the Blob is made, and we do that only for sliced files.
> 
> Why are sliced files special? It appears that reader would always care when the file changed beneath it, wouldn't it?

Sliced file is special, since slicing creates a new reference to a File.
The spec says the UA must throw an error if the file has been modified after "a reference for the File is created", or "a read operation is initiated".  Just creating a File object doesn't mean we should disallow any file changes on it, but once a read operation is initiated or the file is referenced (by another Blob by slicing) we need to detect file changes, as the reader would want to to make sure it is reading/referring the same one as its reference is created.
------- Comment #9 From 2012-05-19 09:34:52 PST -------
Thank you for the explanation.
------- Comment #10 From 2012-05-21 15:26:55 PST -------
(In reply to comment #8)
> (In reply to comment #7)
> > > only when we want to make sure the file is not modified since the Blob is made, and we do that only for sliced files.
> > 
> > Why are sliced files special? It appears that reader would always care when the file changed beneath it, wouldn't it?
> 
> Sliced file is special, since slicing creates a new reference to a File.
> The spec says the UA must throw an error if the file has been modified after "a reference for the File is created", or "a read operation is initiated".  Just creating a File object doesn't mean we should disallow any file changes on it, but once a read operation is initiated or the file is referenced (by another Blob by slicing) we need to detect file changes, as the reader would want to to make sure it is reading/referring the same one as its reference is created.

I interpret the spec differently, and I don't see your text written there explicitly.  Looking at the [non-normative] text there, I see: "...guarding against modifications of files on disk after a selection has taken place."  That looks to me like we should actually grab the modification time when the File is first created.

Ah, here's some normative text from the latest editor's draft: "For synchronous reads on a Web Worker [Workers], if the file has been modified on disk since the File object reference is created, user agents MUST throw a NotReadableError; for asynchronous reads, user agents must fire an error event with the error attribute returning a NotReadableError DOMError."

Where it says "the File object reference is created", that looks to me like "when we create the File", not "when we create a Blob that refers to the File".
------- Comment #11 From 2012-05-21 15:45:25 PST -------
(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > > only when we want to make sure the file is not modified since the Blob is made, and we do that only for sliced files.
> > > 
> > > Why are sliced files special? It appears that reader would always care when the file changed beneath it, wouldn't it?
> > 
> > Sliced file is special, since slicing creates a new reference to a File.
> > The spec says the UA must throw an error if the file has been modified after "a reference for the File is created", or "a read operation is initiated".  Just creating a File object doesn't mean we should disallow any file changes on it, but once a read operation is initiated or the file is referenced (by another Blob by slicing) we need to detect file changes, as the reader would want to to make sure it is reading/referring the same one as its reference is created.
> 
> I interpret the spec differently, and I don't see your text written there explicitly.  Looking at the [non-normative] text there, I see: "...guarding against modifications of files on disk after a selection has taken place."  That looks to me like we should actually grab the modification time when the File is first created.
> 
> Ah, here's some normative text from the latest editor's draft: "For synchronous reads on a Web Worker [Workers], if the file has been modified on disk since the File object reference is created, user agents MUST throw a NotReadableError; for asynchronous reads, user agents must fire an error event with the error attribute returning a NotReadableError DOMError."
> 
> Where it says "the File object reference is created", that looks to me like "when we create the File", not "when we create a Blob that refers to the File".

Yes, "Security Considerations" section does say:
  Post-selection file modifications occur when a file changes on disk after it has been selected. In such cases, user agents MAY throw a SecurityError for synchronous read methods, or return a SecurityError DOMError for asynchronous reads.

However, the spec seems to indicate that File.lastModifiedDate and Blob.size should return the latest modification time and size. It did not say that some kind of error should be returned if the file has been changed since the file was selected. Currently getting lastModifiedDate and size are both synchronous operations, in order to be consistent with the old behavior.

I am ok with changing the semantics if we think security concern does make sense. If so, we need to pass file size, modification time when files are selected.
------- Comment #12 From 2012-05-21 15:50:47 PST -------
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > > only when we want to make sure the file is not modified since the Blob is made, and we do that only for sliced files.
> > > > 
> > > > Why are sliced files special? It appears that reader would always care when the file changed beneath it, wouldn't it?
> > > 
> > > Sliced file is special, since slicing creates a new reference to a File.
> > > The spec says the UA must throw an error if the file has been modified after "a reference for the File is created", or "a read operation is initiated".  Just creating a File object doesn't mean we should disallow any file changes on it, but once a read operation is initiated or the file is referenced (by another Blob by slicing) we need to detect file changes, as the reader would want to to make sure it is reading/referring the same one as its reference is created.
> > 
> > I interpret the spec differently, and I don't see your text written there explicitly.  Looking at the [non-normative] text there, I see: "...guarding against modifications of files on disk after a selection has taken place."  That looks to me like we should actually grab the modification time when the File is first created.
> > 
> > Ah, here's some normative text from the latest editor's draft: "For synchronous reads on a Web Worker [Workers], if the file has been modified on disk since the File object reference is created, user agents MUST throw a NotReadableError; for asynchronous reads, user agents must fire an error event with the error attribute returning a NotReadableError DOMError."
> > 
> > Where it says "the File object reference is created", that looks to me like "when we create the File", not "when we create a Blob that refers to the File".
> 
> Yes, "Security Considerations" section does say:
>   Post-selection file modifications occur when a file changes on disk after it has been selected. In such cases, user agents MAY throw a SecurityError for synchronous read methods, or return a SecurityError DOMError for asynchronous reads.
> 
> However, the spec seems to indicate that File.lastModifiedDate and Blob.size should return the latest modification time and size. It did not say that some kind of error should be returned if the file has been changed since the file was selected. Currently getting lastModifiedDate and size are both synchronous operations, in order to be consistent with the old behavior.

It does say that we have the option to return null for those attributes.  I think the spec's just letting this fall through the cracks: we should throw an exception if it's changed and you try to use it, but we should also report mod time correctly if we can.  I think that's a spec bug, so I'll post to public-webapps.

> I am ok with changing the semantics if we think security concern does make sense. If so, we need to pass file size, modification time when files are selected.

I think the security concern does make sense; let's see what happens on public-webapps.
------- Comment #13 From 2012-05-21 21:22:40 PST -------
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #8)
> > > > (In reply to comment #7)
> > > > > > only when we want to make sure the file is not modified since the Blob is made, and we do that only for sliced files.
> > > > > 
> > > > > Why are sliced files special? It appears that reader would always care when the file changed beneath it, wouldn't it?
> > > > 
> > > > Sliced file is special, since slicing creates a new reference to a File.
> > > > The spec says the UA must throw an error if the file has been modified after "a reference for the File is created", or "a read operation is initiated".  Just creating a File object doesn't mean we should disallow any file changes on it, but once a read operation is initiated or the file is referenced (by another Blob by slicing) we need to detect file changes, as the reader would want to to make sure it is reading/referring the same one as its reference is created.
> > > 
> > > I interpret the spec differently, and I don't see your text written there explicitly.  Looking at the [non-normative] text there, I see: "...guarding against modifications of files on disk after a selection has taken place."  That looks to me like we should actually grab the modification time when the File is first created.
> > > 
> > > Ah, here's some normative text from the latest editor's draft: "For synchronous reads on a Web Worker [Workers], if the file has been modified on disk since the File object reference is created, user agents MUST throw a NotReadableError; for asynchronous reads, user agents must fire an error event with the error attribute returning a NotReadableError DOMError."
> > > 
> > > Where it says "the File object reference is created", that looks to me like "when we create the File", not "when we create a Blob that refers to the File".

I was initially interpreting like that, (and have once created a patch along the line since it'd simplify the implementation), but it also says "The File interface [...] represents file data that can be read into memory at the time a read operation is initiated" which makes me feel that when we capture the 'content' is not strictly combined to the object creation time.  (And if we do so we could likely break many Web apps)
The security concern makes sense, but it sounds like we should throw SecurityError rather than NotReadableError if post-selection file modification occurs.

> > Yes, "Security Considerations" section does say:
> >   Post-selection file modifications occur when a file changes on disk after it has been selected. In such cases, user agents MAY throw a SecurityError for synchronous read methods, or return a SecurityError DOMError for asynchronous reads.
> > 
> > However, the spec seems to indicate that File.lastModifiedDate and Blob.size should return the latest modification time and size. It did not say that some kind of error should be returned if the file has been changed since the file was selected. Currently getting lastModifiedDate and size are both synchronous operations, in order to be consistent with the old behavior.
> 
> It does say that we have the option to return null for those attributes.  I think the spec's just letting this fall through the cracks: we should throw an exception if it's changed and you try to use it, but we should also report mod time correctly if we can.  I think that's a spec bug, so I'll post to public-webapps.
> 
> > I am ok with changing the semantics if we think security concern does make sense. If so, we need to pass file size, modification time when files are selected.
> 
> I think the security concern does make sense; let's see what happens on public-webapps.

Yup we should make it clear on public-webapps.  Thanks for posting on public-webapps.
------- Comment #14 From 2012-05-22 21:44:02 PST -------
Looking at the discussion on public-webapps it looks whether we should treat sliced files specifically or not is a bit gray, though this patch still holds some rationale as it makes its behavior consistent to the current existing behavior.

I can either
1. land this patch for consistency (possibly with FIXME) and make horizontal changes later when we reach a clear agreement on public-webapps, or
2. go ahead and assume we should just throw exception whether a file is sliced or not, but allow embedders to pass NULL modificationTime (since it's allowed to be NULL)

Any thoughts?
------- Comment #15 From 2012-05-23 01:38:58 PST -------
I think probably we should take option 2 (because regardless of whether it is sliced or not we should allow null modificationTime as valid metadata per spec).  I'm updating the issue summary.
------- Comment #16 From 2012-05-23 01:44:49 PST -------
Created an attachment (id=143500) [details]
Patch
------- Comment #17 From 2012-05-23 01:47:05 PST -------
Jian, sorry for bothering you.  I uploaded a new patch-- could you take a look?
------- Comment #18 From 2012-05-23 21:04:18 PST -------
Created an attachment (id=143720) [details]
Patch

Sorry the issue summary was telling the opposite. Updated the ChangeLog (and issue summary)
------- Comment #19 From 2012-05-25 10:56:09 PST -------
(From update of attachment 143720 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=143720&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests as this change is not for regular files/filesystems.

Please describe the reason why we need to fix this.

> Source/WebCore/fileapi/File.cpp:133
> +    if (m_snapshotSize >= 0)

It would be better if we can introduce a helper function like isValidSnapshot so that we have one centralized place to perform this check and you can put comment there.

> Source/WebCore/fileapi/File.h:103
> +    // If valid file metadata (whose length field is non-negative) is given at construction time we use the metadata in size(), lastModifiedTime() and webkitSlice().

nit: comma before "we use the ..."

> Source/WebCore/fileapi/File.h:105
> +    // Note that we may return null lastModifiedTime if it is given (together with positive file length).

The whole comment is a bit confusing. How about rephrasing to something like:
// If m_snapShotSize is negative (initialized to -1 by default), the snapshot metadata is invalid and we retrieve the latest metadata synchronously in size(), lastModifiedTime() and webkitSlice(). Otherwise, the valid snapshot metadata can be used directly in those methods.
// Note that the valid snapshot metadata could have non-negative size and null modified time which mean that ......
------- Comment #20 From 2012-05-28 07:11:02 PST -------
Created an attachment (id=144354) [details]
Patch
------- Comment #21 From 2012-05-28 07:11:32 PST -------
(From update of attachment 143720 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=143720&action=review

>> Source/WebCore/ChangeLog:8
>> +        No new tests as this change is not for regular files/filesystems.
> 
> Please describe the reason why we need to fix this.

Done.

>> Source/WebCore/fileapi/File.cpp:133
>> +    if (m_snapshotSize >= 0)
> 
> It would be better if we can introduce a helper function like isValidSnapshot so that we have one centralized place to perform this check and you can put comment there.

Done.

>> Source/WebCore/fileapi/File.h:105
>> +    // Note that we may return null lastModifiedTime if it is given (together with positive file length).
> 
> The whole comment is a bit confusing. How about rephrasing to something like:
> // If m_snapShotSize is negative (initialized to -1 by default), the snapshot metadata is invalid and we retrieve the latest metadata synchronously in size(), lastModifiedTime() and webkitSlice(). Otherwise, the valid snapshot metadata can be used directly in those methods.
> // Note that the valid snapshot metadata could have non-negative size and null modified time which mean that ......

Updated as suggested. Thanks!
------- Comment #22 From 2012-05-29 03:19:43 PST -------
(From update of attachment 144354 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=144354&action=review

> Source/WebCore/ChangeLog:20
> +        File.idl but will fix in another patch.)

The change for this FIXME is: https://bugs.webkit.org/show_bug.cgi?id=87709
I'll remove the FIXME if the other one gets in earlier.
------- Comment #23 From 2012-05-29 09:59:04 PST -------
(From update of attachment 144354 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=144354&action=review

> Source/WebCore/ChangeLog:14
> +        2. it is valid per spec whether it indicates(http://dev.w3.org/2006/webapi/FileAPI/#dfn-lastModifiedDate says the

nit: "whether it indicates" seems to be redundant.
------- Comment #24 From 2012-05-30 02:09:05 PST -------
Committed r118907: <http://trac.webkit.org/changeset/118907>