Bug 60281 - Associated URL loaders need to keep a reference to downloaded files
Summary: Associated URL loaders need to keep a reference to downloaded files
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brett Wilson (Google)
Depends on: 60444
  Show dependency treegraph
Reported: 2011-05-05 11:20 PDT by Brett Wilson (Google)
Modified: 2011-06-17 23:06 PDT (History)
3 users (show)

See Also:

Patch (2.09 KB, patch)
2011-05-05 16:10 PDT, Brett Wilson (Google)
no flags Details | Formatted Diff | Diff
Indentation fixed (2.09 KB, patch)
2011-05-05 16:48 PDT, Brett Wilson (Google)
no flags Details | Formatted Diff | Diff
Different direction (not ready for checkin) (3.42 KB, patch)
2011-05-06 11:06 PDT, Brett Wilson (Google)
no flags Details | Formatted Diff | Diff
New patch (5.49 KB, patch)
2011-05-06 12:57 PDT, Brett Wilson (Google)
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brett Wilson (Google) 2011-05-05 11:20:09 PDT
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.
Comment 1 Brett Wilson (Google) 2011-05-05 16:10:48 PDT
Created attachment 92491 [details]
Comment 2 Adam Barth 2011-05-05 16:21:40 PDT
Comment on attachment 92491 [details]

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.
Comment 3 Michael Nordman 2011-05-05 16:39:59 PDT

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...
Comment 4 Brett Wilson (Google) 2011-05-05 16:48:21 PDT
Created attachment 92499 [details]
Indentation fixed
Comment 5 Brett Wilson (Google) 2011-05-06 11:06:49 PDT
Created attachment 92609 [details]
Different direction (not ready for checkin)
Comment 6 Adam Barth 2011-05-06 11:28:16 PDT
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.
Comment 7 Brett Wilson (Google) 2011-05-06 12:57:48 PDT
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 8 Adam Barth 2011-05-06 13:02:09 PDT
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 9 Michael Nordman 2011-05-06 13:11:18 PDT
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())
Comment 10 Brett Wilson (Google) 2011-05-06 13:58:59 PDT
Checked in (with Michael's assert suggestion) as r85974
Comment 11 Abhishek Arya 2011-05-07 17:43:56 PDT
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.
Comment 12 Brett Wilson (Google) 2011-05-10 10:13:26 PDT
Going to re-land, the problem in Chrome should be fixed by
Comment 13 Brett Wilson (Google) 2011-05-10 10:26:37 PDT
Re-landed as r86164
Comment 14 Michael Nordman 2011-05-10 19:54:36 PDT
(In reply to comment #13)
> Re-landed as r86164

Drat i wish i would have noticed this before...

void WebURLResponse::setDownloadFilePath(const WebString& downloadFilePath)
        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.
Comment 15 Brett Wilson (Google) 2011-05-13 13:43:54 PDT
Filed bug 60798 for Michael's suggestion.
Comment 16 Adam Barth 2011-06-17 23:06:36 PDT
Looks like the patch has been re-landed.