Bug 47318

Summary: FileWriter should hold a reference to a Blob during write
Product: WebKit Reporter: Eric U. <ericu>
Component: DOMAssignee: Eric U. <ericu>
Status: RESOLVED FIXED    
Severity: Enhancement CC: commit-queue, dumi, kinuko, levin, michaeln
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 44358    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Eric U.
Reported 2010-10-06 18:25:49 PDT
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.
Attachments
Patch (2.46 KB, patch)
2010-10-07 19:37 PDT, Eric U. 		no flags
DetailsFormatted DiffDiff
Patch (7.27 KB, patch)
2010-10-08 18:17 PDT, Eric U. 		no flags
DetailsFormatted DiffDiff
Patch (7.41 KB, patch)
2010-10-08 19:07 PDT, Eric U. 		no flags
DetailsFormatted DiffDiff
Patch (7.64 KB, patch)
2010-10-11 16:02 PDT, Eric U. 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Eric U.
Comment 1 2010-10-07 19:37:06 PDT
Created attachment 70191 [details] Patch
Michael Nordman
Comment 2 2010-10-07 19:47:38 PDT
patch looks good!
Kinuko Yasuda
Comment 3 2010-10-07 19:56:46 PDT
LGTM too. Yeah we would need that.
David Levin
Comment 4 2010-10-07 22:53:43 PDT
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.
Comment 5 2010-10-07 22:57:11 PDT
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 6 2010-10-07 23:26:18 PDT
Comment on attachment 70191 [details] Patch Please add a test with the patch.
Eric U.
Comment 7 2010-10-08 17:27:19 PDT
(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.
Comment 8 2010-10-08 18:17:41 PDT
Created attachment 70328 [details] Patch
Eric U.
Comment 9 2010-10-08 18:19:34 PDT
(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 10 2010-10-08 18:49:52 PDT
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.
Comment 11 2010-10-08 19:07:21 PDT
Created attachment 70332 [details] Patch
Eric U.
Comment 12 2010-10-08 19:07:47 PDT
(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 13 2010-10-11 03:48:27 PDT
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 14 2010-10-11 11:10:21 PDT
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
Eric U.
Comment 15 2010-10-11 16:02:14 PDT
Created attachment 70486 [details] Patch
Eric U.
Comment 16 2010-10-11 16:03:07 PDT
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 17 2010-10-11 17:24:33 PDT
Comment on attachment 70486 [details] Patch r+'ing based on dave's r+.
Dumitru Daniliuc
Comment 18 2010-10-11 17:39:57 PDT
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 19 2010-10-11 21:09:00 PDT
Comment on attachment 70486 [details] Patch Clearing flags on attachment: 70486 Committed r69560: <http://trac.webkit.org/changeset/69560>
Dumitru Daniliuc
Comment 20 2010-10-11 21:09:06 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.