Bug 60281 – Associated URL loaders need to keep a reference to downloaded files

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Brett Wilson (Google)]

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.

[Brett Wilson (Google)]

Created attachment 92491 [details]
Patch

[Adam Barth]

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.

[Michael Nordman]

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

[Brett Wilson (Google)]

Created attachment 92499 [details]
Indentation fixed

[Brett Wilson (Google)]

Created attachment 92609 [details]
Different direction (not ready for checkin)

[Adam Barth]

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.

[Brett Wilson (Google)]

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?

[Adam Barth]

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.

[Michael Nordman]

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())

[Brett Wilson (Google)]

Checked in (with Michael's assert suggestion) as r85974

[Abhishek Arya]

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.

[Brett Wilson (Google)]

Going to re-land, the problem in Chrome should be fixed by
http://src.chromium.org/viewvc/chrome?view=rev&revision=84797

[Brett Wilson (Google)]

Re-landed as r86164

[Michael Nordman]

(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.

[Brett Wilson (Google)]

Filed bug 60798 for Michael's suggestion.

[Adam Barth]

Looks like the patch has been re-landed.