Bug 87356 – [Chromium] File blob layout tests are failing since chromium r138702.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Vsevolod Vlasov]

The following layout tests are failing on chromium

fast/files/read-blob-async.html
fast/files/workers/worker-read-blob-async.html
fast/files/workers/worker-read-blob-sync.html

This is caused by http://src.chromium.org/viewvc/chrome?view=rev&revision=138702

[Vsevolod Vlasov]

--- /mnt/data/b/build/slave/Webkit_Linux/build/layout-test-results/fast/files/read-blob-async-expected.txt 
+++ /mnt/data/b/build/slave/Webkit_Linux/build/layout-test-results/fast/files/read-blob-async-actual.txt 
@@ -1,15 +1,21 @@
 
 Test reading a blob containing non-existent file
 readyState: 0
-Received error event
+Received loadstart event
+readyState: 1
+Received load event
 readyState: 2
-error code: 1
+result size: 0
+result: 
 Received loadend event
 Test reading a blob containing existent and non-existent file
 readyState: 0
-Received error event
+Received loadstart event
+readyState: 1
+Received load event
 readyState: 2
-error code: 1
+result size: 5
+result: Hello
 Received loadend event
 Test reading a blob containing empty file
 readyState: 0



--- /mnt/data/b/build/slave/Webkit_Linux/build/layout-test-results/fast/files/workers/worker-read-blob-async-expected.txt 
+++ /mnt/data/b/build/slave/Webkit_Linux/build/layout-test-results/fast/files/workers/worker-read-blob-async-actual.txt 
@@ -2,15 +2,21 @@
 Received files in worker
 Test reading a blob containing non-existent file
 readyState: 0
-Received error event
+Received loadstart event
+readyState: 1
+Received load event
 readyState: 2
-error code: 1
+result size: 0
+result: 
 Received loadend event
 Test reading a blob containing existent and non-existent file
 readyState: 0
-Received error event
+Received loadstart event
+readyState: 1
+Received load event
 readyState: 2
-error code: 1
+result size: 5
+result: Hello
 Received loadend event
 Test reading a blob containing empty file
 readyState: 0



--- /mnt/data/b/build/slave/Webkit_Linux/build/layout-test-results/fast/files/workers/worker-read-blob-sync-expected.txt 
+++ /mnt/data/b/build/slave/Webkit_Linux/build/layout-test-results/fast/files/workers/worker-read-blob-sync-actual.txt 
@@ -1,10 +1,12 @@
 
 Received files in worker
 Test reading a blob containing non-existent file
-Received exception 1: NOT_FOUND_ERR
+result size: 0
+result: 
 Received exception 8: NOT_FOUND_ERR
 Test reading a blob containing existent and non-existent file
-Received exception 1: NOT_FOUND_ERR
+result size: 5
+result: Hello
 Received exception 8: NOT_FOUND_ERR
 Test reading a blob containing empty file
 result size: 0

[Vsevolod Vlasov]

Skipped in: https://trac.webkit.org/changeset/118337

[Eric U.]

Due to my Chromium change, the mechanism the test uses to set up blobs containing nonexistent files no longer works [for Chromium only].  We notice that the files don't exist, treat them as zero length, and don't bother to add them to the blob.  Since this is only text fixture code, there's no real bug to fix, but we can't easily reproduce the test functionality without adding a bunch of code to DumpRenderTree.  I think the right fix here is just to add Chromium-specific expectations for these three tests.  Another alternative would be to break up the test into the pieces Chromium can handle and those it can't, and skip the latter, but that seems messier for non-Chromium ports.

[Eric U.]

Created attachment 144937 [details]
Patch

[Kinuko Yasuda]

Is the chromium behavior (silently dropping a File reference for nonexisting file and not raising NotFoundError) right interms of the spec?   Adding chrome-only exceptional behavior doesn't sound really right to me.

http://www.w3.org/TR/FileAPI/#dfn-error-codes says:
> If the File or Blob resource could not be found at the time the read was processed, then for asynchronous read methods the error attribute MUST return a "NotFoundError" DOMError and synchronous read methods MUST throw a NotFoundError exception.

[Eric U.]

(In reply to comment #5)
> Is the chromium behavior (silently dropping a File reference for nonexisting file and not raising NotFoundError) right interms of the spec?   Adding chrome-only exceptional behavior doesn't sound really right to me.
> 
> http://www.w3.org/TR/FileAPI/#dfn-error-codes says:
> > If the File or Blob resource could not be found at the time the read was processed, then for asynchronous read methods the error attribute MUST return a "NotFoundError" DOMError and synchronous read methods MUST throw a NotFoundError exception.

Sorry--I should have added more information.  There is still a problem here, that we're not raising NotFound, but this test isn't the right place for it.  The reason for that is that this test creates the blobs via a private API that's not exposed to the web, and throwing an error out of that private API isn't helpful.

You shouldn't be able to create a File that's not there--that what the private API lets you do.  If you try to read a File that's not there [say you created a valid File, then deleted the underlying file], we'll still throw NotFound at the right place.  However, if you have that File-with-no-file and add it to a Blob, we'll drop it silently, whereas the non-Chromium implementation will just add it.  Perhaps we should be throwing NotFound when you do the addition?  It's not really covered by the spec--the bit you quoted is about reads, but we're talking about Files that are invalid when used as constructor arguments.  I think it would be reasonable to throw right then.

We can't easily reenable this private API to create nonexistent files--the code gets pretty messy.  Of course it's possible, but it would be a fair amount of churn and code that wouldn't benefit non-chromium ports at all, so I'm thinking that we should skip it.  I'd been planning to log another bug suggesting that we throw in the Blob constructor, but perhaps we should just deal with it all here?

[Kinuko Yasuda]

(In reply to comment #6)
> (In reply to comment #5)
> > Is the chromium behavior (silently dropping a File reference for nonexisting file and not raising NotFoundError) right interms of the spec?   Adding chrome-only exceptional behavior doesn't sound really right to me.
> > 
> > http://www.w3.org/TR/FileAPI/#dfn-error-codes says:
> > > If the File or Blob resource could not be found at the time the read was processed, then for asynchronous read methods the error attribute MUST return a "NotFoundError" DOMError and synchronous read methods MUST throw a NotFoundError exception.
> 
> Sorry--I should have added more information.  There is still a problem here, that we're not raising NotFound, but this test isn't the right place for it.  The reason for that is that this test creates the blobs via a private API that's not exposed to the web, and throwing an error out of that private API isn't helpful.
> 
> You shouldn't be able to create a File that's not there--that what the private API lets you do.  If you try to read a File that's not there [say you created a valid File, then deleted the underlying file], we'll still throw NotFound at the right place.  However, if you have that File-with-no-file and add it to a Blob, we'll drop it silently, whereas the non-Chromium implementation will just add it.  Perhaps we should be throwing NotFound when you do the addition?  It's not really covered by the spec--the bit you quoted is about reads, but we're talking about Files that are invalid when used as constructor arguments.  I think it would be reasonable to throw right then.

I think the same problem could happen on the real Web platform, since the error seems to be happening because we are creating a new Blob with non-existent files.  We usually do not check the file's existence when we create a File object (in the current code) but do the check when we create a new Blob by combining a File or slicing a File.  That's why we're getting length=0 file blob item in the backend.  And in that case throwing an error when a read operation is initiated sounds valid to me.  (Since otherwise creating a new Blob from an existing Blob/File needs synchronous file operation, though we happen to do so in the current WebKit/chromium code)

Please correct me if I'm missing something. Thanks,

> We can't easily reenable this private API to create nonexistent files--the code gets pretty messy.  Of course it's possible, but it would be a fair amount of churn and code that wouldn't benefit non-chromium ports at all, so I'm thinking that we should skip it.  I'd been planning to log another bug suggesting that we throw in the Blob constructor, but perhaps we should just deal with it all here?

[Eric U.]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Is the chromium behavior (silently dropping a File reference for nonexisting file and not raising NotFoundError) right interms of the spec?   Adding chrome-only exceptional behavior doesn't sound really right to me.
> > > 
> > > http://www.w3.org/TR/FileAPI/#dfn-error-codes says:
> > > > If the File or Blob resource could not be found at the time the read was processed, then for asynchronous read methods the error attribute MUST return a "NotFoundError" DOMError and synchronous read methods MUST throw a NotFoundError exception.
> > 
> > Sorry--I should have added more information.  There is still a problem here, that we're not raising NotFound, but this test isn't the right place for it.  The reason for that is that this test creates the blobs via a private API that's not exposed to the web, and throwing an error out of that private API isn't helpful.
> > 
> > You shouldn't be able to create a File that's not there--that what the private API lets you do.  If you try to read a File that's not there [say you created a valid File, then deleted the underlying file], we'll still throw NotFound at the right place.  However, if you have that File-with-no-file and add it to a Blob, we'll drop it silently, whereas the non-Chromium implementation will just add it.  Perhaps we should be throwing NotFound when you do the addition?  It's not really covered by the spec--the bit you quoted is about reads, but we're talking about Files that are invalid when used as constructor arguments.  I think it would be reasonable to throw right then.
> 
> I think the same problem could happen on the real Web platform, since the error seems to be happening because we are creating a new Blob with non-existent files.  We usually do not check the file's existence when we create a File object (in the current code) but do the check when we create a new Blob by combining a File or slicing a File.

You're about to change that, though, right?  Now we're going to snapshot the File when it's created?  I thought that's where the discussion on WhatWG was going.

>  That's why we're getting length=0 file blob item in the backend.  And in that case throwing an error when a read operation is initiated sounds valid to me.  (Since otherwise creating a new Blob from an existing Blob/File needs synchronous file operation, though we happen to do so in the current WebKit/chromium code)

Once we snapshot the File at creation time, there's no need for IO at slice time, so we wouldn't find out then.  We'd either throw at creation, or get a read error, either of which seems correct to me.

Then, since we won't be doing IO at slice time, this test either would work [if we rewrite it to somehow generate the same invalid File by other means] or not [if we don't so the exception would get thrown at File creation time, invalidating the test].

> Please correct me if I'm missing something. Thanks,
> 
> > We can't easily reenable this private API to create nonexistent files--the code gets pretty messy.  Of course it's possible, but it would be a fair amount of churn and code that wouldn't benefit non-chromium ports at all, so I'm thinking that we should skip it.  I'd been planning to log another bug suggesting that we throw in the Blob constructor, but perhaps we should just deal with it all here?

[Kinuko Yasuda]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > Is the chromium behavior (silently dropping a File reference for nonexisting file and not raising NotFoundError) right interms of the spec?   Adding chrome-only exceptional behavior doesn't sound really right to me.
> > > > 
> > > > http://www.w3.org/TR/FileAPI/#dfn-error-codes says:
> > > > > If the File or Blob resource could not be found at the time the read was processed, then for asynchronous read methods the error attribute MUST return a "NotFoundError" DOMError and synchronous read methods MUST throw a NotFoundError exception.
> > > 
> > > Sorry--I should have added more information.  There is still a problem here, that we're not raising NotFound, but this test isn't the right place for it.  The reason for that is that this test creates the blobs via a private API that's not exposed to the web, and throwing an error out of that private API isn't helpful.
> > > 
> > > You shouldn't be able to create a File that's not there--that what the private API lets you do.  If you try to read a File that's not there [say you created a valid File, then deleted the underlying file], we'll still throw NotFound at the right place.  However, if you have that File-with-no-file and add it to a Blob, we'll drop it silently, whereas the non-Chromium implementation will just add it.  Perhaps we should be throwing NotFound when you do the addition?  It's not really covered by the spec--the bit you quoted is about reads, but we're talking about Files that are invalid when used as constructor arguments.  I think it would be reasonable to throw right then.
> > 
> > I think the same problem could happen on the real Web platform, since the error seems to be happening because we are creating a new Blob with non-existent files.  We usually do not check the file's existence when we create a File object (in the current code) but do the check when we create a new Blob by combining a File or slicing a File.
> 
> You're about to change that, though, right?  Now we're going to snapshot the File when it's created?  I thought that's where the discussion on WhatWG was going.

We haven't made the change yet. I just want sure that the current chromium behavior is incompatible with the spec on the real environment too (partly due to the current WebKit code).

If this sounds correct could you file a bug to track this chromium-specific behavior until we fix file snapshotting or the chromium behavior?  Then I'll be happy to land this rebaseline.

Thanks!

> >  That's why we're getting length=0 file blob item in the backend.  And in that case throwing an error when a read operation is initiated sounds valid to me.  (Since otherwise creating a new Blob from an existing Blob/File needs synchronous file operation, though we happen to do so in the current WebKit/chromium code)
> 
> Once we snapshot the File at creation time, there's no need for IO at slice time, so we wouldn't find out then.  We'd either throw at creation, or get a read error, either of which seems correct to me.
> 
> Then, since we won't be doing IO at slice time, this test either would work [if we rewrite it to somehow generate the same invalid File by other means] or not [if we don't so the exception would get thrown at File creation time, invalidating the test].
> 
> > Please correct me if I'm missing something. Thanks,
> > 
> > > We can't easily reenable this private API to create nonexistent files--the code gets pretty messy.  Of course it's possible, but it would be a fair amount of churn and code that wouldn't benefit non-chromium ports at all, so I'm thinking that we should skip it.  I'd been planning to log another bug suggesting that we throw in the Blob constructor, but perhaps we should just deal with it all here?

[Eric U.]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > (In reply to comment #5)
> > > > > Is the chromium behavior (silently dropping a File reference for nonexisting file and not raising NotFoundError) right interms of the spec?   Adding chrome-only exceptional behavior doesn't sound really right to me.
> > > > > 
> > > > > http://www.w3.org/TR/FileAPI/#dfn-error-codes says:
> > > > > > If the File or Blob resource could not be found at the time the read was processed, then for asynchronous read methods the error attribute MUST return a "NotFoundError" DOMError and synchronous read methods MUST throw a NotFoundError exception.
> > > > 
> > > > Sorry--I should have added more information.  There is still a problem here, that we're not raising NotFound, but this test isn't the right place for it.  The reason for that is that this test creates the blobs via a private API that's not exposed to the web, and throwing an error out of that private API isn't helpful.
> > > > 
> > > > You shouldn't be able to create a File that's not there--that what the private API lets you do.  If you try to read a File that's not there [say you created a valid File, then deleted the underlying file], we'll still throw NotFound at the right place.  However, if you have that File-with-no-file and add it to a Blob, we'll drop it silently, whereas the non-Chromium implementation will just add it.  Perhaps we should be throwing NotFound when you do the addition?  It's not really covered by the spec--the bit you quoted is about reads, but we're talking about Files that are invalid when used as constructor arguments.  I think it would be reasonable to throw right then.
> > > 
> > > I think the same problem could happen on the real Web platform, since the error seems to be happening because we are creating a new Blob with non-existent files.  We usually do not check the file's existence when we create a File object (in the current code) but do the check when we create a new Blob by combining a File or slicing a File.
> > 
> > You're about to change that, though, right?  Now we're going to snapshot the File when it's created?  I thought that's where the discussion on WhatWG was going.
> 
> We haven't made the change yet. I just want sure that the current chromium behavior is incompatible with the spec on the real environment too (partly due to the current WebKit code).
> 
> If this sounds correct could you file a bug to track this chromium-specific behavior until we fix file snapshotting or the chromium behavior?  Then I'll be happy to land this rebaseline.

Logged https://bugs.webkit.org/show_bug.cgi?id=89034; is that what you wanted?

> Thanks!
> 
> > >  That's why we're getting length=0 file blob item in the backend.  And in that case throwing an error when a read operation is initiated sounds valid to me.  (Since otherwise creating a new Blob from an existing Blob/File needs synchronous file operation, though we happen to do so in the current WebKit/chromium code)
> > 
> > Once we snapshot the File at creation time, there's no need for IO at slice time, so we wouldn't find out then.  We'd either throw at creation, or get a read error, either of which seems correct to me.
> > 
> > Then, since we won't be doing IO at slice time, this test either would work [if we rewrite it to somehow generate the same invalid File by other means] or not [if we don't so the exception would get thrown at File creation time, invalidating the test].
> > 
> > > Please correct me if I'm missing something. Thanks,
> > > 
> > > > We can't easily reenable this private API to create nonexistent files--the code gets pretty messy.  Of course it's possible, but it would be a fair amount of churn and code that wouldn't benefit non-chromium ports at all, so I'm thinking that we should skip it.  I'd been planning to log another bug suggesting that we throw in the Blob constructor, but perhaps we should just deal with it all here?

[Kinuko Yasuda]

Committed r120273: <http://trac.webkit.org/changeset/120273>