1150001 - Cache API should not return Response body when matching Request with HEAD method

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

The spec has changed:

  https://github.com/slightlyoff/ServiceWorker/issues/655

Since HEAD does not normally return a body from the network stack, Cache should not return a body either.  Content can opt out of this behavior by setting ignoreMethod.

Implementation should be somewhat straightforward.  In the dom/cache/Manager.cpp, look for cases where FileUtils::BodyOpen() is called and skip it.

Comment 1

[Timur Valeev]

Is it needed to delete all FileUtils::BodyOpen calls (it's called 4 times in different classes)?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Timur Valeev from comment #1)
> Is it needed to delete all FileUtils::BodyOpen calls (it's called 4 times in
> different classes)?

No, we just don't want to make that call if the request used a method of "HEAD" and ignoreMethod is not set.  Most of the BodyOpen() calls have a check to see if the body ID is set first.  We can probably just incorporate the method and ignoreMethod checks in those if-statements.

The method is part of the PCacheRequest type and ignoreMethod is part of PCacheQueryParams.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Note, we will also want a test for this.  It can probably be incorporated in the existing tests in dom/cache/test/mochitest pretty easily.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Also, I should warn whoever takes this that I am in the process of renaming the PCacheRequest types, etc.  So your patches may need to be fixed up when I land that.  (Unless you think you can do this immediately and land first.)

I'm doing that work in bug 1110485.

Comment 5

[Timur Valeev]

Attachment: bug-1150001-fix-0.1.patch

I created draft patch that use approach that described in comment 2. If the patch is right I will try to add some tests to it.

Comment 6

[Timur Valeev]

Attachment: bug-1150001-fix-0.2.patch

Added check for PCacheRequest type.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8587578 [details] [diff] [review]
bug-1150001-fix-0.2.patch

Review of attachment 8587578 [details] [diff] [review]:
-----------------------------------------------------------------

This is the right idea and would work, but I'd like a slightly different style if you don't mind.

Can you make an anonymous function, like IsHeadRequest(), and then do the check with the existing if-statement in each of those clauses?

So something like:

    if (!mFoundResponse || !mResponse.mHasBodyId) {
      return rv;
    }

Would become:

    if (!mFoundResponse || !mResponse.mHasBodyId || IsHeadRequest(mrequest, mParams)) {
      return rv;
    }

You could make an IsHeadRequest() override that takes PCacheRequestOrVoid as well to make that check cleaner.

Thanks!

Comment 8

[Timur Valeev]

Attachment: bug-1150001-fix-0.3.patch

I add some improvements to the patch. Is it good?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8588506 [details] [diff] [review]
bug-1150001-fix-0.3.patch

Review of attachment 8588506 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, that was what I was asking for.  Just a few nits to fix.  Thanks!

We just need to add tests and then this should be good.

::: dom/cache/Manager.cpp
@@ +1398,5 @@
> +Manager::IsHeadRequest(PCacheRequestOrVoid aRequest, PCacheQueryParams aParams)
> +{
> +  if (aRequest.type() == PCacheRequestOrVoid::TPCacheRequest) {
> +    return aRequest.get_PCacheRequest().method().LowerCaseEqualsLiteral("head") &&
> +           !aParams.ignoreMethod();

nit: put the ignoreMethod check first to avoid wasting CPU cycles on the string comparison if the flag is set.

@@ +1400,5 @@
> +  if (aRequest.type() == PCacheRequestOrVoid::TPCacheRequest) {
> +    return aRequest.get_PCacheRequest().method().LowerCaseEqualsLiteral("head") &&
> +           !aParams.ignoreMethod();
> +  } else {
> +    return false;

nit: gecko style is to just terminate the if-clause here without an else-clause since the block returns.  Then you just do return false after the if-statement block.

::: dom/cache/Manager.h
@@ +117,5 @@
>  
>    static nsresult GetOrCreate(ManagerId* aManagerId, Manager** aManagerOut);
>    static already_AddRefed<Manager> Get(ManagerId* aManagerId);
> +  static bool IsHeadRequest(PCacheRequest aRequest, PCacheQueryParams aParams);
> +  static bool IsHeadRequest(PCacheRequestOrVoid aRequestOrVoid, PCacheQueryParams aParams);

These should be declared as private.  Alternatively, you could not declare them on Manager at all and just use an anonymous namespace in Manager.cpp.

Comment 10

[Timur Valeev]

Attachment: bug-1150001-fix-0.4.patch

Added some code improvements based on comment 9. 
Am I right that is needed to add tests to check that response doesn't have body to dom/cache/test/mochitest/test_cache_match_request.js and dom/cache/test/mochitest/test_cache_matchAll_request.js?

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8588818 [details] [diff] [review]
bug-1150001-fix-0.4.patch

Review of attachment 8588818 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/Manager.cpp
@@ +138,5 @@
> +
> +bool IsHeadRequest(PCacheRequestOrVoid aRequest, PCacheQueryParams aParams)
> +{
> +  if (aRequest.type() == PCacheRequestOrVoid::TPCacheRequest) {
> +    return !aParams.ignoreMethod() && 

nit: please remove trailing whitespace here

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Timur Valeev from comment #10)
> Am I right that is needed to add tests to check that response doesn't have
> body to dom/cache/test/mochitest/test_cache_match_request.js and
> dom/cache/test/mochitest/test_cache_matchAll_request.js?

Right, you can add cache.match() and cache.matchAll() calls with a HEAD Request and then verify the returned Response.text() resolves to empty string.  And then do this again with ignoreVary set and verify the Response.text() gives you the actual body.

Thanks!

Comment 13

[Timur Valeev]

Thank you for this information!

But I have a problem after applying my changes. When I try to run test locally:
./mach mochitest-plain dom/cache/test/mochitest/test_cache_match_request.html
Browser always crash:

#01: mozilla::dom::cache::AutoParentResponseOrVoid::Add(mozilla::dom::cache::SavedResponse const&, mozilla::dom::cache::StreamList*) (/home/valeev/mozilla-central/dom/cache/AutoUtils.cpp:454)
#02: mozilla::dom::cache::CacheParent::OnCacheMatch(unsigned long, nsresult, mozilla::dom::cache::SavedResponse const*, mozilla::dom::cache::StreamList*) (/home/valeev/mozilla-central/dom/cache/CacheParent.cpp:200)
#03: mozilla::dom::cache::Manager::CacheMatchAction::Complete(mozilla::dom::cache::Manager::Listener*, nsresult) (/home/valeev/mozilla-central/dom/cache/Manager.cpp:554)
#04: mozilla::dom::cache::Manager::BaseAction::CompleteOnInitiatingThread(nsresult) (/home/valeev/mozilla-central/dom/cache/Manager.cpp:452)
#05: mozilla::dom::cache::Context::ActionRunnable::Run() (/home/valeev/mozilla-central/dom/cache/Context.cpp:593)
#06: nsThread::ProcessNextEvent(bool, bool*) (/home/valeev/mozilla-central/xpcom/threads/nsThread.cpp:866 (discriminator 1))
#07: NS_ProcessNextEvent(nsIThread*, bool) (/home/valeev/mozilla-central/xpcom/glue/nsThreadUtils.cpp:265)
#08: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (/home/valeev/mozilla-central/ipc/glue/MessagePump.cpp:368)
#09: MessageLoop::RunInternal() (/home/valeev/mozilla-central/ipc/chromium/src/base/message_loop.cc:234)
#10: ~AutoRunState (/home/valeev/mozilla-central/ipc/chromium/src/base/message_loop.cc:517)
#11: nsThread::ThreadFunc(void*) (/home/valeev/mozilla-central/xpcom/threads/nsThread.cpp:366)
#12: _pt_root (/home/valeev/mozilla-central/nsprpub/pr/src/pthreads/ptthread.c:215)
#13: start_thread (/build/buildd/eglibc-2.19/nptl/pthread_create.c:312 (discriminator 2))
#14: __clone (/build/buildd/eglibc-2.19/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:113)
#15: ??? (???:???)

So now I'm trying to investigate why it happens.

Comment 14

[Timur Valeev]

By information from gdb output it's crashed in:
302 AutoParentBase::SerializeReadStream(const nsID& aId, StreamList* aStreamList,PCacheReadStream* aReadStreamOut)
310	  MOZ_ASSERT(stream);
Looks like "stream" is null.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Traveling today, so ni myself to look later.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Ah, I think you need to mHasBodyId to false when you do the short-circuit return in Manager.cpp.  That previously always used to be the case, but now its possible for it to be true there.  The serialization logic needs that set to false if no body is being returned.

Sorry that I missed this originally!

Comment 17

[Timur Valeev]

Attachment: bug-1150001-fix-0.5.patch

Thank you for the explanation. I create the new patch with set mHasBodyId to false. And add changed tests to it. But I'm not sure that tests are good.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Looks like the test changes are not quite right.  There you are modifying the comparison of the body for all requests made.

What we need is to do something like this:

  cache.match(new Request(url, { method: 'HEAD' })).then(function (response) {
    return response.text();
  }).then(function (text) {
    is (text, '', 'HEAD request should not provide a body.');
  });

Does that make sense?

Comment 19

[Timur Valeev]

Attachment: bug-1150001-fix-0.6.patch

But we've already have HEAD request:
c.match(new Request(request, {method: "HEAD"}));
May be better to change it?
Some like this patch...

Comment 20

[Timur Valeev]

Attachment: bug-1150001-fix-0.7.patch

Sorry. Another one.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8593928 [details] [diff] [review]
bug-1150001-fix-0.7.patch

Review of attachment 8593928 [details] [diff] [review]:
-----------------------------------------------------------------

You're right.  There was already a HEAD request in the tests.  I think your checks are not quite right yet, though.  See my comments below.

Thanks!

::: dom/cache/Manager.cpp
@@ +130,5 @@
>  namespace cache {
>  
> +namespace {
> +
> +bool IsHeadRequest(PCacheRequest aRequest, PCacheQueryParams aParams)

Please note that bug 1110485 is expected to land in mozilla-central today.  When that happens all of the types like PCacheRequest will be changed to CacheRequest.  This means you'll need to update your tree and rebase your patch.

I'm sorry for the hassle!

::: dom/cache/test/mochitest/test_cache_match_request.js
@@ +66,5 @@
> +    ok(r.text(), '', 'HEAD request should not provide a body');
> +  }).then(function() {
> +    return c.match(new Request(request, {method: "HEAD"}, {ignoreMethod: true}));
> +  }).then(function(r) {
> +    ok(r.text(), responseText, "The response body should be the same");

r.text() returns a Promise.  You need to wait for that promise to resolve before comparing to the expected string.  The checkResponse() function does this properly.

I think maybe you should add an extra argument to checkResponse() to pass the expected body.  If nothing is passed you could default to responseText.

So something like:

  function checkResponse(response, expectedBody) {
    if (expectedBody === undefined) {
      expectedBody = responseText;
    }
    // existing checkResponse code...
  }

Then you could do checkResponse(r, '') for the HEAD case.

Comment 22

[Timur Valeev]

Attachment: bug-1150001-fix-0.8.patch

I change Manager.cpp and all tests. But to be honest about the tests I'm still not sure.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8595955 [details] [diff] [review]
bug-1150001-fix-0.8.patch

Review of attachment 8595955 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking really close!  Let me know if the comments make sense.  Thanks!

After this gets r+ we will also need to do a try run.  Are you familiar with doing those?

::: dom/cache/Manager.cpp
@@ +527,5 @@
>      nsresult rv = db::CacheMatch(aConn, mCacheId, mArgs.request(),
>                                   mArgs.params(), &mFoundResponse, &mResponse);
>      if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
> +    if (!mFoundResponse || !mResponse.mHasBodyId || IsHeadRequest(mArgs.request(), mArgs.params())) {

nit: These look like they extend beyond 80 chars.  Please wrap the IsHeadRequest() to the next line.  Here and in the other cases.

::: dom/cache/test/mochitest/test_cache_match_request.js
@@ +17,5 @@
>    is(r.type, response.type, "The response types should be the same");
>    is(r.ok, response.ok, "Both responses should have succeeded");
>    is(r.statusText, response.statusText,
>       "Both responses should have the same status text");
> +  if (expectedBody == "") {

So now that you have an expectedBody variable, you don't need to explicitly check it yourself, but instead can use it in the existing is() check like:

  if (text !== expectedBody) {
    is(text, expectedBody, "The response body should be correct);
  }

@@ +18,5 @@
>    is(r.ok, response.ok, "Both responses should have succeeded");
>    is(r.statusText, response.statusText,
>       "Both responses should have the same status text");
> +  if (expectedBody == "") {
> +    ok(r.text(), '', 'HEAD request should not provide a body');

So you can lose this ok() call.  Also, note that r.text() returns a Promise to a string, not a string itself.  So this would always fail I think.

@@ +23,5 @@
> +  } else {
> +    return r.text().then(function(text) {
> +      // Avoid dumping out the large response text to the log if they're equal.
> +      if (text !== responseText) {
> +        is(text, responseText, "The response body should be correct");

And then change this to use expectedBody instead of responseText.

The comments apply to the other test file as well, of course.

@@ +69,5 @@
>      return checkResponse(r);
>    }).then(function() {
>      return c.match(new Request(request, {method: "HEAD"}));
>    }).then(function(r) {
> +    return checkResponse(r, '');

Please add a case for a HEAD request with ignoreMethod set.  Something like:

  }).then(function() {
    return c.match(new Request(request, {method: "HEAD", ignoreMethod: true}));	
  }).then(function(r) {
    return checkResponse(r);
  }).then*function() {

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #23)
> So now that you have an expectedBody variable, you don't need to explicitly
> check it yourself, but instead can use it in the existing is() check like:
> 
>   if (text !== expectedBody) {
>     is(text, expectedBody, "The response body should be correct);
>   }

Also, just for future reference, this pattern of:

  if (a !== b) {
    is(a, b, "foo");
  }

Is not normal.  We are doing this special here because the response body text can be very long.  Since is() always logs its args to the console, it was making the logs very long.  So we only use is() if its actually going to catch a failure.

Nothing for you to change here, just thought I would explain as its a bit non-obvious.

> > +    ok(r.text(), '', 'HEAD request should not provide a body');
> 
> So you can lose this ok() call.  Also, note that r.text() returns a Promise
> to a string, not a string itself.  So this would always fail I think.

Oh, this was passing because it was using ok() instead of is().  The ok() function just checks its first argument for truthiness.  The empty string second argument was being used as the failure message.  If you changed this to an is(), it would then fail all the time as expected.

Comment 25

[Timur Valeev]

Attachment: bug-1150001-fix-0.9.patch

Thank you for the explanation! I tried to fix all my errors. Here is new the new one.

Comment 26

[Timur Valeev]

And this is try build https://treeherder.mozilla.org/#/jobs?repo=try&revision=64f61622925c

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8596012 [details] [diff] [review]
bug-1150001-fix-0.9.patch

Review of attachment 8596012 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry!  I think I gave you the wrong syntax for passing ignoreMethod in my last comment.  See the comments below.

Can you cancel your last try, fix that, and try again?  Thanks for your patience!

::: dom/cache/Manager.cpp
@@ +528,5 @@
>                                   mArgs.params(), &mFoundResponse, &mResponse);
>      if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
> +    if (!mFoundResponse || !mResponse.mHasBodyId
> +        || IsHeadRequest(mArgs.request(), mArgs.params())) {

nit: The || operator goes at the end of the preceding line.

::: dom/cache/test/mochitest/test_cache_matchAll_request.js
@@ +82,5 @@
>    }).then(function(r) {
>      is(r.length, 1, "Should only find 1 item");
> +    return checkResponse(r[0], response1, response1Text, "");
> +  }).then(function() {
> +    return c.matchAll(new Request(request1, {method: "HEAD", ignoreMethod: true}));

I'm sorry.  I had a typo in my last comment.  The ignoreMethod needs to go to the matchAll() instead of new Request().  So:

  return c.matchAll(new Request(request1, { method: "HEAD" }), { ignoreMethod: true });

@@ +85,5 @@
> +  }).then(function() {
> +    return c.matchAll(new Request(request1, {method: "HEAD", ignoreMethod: true}));
> +  }).then(function(r) {
> +    is(r.length, 1, "Should only find 1 item");
> +    return checkResponse(r[0], response1, response1Text, "");

I don't think this should expect "" for the body.  This passes because I gave you the wrong ignoreMethod syntax in my previous comment.  Sorry!

::: dom/cache/test/mochitest/test_cache_match_request.js
@@ +67,5 @@
>      return c.match(new Request(request, {method: "HEAD"}));
>    }).then(function(r) {
> +    return checkResponse(r, '');
> +  }).then(function() {
> +    return c.match(new Request(request, {method: "HEAD", ignoreMethod: true}));

Same problem here.  Should be:

  return c.match(new Request(request, { method: "HEAD" }), { ignoreMethod: true });

@@ +69,5 @@
> +    return checkResponse(r, '');
> +  }).then(function() {
> +    return c.match(new Request(request, {method: "HEAD", ignoreMethod: true}));
> +  }).then(function(r) {
> +    return checkResponse(r, '');

Same issue.  I don't think this should expect an empty body.

Comment 28

[Timur Valeev]

Attachment: bug-1150001-fix-0.10.patch

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8596396 [details] [diff] [review]
bug-1150001-fix-0.10.patch

Review of attachment 8596396 [details] [diff] [review]:
-----------------------------------------------------------------

The test_cache_match_request.js looks perfect!  Just a couple more tweaks to test_cache_matchAll_request.js and we will be there.

::: dom/cache/test/mochitest/test_cache_matchAll_request.js
@@ +7,5 @@
>  var c;
>  var response1Text, response3Text;
>  var name = "matchAll-request" + context;
>  
> +function checkResponse(r, response, responseText, expectedBody) {

Oh, I did not notice that the version of checkResponse() in this file already has responseText was passed in.  I think in that case we can just lose the changes to pass expectedBody.

@@ +80,5 @@
>    }).then(function() {
>      return c.matchAll(new Request(request1, {method: "HEAD"}));
>    }).then(function(r) {
>      is(r.length, 1, "Should only find 1 item");
> +    return checkResponse(r[0], response1, response1Text, "");

Since responseText is passed in, we can just change this to:

  return checkResponse(r[0], response1, "");

@@ +85,5 @@
> +  }).then(function() {
> +    return c.matchAll(new Request(request1, {method: "HEAD"}), {ignoreMethod: true});
> +  }).then(function(r) {
> +    is(r.length, 1, "Should only find 1 item");
> +    return checkResponse(r[0], response1, response1Text, "");

And this should expect the full body, so it should be:

  return checkResponse(r[0], response1, response1Text);

::: dom/cache/test/mochitest/test_cache_match_request.js
@@ +68,5 @@
>    }).then(function(r) {
> +    return checkResponse(r, '');
> +  }).then(function() {
> +    return c.match(new Request(request, {method: "HEAD"}), {ignoreMethod: true});
> +  }).then(function(r) {

Perfect!

Comment 30

[Timur Valeev]

Attachment: bug-1150001-fix-0.11.patch

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8596557 [details] [diff] [review]
bug-1150001-fix-0.11.patch

Review of attachment 8596557 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect!  Thanks for sticking through all the review back and forth.

Can you please trigger a new try build and, if its green, add the checkin-needed keyword.

Thanks again.

Comment 32

[Timur Valeev]

Ok. Thank you a lot!

Comment 33

[Timur Valeev]

Try build https://treeherder.mozilla.org/#/jobs?repo=try&revision=8325b82880ec

Comment 34

[Timur Valeev]

Hi Ben! Is it ok?

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Looks good. Thanks!

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77a78b57afbf

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77a78b57afbf