FileWriter should hold a reference to a Blob during write

• Component: DOM
• Status: RESOLVED FIXED

[Eric U.]

The current Chromium WebKit API just grabs the URL of a Blob and passes that to the writer.
If the Blob itself is garbage-collected while the write is taking place, the write will fail.
This seems like a very nonintuitive result.  For example:

function doWriter(fileWriter) {
  var bb = new BlobBuilder();
  bb.append("foo");
  var blob = bb.getBlob();
  fileWriter.write(blob);
}

If that blob gets garbage-collected very quickly, this write could fail, which I clearly the wrong behavior.

FileWriter should hold on to a reference to the blob it's writing until the write is complete, so that it sticks around regardless of the underlying implementation of AsyncFileWriter.

[Eric U.]

Created attachment 70191 [details]
Patch

[Michael Nordman]

patch looks good!

[Kinuko Yasuda]

LGTM too. Yeah we would need that.

[David Levin]

Test?

(Even if it doesn't fail all of the time. A test that becomes flaky if this break would be good. You could put a note in the test about why it might be flaky even.)

[Eric U.]

I'll be adding FileWriter layout tests tomorrow, and I'll see if I can include one that tries to tickle this a bit.  I suppose the thing to do is to do a large write while churning memory a bit.

[David Levin]

Comment on attachment 70191 [details]
Patch

Please add a test with the patch.

[Eric U.]

(In reply to comment #6)
> (From update of attachment 70191 [details])
> Please add a test with the patch.

The Chromium implementation is now complete, with the exception of this bugfix.
However, there is not yet a TestShell implementation or a WebCore/Safari implementation.  So there's nothing to run an automated LayoutTest in yet, although I can run them manually using Chromium.

How about I add a test that I can manually run, but mark it as skipped for now, and write the TestShell implementation of FileWriter next week?

[Eric U.]

Created attachment 70328 [details]
Patch

[Eric U.]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 70191 [details] [details])
> > Please add a test with the patch.
> 
> The Chromium implementation is now complete, with the exception of this bugfix.
> However, there is not yet a TestShell implementation or a WebCore/Safari implementation.  So there's nothing to run an automated LayoutTest in yet, although I can run them manually using Chromium.
> 
> How about I add a test that I can manually run, but mark it as skipped for now, and write the TestShell implementation of FileWriter next week?

Update: I've added a suppressed test.  The output was generated by running the page in Chromium, built from an unchanged source tree.  Michael Nordman has volunteered to put in the TestShell implementation of FileWriter, so we'll be able to remove the suppression shortly.

I'm working on more tests now.

[Kinuko Yasuda]

Comment on attachment 70328 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70328&action=review

(Haven't looked into the code, a comment just for suppression...)

> LayoutTests/ChangeLog:20
> +        * platform/chromium/fast/filesystem/Skipped: Added.

For chromium the suppression needs to be added in platform/chromium/test_expectations.txt.
BUGXXXXX SKIP : fast/filesystem/file-writer-gc-blob.html = FAIL

[Eric U.]

Created attachment 70332 [details]
Patch

[Eric U.]

(In reply to comment #10)
> (From update of attachment 70328 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70328&action=review
> 
> (Haven't looked into the code, a comment just for suppression...)
> 
> > LayoutTests/ChangeLog:20
> > +        * platform/chromium/fast/filesystem/Skipped: Added.
> 
> For chromium the suppression needs to be added in platform/chromium/test_expectations.txt.
> BUGXXXXX SKIP : fast/filesystem/file-writer-gc-blob.html = FAIL

Fixed, thanks.

[David Levin]

Comment on attachment 70332 [details]
Patch

I'm not doing a cq+ right now because I don't want to deal with any failures right now. Just get any committer to to cq+ it in the morning.

[WebKit Commit Bot]

Comment on attachment 70332 [details]
Patch

Rejecting patch 70332 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 70332]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=70332&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=47318&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 70332 from bug 47318.
Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'David Levin', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4347027

[Eric U.]

Created attachment 70486 [details]
Patch

[Eric U.]

Merged out the latest code; no actual changes since the R+ version.
Hopefully this will make the commit queue bot happy.

Could I get another R+/CQ+ please?

[Dumitru Daniliuc]

Comment on attachment 70486 [details]
Patch

r+'ing based on dave's r+.

[Dumitru Daniliuc]

Comment on attachment 70486 [details]
Patch

cq-'ing the patch. i'll try to submit it manually, since we want it to get in before midnight.

[Dumitru Daniliuc]

Comment on attachment 70486 [details]
Patch

Clearing flags on attachment: 70486

Committed r69560: <http://trac.webkit.org/changeset/69560>

[Dumitru Daniliuc]

All reviewed patches have been landed.  Closing bug.