Add a blob field to ServiceWorkerFetchResponse and read the blob

Add a blob field to ServiceWorkerFetchResponse and read the blob

This will allow Blink-side to provide a body in its responses to
fetch events.

BUG=374123
R=michaeln@chromium.org, mmenke@chromium.org, tsepez@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273762

[michaeln, 2014-05-22 03:26:22]

https://codereview.chromium.org/293083002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_request_handler.h (right):

https://codereview.chromium.org/293083002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_request_handler.h:44:
ResourceType::Type resource_type);
BlobContext* blob_context

https://codereview.chromium.org/293083002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_request_handler.h:72: 
base::WeakPtr<BlobStorageContext> blob_context_;

https://codereview.chromium.org/293083002/diff/20001/content/browser/storage_...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/293083002/diff/20001/content/browser/storage_...
content/browser/storage_partition_impl_map.cc:418:
ServiceWorkerRequestHandler::CreateInterceptor(blob_storage_context)
A lot of plumbing to get this value from here to where its needed and adds as
coded adds an odd bit of state to this otherwise stateless class. A different
approach might eliminate some of the plumbing (maybe) and put it next to other
similar plumbing (definitely).

ServiceWorkerRequestHandler::InitializeHandler(
   p1, sw_context_wrapper,...,
   filter_->blob_storage_context());

That method is how we associate the swcontext with a requesthandler which is
very similar in nature.

[falken, 2014-05-22 11:40:59]

Thanks Michael! This patch is now ready for review.

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:193:
blob_uuid_)));
One problem is that I can change blob_uuid_ to anything and the test still
passes. This seems due to the way BlobURLRequestJob works, you pass in BlobData
and it reads that data, regardless of the UUID from the SWResponse. So I'm not
sure the functionality is fully exercised by this test.

[michaeln, 2014-05-23 02:23:22]

looks pretty good, couple questions

https://codereview.chromium.org/293083002/diff/80001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/293083002/diff/80001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:313:
webkit_blob::BlobStorageContext* GetBlobStorageContext(
might make this helper take filter_ as input since we got a helper here now

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:53:
net::URLRequestJob::Kill();
would it be good to kill the blob_request here too... as in delete it?

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:244: if
(response_type_ == FORWARD_TO_SERVICE_WORKER &&
Should this just be a function of fetchresult and status, can we get here if
response_type_ is not FTSW?

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:247:
blob_storage_context_->GetBlobDataFromUUID(response.blob_uuid);
test if (blob_storage_context_) to see if the weakptr is still good first?

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:256:
CreateResponseHeader(response);
i think we might want to defer setting http_response_info_ till just prior to
NotifyHeadersComplete time. Otherwise the various getters could be called early
and could return a valid looking but ultimiately invalid result.

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:193:
blob_uuid_)));
On 2014/05/22 11:40:59, falken wrote:
> One problem is that I can change blob_uuid_ to anything and the test still
> passes. This seems due to the way BlobURLRequestJob works, you pass in
BlobData
> and it reads that data, regardless of the UUID from the SWResponse. So I'm not
> sure the functionality is fully exercised by this test.

wha???

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:211:
SetUpWithHelper(new BlobResponder(kProcessID, blob_handle->uuid()));
oh, i see, because of how the mockprotocolhandler works.

You might be able to use the  real webkit_blob::BlobProtocolHandler which will
make real webkit_blob::BlobURLRequestJobs. The filesystemcontext and loop can be
null provided you dont request files backed by files or filesystem files.

[michaeln, 2014-05-23 02:32:31]

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:248:
blob_request_ = webkit_blob::BlobProtocolHandler::CreateBlobRequest(
This line right here setups so the webkit_blob::BlobProtocolHandler will return
the blob found via the uuid. The mockhandler knows nothing of this.

In case you're wondering, the reason we lookup early instead of just doing it
inside of blobjob.Start() is that with RDH there can be a lengthy delay between
when the request obj is created and when its started, many msg loop cycles. The
BlobDataHandle returned guarantees the blob will still be around at
blobjob.Start() time.

[falken, 2014-05-23 11:15:46]

Thanks! PTAL.

https://codereview.chromium.org/293083002/diff/80001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/293083002/diff/80001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:313:
webkit_blob::BlobStorageContext* GetBlobStorageContext(
On 2014/05/23 02:23:23, michaeln wrote:
> might make this helper take filter_ as input since we got a helper here now

yea that's a better, done

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:53:
net::URLRequestJob::Kill();
On 2014/05/23 02:23:23, michaeln wrote:
> would it be good to kill the blob_request here too... as in delete it?

Yes, that looks like the right thing to do. Done.

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:244: if
(response_type_ == FORWARD_TO_SERVICE_WORKER &&
On 2014/05/23 02:23:23, michaeln wrote:
> Should this just be a function of fetchresult and status, can we get here if
> response_type_ is not FTSW?

Good point. When we get here status is OK and fetch_result is
SERVICE_WORKER_FETCH_EVENT_RESULT_RESPONSE. I'll move this below the DCHECK to
show this.

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:247:
blob_storage_context_->GetBlobDataFromUUID(response.blob_uuid);
On 2014/05/23 02:23:23, michaeln wrote:
> test if (blob_storage_context_) to see if the weakptr is still good first?

Done.

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:248:
blob_request_ = webkit_blob::BlobProtocolHandler::CreateBlobRequest(
On 2014/05/23 02:32:32, michaeln wrote:
> This line right here setups so the webkit_blob::BlobProtocolHandler will
return
> the blob found via the uuid. The mockhandler knows nothing of this.

I see!

> 
> In case you're wondering, the reason we lookup early instead of just doing it
> inside of blobjob.Start() is that with RDH there can be a lengthy delay
between
> when the request obj is created and when its started, many msg loop cycles.
The
> BlobDataHandle returned guarantees the blob will still be around at
> blobjob.Start() time.

Ah I think I see. Thanks for explaining.

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:256:
CreateResponseHeader(response);
On 2014/05/23 02:23:23, michaeln wrote:
> i think we might want to defer setting http_response_info_ till just prior to
> NotifyHeadersComplete time. Otherwise the various getters could be called
early
> and could return a valid looking but ultimiately invalid result.

That does seem safer though it complicates the code a little. How does it look
now?

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/293083002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:211:
SetUpWithHelper(new BlobResponder(kProcessID, blob_handle->uuid()));
On 2014/05/23 02:23:23, michaeln wrote:
> oh, i see, because of how the mockprotocolhandler works.
> 
> You might be able to use the  real webkit_blob::BlobProtocolHandler which will
> make real webkit_blob::BlobURLRequestJobs. The filesystemcontext and loop can
be
> null provided you dont request files backed by files or filesystem files.

Great! I could use the real BlobProtocolHandler.

[kinuko, 2014-05-23 13:02:39]

(minor comment only, I'd like this to be reviewed by other owner (i.e.
michaeln))

https://codereview.chromium.org/293083002/diff/140001/content/browser/fileapi...
File content/browser/fileapi/blob_url_request_job_unittest.h (right):

https://codereview.chromium.org/293083002/diff/140001/content/browser/fileapi...
content/browser/fileapi/blob_url_request_job_unittest.h:14: class
MockURLRequestDelegate : public net::URLRequest::Delegate {
Should this be probably factored out in its own .h/.cc (e.g.
mock_url_request_delegate.h)? It looks a little wired to me to include another
test's _unittest.h from unittest.

[falken, 2014-05-26 12:02:58]

Thanks. Michael, could you review please?

https://codereview.chromium.org/293083002/diff/140001/content/browser/fileapi...
File content/browser/fileapi/blob_url_request_job_unittest.h (right):

https://codereview.chromium.org/293083002/diff/140001/content/browser/fileapi...
content/browser/fileapi/blob_url_request_job_unittest.h:14: class
MockURLRequestDelegate : public net::URLRequest::Delegate {
On 2014/05/23 13:02:40, kinuko wrote:
> Should this be probably factored out in its own .h/.cc (e.g.
> mock_url_request_delegate.h)? It looks a little wired to me to include another
> test's _unittest.h from unittest.

Yeah, sounds nicer. Done.

[michaeln, 2014-05-27 21:26:16]

https://codereview.chromium.org/293083002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/293083002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:248:
scoped_ptr<webkit_blob::BlobDataHandle> blob_data_handle =
if blob_data_handle is NULL here, i think we should synthesize and error
response of some kind, this is not OK. also, we have everything we're going to
have to compose the complete response (headers and body) maybe don't start the
blobrequest in this case?

might make sense to add a TODO here to put an error message on the console
(maybe one for the serviceworker's and another on requesting pages console).

https://codereview.chromium.org/293083002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/293083002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:236:
// Expect 200 OK status, as per the ServiceWorkerResponse headers, with
200 OK seems incorrect for a NonExistentBlob test case.

[falken, 2014-05-28 12:32:00]

Thanks! How does this look?

https://codereview.chromium.org/293083002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/293083002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:248:
scoped_ptr<webkit_blob::BlobDataHandle> blob_data_handle =
On 2014/05/27 21:26:16, michaeln wrote:
> if blob_data_handle is NULL here, i think we should synthesize and error
> response of some kind, this is not OK. also, we have everything we're going to
> have to compose the complete response (headers and body) maybe don't start the
> blobrequest in this case?
> 
> might make sense to add a TODO here to put an error message on the console
> (maybe one for the serviceworker's and another on requesting pages console).

Now I return 500 "Service Worker Response Error" when this happens. (It should
only happen if the renderer process sent a bad message or there was some
unexpected internal error, right?)

Might be nice to add a body too, but seems like a bit more work to make
ReadRawData read an error message.

[michaeln, 2014-05-29 01:09:03]

lgtm

https://codereview.chromium.org/293083002/diff/270001/content/browser/fileapi...
File content/browser/fileapi/mock_url_request_delegate.cc (right):

https://codereview.chromium.org/293083002/diff/270001/content/browser/fileapi...
content/browser/fileapi/mock_url_request_delegate.cc:49: if
(!request->status().is_io_pending()) {
nit: brackets not needed

https://codereview.chromium.org/293083002/diff/270001/content/browser/fileapi...
File content/browser/fileapi/mock_url_request_delegate.h (right):

https://codereview.chromium.org/293083002/diff/270001/content/browser/fileapi...
content/browser/fileapi/mock_url_request_delegate.h:32:
scoped_refptr<net::IOBuffer> received_data_;
this name is confusing, io_buffer_ maybe?

https://codereview.chromium.org/293083002/diff/270001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/293083002/diff/270001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:236:
SetUpWithHelper(new BlobResponder(kProcessID, "blob-id:nothing-is-here"));
looks like you could delete line 231 thru 235

[falken, 2014-05-29 02:10:45]

Can OWNERs please review this?
tsepez: messages/
mmenke: loader/

https://codereview.chromium.org/293083002/diff/270001/content/browser/fileapi...
File content/browser/fileapi/mock_url_request_delegate.cc (right):

https://codereview.chromium.org/293083002/diff/270001/content/browser/fileapi...
content/browser/fileapi/mock_url_request_delegate.cc:49: if
(!request->status().is_io_pending()) {
On 2014/05/29 01:09:04, michaeln wrote:
> nit: brackets not needed

Done.

https://codereview.chromium.org/293083002/diff/270001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/293083002/diff/270001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:236:
SetUpWithHelper(new BlobResponder(kProcessID, "blob-id:nothing-is-here"));
On 2014/05/29 01:09:04, michaeln wrote:
> looks like you could delete line 231 thru 235

Done.

[mmenke, 2014-05-29 15:34:49]

loader/ LGTM.

[Tom Sepez, 2014-05-29 17:29:30]

Messages LGTM.

[falken, 2014-05-29 17:31:43]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-05-29 17:35:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/falken@chromium.org/293083002/310001

[commit-bot: I haz the power, 2014-05-29 19:34:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[commit-bot: I haz the power, 2014-05-29 20:26:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-29 20:26:51]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[falken, 2014-05-30 00:22:50]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-05-30 00:24:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/falken@chromium.org/293083002/310001

[commit-bot: I haz the power, 2014-05-30 00:49:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-30 02:08:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-30 02:08:03]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[falken, 2014-05-30 02:39:30]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-05-30 02:40:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/falken@chromium.org/293083002/330001

[falken, 2014-05-30 08:12:38]

Committed patchset #15 manually as r273762 (presubmit successful).