Service Worker: Cache.put() consumes request/response bodies

Service Worker: Cache.put() consumes request/response bodies

Per SW spec issue:
https://github.com/slightlyoff/ServiceWorker/issues/550

Cache.put() should throw if request/response bodies
are already used, and consume request body (if non-empty)
and response body.

BUG=430962
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185323

[jsbell, 2014-11-07 20:11:55]

jsbell@chromium.org changed reviewers:
+ asanka@chromium.org, gavinp@chromium.org

[jsbell, 2014-11-07 20:11:55]

This is my interpretation of
https://github.com/slightlyoff/ServiceWorker/issues/550 - I'm pretty sure it
doesn't the subtleties in timing that @domenic might be referring to, but covers
the most common use cases, maybe?

(I'm brimming with confidence as you can tell...)

Please take a look?

[jsbell, 2014-11-07 20:25:23]

jsbell@chromium.org changed reviewers:
+ bkelly@mozilla.com

[jsbell, 2014-11-07 20:25:24]

Adding bkelly@ to reviewers, specifically for the test changes.

[bkelly, 2014-11-07 20:35:19]

https://codereview.chromium.org/708703002/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/Cache.cpp (right):

https://codereview.chromium.org/708703002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/Cache.cpp:236: response->setBodyUsed();
While it will be a somewhat rare case, it is possible to have a response without
a body.  So I think you want:

  if (response->hasBody())
    response->setBodyUsed();

https://codereview.chromium.org/708703002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/Cache.cpp:251: response->setBodyUsed();
Likewise, check hasBody() here as well.

[bkelly, 2014-11-07 20:41:22]

The tests look good to me, although I have not run them.  Our Request/Response
clone() methods are not working yet.

Thanks!

[jsbell, 2014-11-07 21:15:08]

On 2014/11/07 20:35:19, bkelly wrote:
>
https://codereview.chromium.org/708703002/diff/40001/Source/modules/servicewo...
> File Source/modules/serviceworkers/Cache.cpp (right):
> 
>
https://codereview.chromium.org/708703002/diff/40001/Source/modules/servicewo...
> Source/modules/serviceworkers/Cache.cpp:236: response->setBodyUsed();
> While it will be a somewhat rare case, it is possible to have a response
without
> a body.  So I think you want:
> 
>   if (response->hasBody())
>     response->setBodyUsed();
> 
>
https://codereview.chromium.org/708703002/diff/40001/Source/modules/servicewo...
> Source/modules/serviceworkers/Cache.cpp:251: response->setBodyUsed();
> Likewise, check hasBody() here as well.

Thanks. While looking at this I noticed that we don't support `new Response()`
and incorrectly support `new Response(null)` as a way of constructing a Response
without a body. Noted in http://crbug.com/412027. I will tackle that separately,
and do a follow up to address bkelly's comment (above) as a follow up to that CL
and this CL.

[jsbell, 2014-11-11 17:59:54]

Rebased, which left Cache::put() pretty messy. I'll see if I can clean that up a
bit.

Also, added support and test for not consuming bodyless Response, now that
crbug.com/431905 is fixed.

This does NOT do anything special for zero-length bodies, and there are no tests
for getting actual "empty" responses via fetch(); if we decide to treat all of
those as special too then SW scripts would be able to remove some clone() calls.

[jsbell, 2014-11-12 01:13:50]

Okay, bots look happy. 

gavinp@, asanka@ - PTAL?

This latest iteration should be able to be merged to M40 if we so desire.

[asanka, 2014-11-12 15:42:03]

LGTM!

[gavinp, 2014-11-13 19:35:03]

LGTM. My comments are nits, this can land with or without them addressed.

https://codereview.chromium.org/708703002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/Body.h (right):

https://codereview.chromium.org/708703002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Body.h:48: void setBodyUsed();
Should we put this after bodyUsed() so the order matches that of Body.cc? I'm a
huge fan of definition order following declaration order. Blink coding style
doesn't discuss this, although Chromium and Google C++ style is for definition
order to follow declaration order.

Better yet, why not just inline these?

https://codereview.chromium.org/708703002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/Response.cpp (right):

https://codereview.chromium.org/708703002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Response.cpp:242: 
Does Response::hasBody() go here?

(and Response::trace too?)

[jsbell, 2014-11-13 20:52:26]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-11-13 20:53:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/708703002/100001

[jsbell, 2014-11-13 21:16:05]

Thanks!

https://codereview.chromium.org/708703002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/Body.h (right):

https://codereview.chromium.org/708703002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Body.h:48: void setBodyUsed();
On 2014/11/13 19:35:02, gavinp wrote:
> Should we put this after bodyUsed() so the order matches that of Body.cc? I'm
a
> huge fan of definition order following declaration order. Blink coding style
> doesn't discuss this, although Chromium and Google C++ style is for definition
> order to follow declaration order.
> 
> Better yet, why not just inline these?

Inlined, to minimize diff.

[commit-bot: I haz the power, 2014-11-13 21:58:10]

Committed patchset #6 (id:100001) as 185323