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.
Created attachment 70191 [details] Patch
patch looks good!
LGTM too. Yeah we would need that.
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.)
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.
Comment on attachment 70191 [details] Patch Please add a test with the patch.
(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?
Created attachment 70328 [details] Patch
(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.
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
Created attachment 70332 [details] Patch
(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.
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.
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 commit-queue@webkit.org... 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
Created attachment 70486 [details] Patch
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?
Comment on attachment 70486 [details] Patch r+'ing based on dave's r+.
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.
Comment on attachment 70486 [details] Patch Clearing flags on attachment: 70486 Committed r69560: <http://trac.webkit.org/changeset/69560>
All reviewed patches have been landed. Closing bug.