Closed
Bug 1120501
Opened 9 years ago
Closed 9 years ago
Service Worker Cache should perform Add()/AddAll() fetch in child process
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
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)
4.06 KB,
patch
|
nsm
:
review+
|
Details | Diff | Splinter Review |
69.98 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
4.03 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 2•9 years ago
|
||
Alternatively, Cache moves the fetches for Add/AddAll back to the child process.
Assignee | ||
Comment 3•9 years ago
|
||
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
Assignee | ||
Comment 4•9 years ago
|
||
When implementing this we need to be sure to implement the comments on FetchPut.(h|cpp) in attachment 8539438 [details] [diff] [review].
Assignee | ||
Comment 6•9 years ago
|
||
I'm going to do this one now as it will make bug 1110485 much nicer to implement.
Assignee | ||
Comment 7•9 years ago
|
||
Actually, I did the refactor first.
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8595569 -
Flags: review?(nsm.nikhil)
Attachment #8595569 -
Flags: review?(nsm.nikhil) → review+
Assignee | ||
Comment 10•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=297912f2de67
Assignee | ||
Comment 11•9 years ago
|
||
Add bug number to a TODO comment.
Attachment #8596157 -
Attachment is obsolete: true
Assignee | ||
Comment 12•9 years ago
|
||
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 13•9 years ago
|
||
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 14•9 years ago
|
||
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)
Assignee | ||
Comment 15•9 years ago
|
||
(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.
Assignee | ||
Comment 16•9 years ago
|
||
Olli, does this address your concerns? Thanks!
Attachment #8598032 -
Flags: review?(bugs)
Updated•9 years ago
|
Attachment #8598032 -
Flags: review?(bugs) → review+
Comment 17•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1418eccc0e47 https://hg.mozilla.org/integration/mozilla-inbound/rev/8ffa93f4c8c5
https://hg.mozilla.org/mozilla-central/rev/1418eccc0e47 https://hg.mozilla.org/mozilla-central/rev/8ffa93f4c8c5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•