1136757 - Add direct Response overload for FetchEvent.respondWith()

Status: RESOLVED FIXED

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

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Right now we only support Promise<Response>

Comment 1

[Josh Matthews [:jdm]]

This requires adding an overload to FetchEvent.webidl, and adding a C++ overload to ServiceWorkerEvents.cpp that skips the Promise creation and ends up executing the same code as RespondWithHandler::ResolvedCallback currently does.

Comment 2

[Timur Valeev]

In the draft spec I found that:
4.6
FetchEvent
...
interface FetchEvent : Event {
...
void respondWith((Response or Promise<Response>) r);
};
So respondWith can have two types of arguments, Response or Promise<Response>.
 
Am I right that is needed to add support of the FetchEvent.respondWith() that have Response argument (first type)?

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

That is correct. In the gecko implementation (dom/webidl/FetchEvent.webidl), only Promise is specified. You'll want to change that. Then add a similar function in dom/workers/ServiceWorkerEvents.{h,cpp}, which will just create a Promise with the passed Response and call the existing RespondWith that accepts a Promise.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Oh and please assign the bug to yourself if you are working on it. Thanks!

Comment 5

[Timur Valeev]

Ok. I added overload RespondWith(Response& aResponse, ErrorResult& aRv) method to ServiceWorkerEvents.{h,cpp} files.
But now I have a question. How to pass aResponse to the new Promise?

I only found one solution:
nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(&aResponse);
nsRefPtr<Promise> promise = Promise::Create(global, aRv);

Is it right? I would appreciate your help.

Comment 6

[Josh Matthews [:jdm]]

You will want to resolve the promise, passing the response object as an argument. See http://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp?force=1#352 for an example of resolving promises from C++.

Comment 7

[Timur Valeev]

Attachment: 1136757v0.3.diff

I create draft patch. Hope that I understand correct :)

Comment 8

[Josh Matthews [:jdm]]

Looks sensible to me! Best way to find out is to add a test to dom/workers/tests/mochitest/serviceworkers/fetch_event_worker.js and dom/workers/tests/mochitest/serviceworkers/fetch/fetch_tests.js.

Comment 9

[Josh Matthews [:jdm]]

Just checking; there's no rush - are you working on adding a test, Timur?

Comment 10

[Timur Valeev]

Sure. Sorry for the delay.

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 8573954 [details] [diff] [review]
1136757v0.3.diff

Clearing the review until there's a test to show it works :)

Comment 12

[Timur Valeev]

Attachment: 1136757v0.4.diff

I added some test for respondWith. Am I moving in the right direction?:)

Comment 13

[Timur Valeev]

But when I tried to run the test locally by using:
./mach mochitest-plain dom/workers/test/serviceworkers/test_fetch_event.html
I got:
INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_fetch_event.html | uncaught exception - : NS_ERROR_UNEXPECTED:  at http://mochi.test:8888/tests/dom/workers/test/serviceworkers/fetch_event_worker.js:12

Looks like global object from the ServiceWorkerEvents.cpp is not created normally in
254: nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetParentObject());

I don't have ideas why it happened :(.

Comment 14

[Josh Matthews [:jdm]]

Have you stepped through your new method in gdb? Are you sure it's the GetParentObject line that's failing?

Comment 15

[Josh Matthews [:jdm]]

I wonder if you should call MaybeResolve after calling the other RespondWith first?

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8577313 [details] [diff] [review]
1136757v0.4.diff

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

The worker scope is a top level DomEventTargetHelper and has no parent object, but Event::SetOwner() tries to use it.
What you want to do is, instead of using GetParentObject(), use the currently running worker's scope like so:

WorkerPrivate* worker = GetCurrentThreadWorkerPrivate();
MOZ_ASSERT(worker);
worker->AssertIsOnWorkerThread();
...
Promise::Create(worker->GlobalScope())

::: dom/workers/test/serviceworkers/fetch/fetch_tests.js
@@ +26,5 @@
>    my_ok(xhr.responseText == "synthesized response body", "load should have synthesized response");
>    finish();
>  });
>  
> +fetch('response.txt', function(xhr) {

Nit: Please call this something more unique, like 'test-respondwith-response'.

Comment 17

[Timur Valeev]

Attachment: 1136757v0.5.diff

I create a new patch that use the currently running worker's scope that describing in comment 16.

But when I try to test it I have:
INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_fetch_event.html | uncaught exception - : InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable at http://mochi.test:8888/tests/dom/workers/test/serviceworkers/fetch_event_worker.js:12

So may be I created not correct patch?

Comment 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Timur, in your RespondWith function, you set the mWaitToRespond flag to true, which is then checked by the Promise variant leading to failure. Don't modify mWaitToRespond in the Response overload, let the Promise overload deal with it.

Comment 19

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8577632 [details] [diff] [review]
1136757v0.5.diff

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

r=me with comments addressed.
Please upload the changed patch and link to a tryserver build. No need to ask for review again.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +251,5 @@
> +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return;
> +  }
> +
> +  mWaitToRespond = true;

Remove this otherwise it won't work.

::: dom/workers/test/serviceworkers/fetch/fetch_tests.js
@@ +27,5 @@
>    finish();
>  });
>  
> +fetch('test-respondwith-response.txt', function(xhr) {
> +  my_ok(xhr.status == 200, "load should be successful");

in the test description, please prefix "test-respondwith-response.txt" so that on failures it is easy to figure out which test failed.

@@ +28,5 @@
>  });
>  
> +fetch('test-respondwith-response.txt', function(xhr) {
> +  my_ok(xhr.status == 200, "load should be successful");
> +  my_ok(xhr.responseText == "response body", "load should have response");

same here.

::: dom/workers/test/serviceworkers/fetch_event_worker.js
@@ +7,5 @@
>      ev.respondWith(p);
>    }
>  
> +  else if (ev.request.url.contains("test-respondwith-response.txt")) {
> +    var p = new Response("response body", {});

Nit: call this response instead of p.

Comment 20

[Timur Valeev]

Attachment: 1136757v0.6.patch

Updated patch.

Comment 21

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Timur, thanks for the patch. It is correct. I'm just waiting for inbound to be open to land it.

Comment 22

[Timur Valeev]

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

Comment 23

[Timur Valeev]

I created try build. And it has some warnings. But looks like they are not related to the patch. Is it ok? Can I add `checkin-needed` keyword to the bug?

And by the way... could you recommend bugs, in which I can participate? :)

Comment 24

[(no longer active)]

Comment on attachment 8579116 [details] [diff] [review]
1136757v0.6.patch

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

::: dom/webidl/FetchEvent.webidl
@@ +17,5 @@
>    readonly attribute Client? client; // The window issuing the request.
>    readonly attribute boolean isReload;
>  
>    [Throws] void respondWith(Promise<Response> r);
> +  [Throws] void respondWith(Response r);

r=me on the IDL change, provided that we fix this later to use a union.

Comment 25

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Timur, I will land it for you (right now while the tree is green) so don't bother with checkin-needed. But please file a followup that depends on this as comment 24 says, since the spec has changed while we were on this to use respondWIth(Response or Promise<Response>). And then you could work on that :) if you want. It is a fairly simple fix but lots of spelling changes due to the union change.

Comment 26

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f7f9fc972cf

Comment 27

[Timur Valeev]

Hi Nikhil! Sorry for misunderstanding...Am I right (as I understand from comment 24) that respondWIth description in the spec will change? I checked latest spec draft (https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-section) and looks it has the same.

Comment 28

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-section
It has become a union (Response or Promise<Response>) instead of the two different functions that this bug implemented. This will lead to some code changes. Specifically codegen will generate only 1 function. On that type you'll have to do something like

if (arg.IsResponse()) {
  nsRefPtr<Response> res = &arg.GetAsResponse(); (i'm just making this syntax up, it may change slightly)
  // what you had done before
} else if (arg.IsPromise()) {
  // Existing Promise handler with minor syntax changes.
}

Is that clear?

See dom/fetch/Fetch.cpp

Comment 29

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Just change the webidl file to use the union form and then the generated code will make it clearer.

Comment 30

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0f7f9fc972cf

Comment 31

[Timur Valeev]

Thank you for the explanation, Nikhil! I will try to create new patch that use union in a webidl file.