Add direct Response overload for FetchEvent.respondWith()

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nsm, Assigned: tvaleev, Mentored)

Tracking

33 Branch
mozilla39
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

Right now we only support Promise<Response>
Depends on: 1065216
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.
Mentor: josh
Whiteboard: [lang=c++]
(Assignee)

Comment 2

4 years ago
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)?
Flags: needinfo?(josh)
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.
Flags: needinfo?(josh)
Oh and please assign the bug to yourself if you are working on it. Thanks!
(Assignee)

Updated

4 years ago
Assignee: nobody → tvaleev
(Assignee)

Comment 5

4 years ago
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.
Flags: needinfo?(nsm.nikhil)
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++.
Flags: needinfo?(nsm.nikhil)
(Assignee)

Comment 7

4 years ago
Created attachment 8573954 [details] [diff] [review]
1136757v0.3.diff

I create draft patch. Hope that I understand correct :)
Attachment #8573954 - Flags: review?(josh)
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.
Just checking; there's no rush - are you working on adding a test, Timur?
Flags: needinfo?(tvaleev)
(Assignee)

Comment 10

4 years ago
Sure. Sorry for the delay.
Comment on attachment 8573954 [details] [diff] [review]
1136757v0.3.diff

Clearing the review until there's a test to show it works :)
Flags: needinfo?(tvaleev)
Attachment #8573954 - Flags: review?(josh) → feedback+
(Assignee)

Comment 12

4 years ago
Created attachment 8577313 [details] [diff] [review]
1136757v0.4.diff

I added some test for respondWith. Am I moving in the right direction?:)
Attachment #8573954 - Attachment is obsolete: true
Attachment #8577313 - Flags: review?(josh)
(Assignee)

Comment 13

4 years ago
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 :(.
Flags: needinfo?(josh)
Have you stepped through your new method in gdb? Are you sure it's the GetParentObject line that's failing?
Flags: needinfo?(josh)
I wonder if you should call MaybeResolve after calling the other RespondWith first?
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'.
(Assignee)

Comment 17

4 years ago
Created attachment 8577632 [details] [diff] [review]
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?
Attachment #8577313 - Attachment is obsolete: true
Attachment #8577313 - Flags: review?(josh)
Flags: needinfo?(nsm.nikhil)
Flags: needinfo?(josh)
(Assignee)

Updated

4 years ago
Attachment #8577632 - Flags: review?(josh)
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.
Flags: needinfo?(nsm.nikhil)
Flags: needinfo?(josh)
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.
Attachment #8577632 - Flags: review?(josh) → review+
(Assignee)

Comment 20

4 years ago
Created attachment 8579116 [details] [diff] [review]
1136757v0.6.patch

Updated patch.
Attachment #8577632 - Attachment is obsolete: true
Timur, thanks for the patch. It is correct. I'm just waiting for inbound to be open to land it.
(Assignee)

Comment 23

4 years ago
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? :)
Flags: needinfo?(nsm.nikhil)

Comment 24

4 years ago
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.
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.
Mentor: josh → nsm.nikhil
Flags: needinfo?(nsm.nikhil)
(Assignee)

Comment 27

4 years ago
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.
Flags: needinfo?(nsm.nikhil)
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
Flags: needinfo?(nsm.nikhil)
Just change the webidl file to use the union form and then the generated code will make it clearer.
https://hg.mozilla.org/mozilla-central/rev/0f7f9fc972cf
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 31

4 years ago
Thank you for the explanation, Nikhil! I will try to create new patch that use union in a webidl file.
You need to log in before you can comment on or make changes to this bug.