Closed Bug 1120501 Opened 5 years ago Closed 5 years ago

Service Worker Cache should perform Add()/AddAll() fetch in child process

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

The Cache implementation of Add() and AddAll() sends the request over to the parent process and performs the fetch() there.  This avoids a lot of unnecessary copying of data to the child process.  We have to make sure FetchDriver is setup correctly, though, since we can't use Fetch() itself without a document or worker private.

What all this comes down to, is Cache will need to provide a LoadGroup or other callbacks necessary to properly receive the fetch event on these network requests.  If content code does a cache.addAll() from a window, then those requests should be intercepted for any ServiceWorker registered for that scope.

Of course, can we run the ServiceWorker in the parent process?  Do we want to?  Or would this have to kick back out to the child process?
(In reply to Ben Kelly [:bkelly] from comment #0)
> The Cache implementation of Add() and AddAll() sends the request over to the
> parent process and performs the fetch() there.  This avoids a lot of
> unnecessary copying of data to the child process.  We have to make sure
> FetchDriver is setup correctly, though, since we can't use Fetch() itself
> without a document or worker private.
> 
> What all this comes down to, is Cache will need to provide a LoadGroup or
> other callbacks necessary to properly receive the fetch event on these
> network requests.  If content code does a cache.addAll() from a window, then
> those requests should be intercepted for any ServiceWorker registered for
> that scope.
> 
> Of course, can we run the ServiceWorker in the parent process?  Do we want
> to?  Or would this have to kick back out to the child process?

So fetches initiated form Cache access from window is allowed to be intercepted by SWs? In that case, we are capable of running standard workers with content privileges from chrome code now, but it may not be so easy to do so for ServiceWorkers because registration state is maintained by ServiceWorkerManager on the child, and we can't rely on the same state to be valid by reading it out from persistent storage, we may end up using SWs that are no longer registered. If we want to move this state to the parent, that would have to be a follow up, and unless it affects ServiceWorkers-v1, I'd rather do it after we ship.
Alternatively, Cache moves the fetches for Add/AddAll back to the child process.
I think for many reasons it would just be better to do the Add/AddAll fetch in the child process for now.  Performing the fetch in the parent process is a nice optimization, but it introduces a lot of complications.

Changing this bug to represent moving the Cache FetchPut code to the child process.  It should use the Fetch() method directly without FetchDriver after doing this.
No longer depends on: 1065216
Summary: Service Worker Cache must setup fetch event interceptor correctly when using FetchDriver in parent process → Service Worker Cache should perform Add()/AddAll() fetch in child process
When implementing this we need to be sure to implement the comments on FetchPut.(h|cpp) in attachment 8539438 [details] [diff] [review].
Duplicate of this bug: 1120496
I'm going to do this one now as it will make bug 1110485 much nicer to implement.
Assignee: nobody → bkelly
Blocks: 1110485
Status: NEW → ASSIGNED
Blocks: 1148656
Blocks: 1148845
Actually, I did the refactor first.
No longer blocks: 1110485
Depends on: 1110485
Blocks: 1154673
Blocks: 1157219
Duplicate of this bug: 1150505
Blocks: 1157434
Add bug number to a TODO comment.
Attachment #8596157 - Attachment is obsolete: true
Comment on attachment 8596167 [details] [diff] [review]
P2 Move Cache Add/AddAll logic to child process. r=ehsan

Ehsan, this patch moves the logic from FetchPut.cpp into the child process.  Its mostly in Cache.cpp, but some of validation ended up in AutoUtils.cpp since it made most sense there.  I also cleaned up a few things to reflect the current spec.  For example, we throw TypeError instead of NetworkError.  Finally, I removed things like the referrer expansion that are no longer needed since we are using FetchRequest() instead of the FetchDriver directly.
Attachment #8596167 - Flags: review?(ehsan)
Comment on attachment 8596167 [details] [diff] [review]
P2 Move Cache Add/AddAll logic to child process. r=ehsan

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

Requesting additional review from smaug on the JSAPI usage in ResolvedCallback() in Cache.cpp.

::: dom/cache/AutoUtils.cpp
@@ +272,5 @@
> +  // we cannot unify them because when operating against the real database
> +  // we don't want to load all request/response objects into memory.
> +
> +  // Note, we can skip the check for a invalid request method because
> +  // Cache will only call into here with a GET or HEAD.

Can you please add a MOZ_ASSERT to that effect?  Thanks!

::: dom/cache/TypeUtils.cpp
@@ +380,5 @@
> +// static
> +void
> +TypeUtils::ProcessURL(nsAString& aUrl, bool* aSchemeValidOut,
> +                      nsAString* aUrlWithoutQueryOut, ErrorResult& aRv)
> +{

I'm assuming the body of this function has not changed.

In the future, please avoid moving code inside a file in patches to make the reviewer's job easier.  If you want the code to move around in the file for some reason, you can do that in separate patches that won't need a review.  Thanks!
Attachment #8596167 - Flags: review?(ehsan)
Attachment #8596167 - Flags: review?(bugs)
Attachment #8596167 - Flags: review+
Comment on attachment 8596167 [details] [diff] [review]
P2 Move Cache Add/AddAll logic to child process. r=ehsan


>+    JS::Rooted<JSObject*> obj(aCx, &aValue.toObject());
What guarantees aValue is actually an object?
Please check that your object is JS_IsArrayObject, assuming you want to deal with array here.
JS_IsArrayObject(cx, value) checks both that the value is object and that it is array object.
Or does something guarantee we have an array object here? If so, please add some comment.

>+    for (uint32_t i = 0; i < length; ++i) {
>+      JS::RootedValue value(aCx);
You're oddly using both JS:Rooted<JSObject*> and JS::RootedValue.
I'd prefer the latter to be JS::Rooted<JS::Value>

>+
>+      if (NS_WARN_IF(!JS_GetElement(aCx, obj, i, &value))) {
>+        Fail();
>+        return;
>+      }
>+
>+      JS::Rooted<JSObject*> responseObj(aCx, &value.toObject());
What guarantees value is an object here?

>+
>+      nsRefPtr<Response> response = nullptr;
no need to initialize nsRefPtr to null



but I don't know enough this code to say what kind of js values we're dealing with here.
(by default it feels wrong if one needs to deal with JS values manually. Can we really not just play with proper C++ objects here?)
Attachment #8596167 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #14)
> but I don't know enough this code to say what kind of js values we're
> dealing with here.
> (by default it feels wrong if one needs to deal with JS values manually. Can
> we really not just play with proper C++ objects here?)

I believe this is a consequence of the current Promise design.  This is the interface I have to work with:

  virtual void
  ResolvedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue) = 0;

There is no path for passing c++ objects through Promises as far as I can tell.
Olli, does this address your concerns?  Thanks!
Attachment #8598032 - Flags: review?(bugs)
Blocks: 1154325
Attachment #8598032 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/1418eccc0e47
https://hg.mozilla.org/mozilla-central/rev/8ffa93f4c8c5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.