Bug 86811 – REGRESSION: We should allow null modificationTime when snapshot metadata is given

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

[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

[Kinuko Yasuda]

Created attachment 142637 [details]
Patch

[Kinuko Yasuda]

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

[Kinuko Yasuda]

Clearing the r flag for now until we figure out what to do.

[Alexey Proskuryakov]

Please do post a rationale when making a final patch.

[Kinuko Yasuda]

Comment on attachment 142637 [details]
Patch

Per offline conversation this fix seems to work and chromeos team will need this.   Jian, could you take a look?  Thanks!

[Kinuko Yasuda]

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

[Alexey Proskuryakov]

> 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?

[Kinuko Yasuda]

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

[Alexey Proskuryakov]

Thank you for the explanation.

[Eric U.]

(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".

[Jian Li]

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

[Eric U.]

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

[Kinuko Yasuda]

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

[Kinuko Yasuda]

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?

[Kinuko Yasuda]

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.

[Kinuko Yasuda]

Created attachment 143500 [details]
Patch

[Kinuko Yasuda]

Jian, sorry for bothering you.  I uploaded a new patch-- could you take a look?

[Kinuko Yasuda]

Created attachment 143720 [details]
Patch

Sorry the issue summary was telling the opposite. Updated the ChangeLog (and issue summary)

[Jian Li]

Comment on attachment 143720 [details]
Patch

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

[Kinuko Yasuda]

Created attachment 144354 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 143720 [details]
Patch

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!

[Kinuko Yasuda]

Comment on attachment 144354 [details]
Patch

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.

[Jian Li]

Comment on attachment 144354 [details]
Patch

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.

[Kinuko Yasuda]

Committed r118907: <http://trac.webkit.org/changeset/118907>