The associated URL loader need to keep a reference to the file object when using load to file. The internal request state is torn down inside the WebCore loader as soon as the request is complete, which frees the reference to the file. This makes accessing the file after load complete fail.
Created attachment 92491 [details] Patch
Comment on attachment 92491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92491&action=review It seems slightly strange to add a reference to the object you want but then not to use that reference anywhere. Where is the code that accesses the file? Also, this patch lacks a test. Having a test is better than having a comment explaining the oddness because a test will cause us not to regress in the future. > Source/WebKit/chromium/src/AssociatedURLLoader.cpp:126 > + m_downloadedFile = File::create(response.downloadFilePath()); WebKit uses four-space indent.
lgtm! WebCore common code does not support 'downloadToFile' semantics, and these semantics are not accessible via script even in chrome. This is in support of pepper-plugins. There are tests for this, but not amongst webkit's layout test suite... http://dev.chromium.org/developers/design-documents/pepper-plugin-implementation
Created attachment 92499 [details] Indentation fixed
Created attachment 92609 [details] Different direction (not ready for checkin)
Comment on attachment 92609 [details] Different direction (not ready for checkin) View in context: https://bugs.webkit.org/attachment.cgi?id=92609&action=review Yeah, I think this approach is better. > Source/WebCore/platform/network/chromium/ResourceResponse.cpp:43 > - data->m_downloadFilePath = m_downloadFilePath; > + //data->m_downloadFilePath = m_downloadFilePath; Rather then commenting out this code, I'd put a FIXME about whether we should copy the downloaded file, ideally with a bug number.
Created attachment 92629 [details] New patch This patch adds a references to bug 60397 which I filed to track the threading issue. I talked to mnordman and we don't know how to test this in a reasonable way in the WebKit repo (there is already a test in Chrome which is how we found this problem). This functionality is really just exposed via the WebKit API, the downloaded file objects currently are not used anywhere in WebKit and are not exposed via script. The main option is to hook this up to the WebKit API unit tests. But those are literally unit tests and aren't hooked up to the network or blob systems. What a test would look like for this would be to synthesize a resource request, synthesize a response with a file, and then somehow hook into the blob system (I don't know how this works or how hard it might be) to watch for the "unref this file" message and make sure it comes later. This test seems to rather complex and of unclear benefit to me. Thoughts or other ideas?
Comment on attachment 92629 [details] New patch Having the test downstream is less good than upstream because we only find out about failures after merging in new versions of WebKit. In some sense, though, the bug here is an integration issue, which is why it's hard to test except with the fully integration.
Comment on attachment 92629 [details] New patch This is much nicer :) > Source/WebKit/chromium/src/WebURLResponse.cpp:-373 > - m_private->m_resourceResponse->setDownloadFilePath(downloadFilePath.utf8().data()); maybe ASSERT(!downloadFilePath.isEmpty())
Checked in (with Michael's assert suggestion) as r85974
This seems to have broken browser tests on all platforms - http://build.chromium.org/p/chromium/builders/Linux%20Tests%20x64/builds/8949/steps/browser_tests/logs/stdio. Also tested using try bots - http://codereview.chromium.org/6953016/. One revision before it works fine. Rolling it for out for now in order to do a webkit roll.
Going to re-land, the problem in Chrome should be fixed by http://src.chromium.org/viewvc/chrome?view=rev&revision=84797
Re-landed as r86164
(In reply to comment #13) > Re-landed as r86164 Drat i wish i would have noticed this before... void WebURLResponse::setDownloadFilePath(const WebString& downloadFilePath) { m_private->m_resourceResponse->setDownloadedFile( downloadFilePath.isEmpty() ? PassRefPtr<File>(0) : File::create(downloadFilePath)); } ... after this rolled into view, i'm seeing 2 more IPCs per request due to empty File() objects, one to 'register' upon construction and another to unregister. I think it'd be better to have NULL in there for the usual case.
Filed bug 60798 for Michael's suggestion.
Looks like the patch has been re-landed.