Closed
Bug 1188545
Opened 9 years ago
Closed 9 years ago
Refactor service worker management code and implement limited lifetime for service workers
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla44
People
(Reporter: catalinb, Assigned: catalinb)
References
(Blocks 1 open bug)
Details
Attachments
(15 files, 18 obsolete files)
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
nsm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
The basic idea is to disentangle the service worker dom object from the underlying worker thread and thus have a more precise control over its lifetime.
For this purpose, code for interacting with the sw through events will be refactored into a separate class (ServiceWorkerPrivate) that will handle spinning up the thread when needed as well as shutting it down when it is idle for too long.
Assignee | ||
Comment 1•9 years ago
|
||
This is still an early version and it doesn't contain any changes regarding
shutting down the workers. But if you think I'm going with the wrong approach
please let me know.
Right now, the patch breaks fetch and I'm not sure why.
Attachment #8640026 -
Flags: feedback?(nsm.nikhil)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → catalin.badea392
Blocks: ServiceWorkers-B2G
Comment on attachment 8640026 [details] [diff] [review]
WIP: Refactor service worker events into a separate class.
Review of attachment 8640026 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/workers/ServiceWorkerPrivate.h
@@ +26,5 @@
> + //virtual void
> + //SetActivateImmediately(bool aActivateImmediately) = 0;
> +};
> +
> +// WorkerPrivate wrapper managing the lifetime of a service worker.
Please add a longer explanation than this :)
I'm only going to r+ this patch with a copious number of comments explaining the architecture, any invariants required (preferably enforceable by assertions), any synchronization being done carefully outlined and so on.
Attachment #8640026 -
Flags: feedback?(nsm.nikhil) → feedback+
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8640026 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8642499 -
Attachment is obsolete: true
Attachment #8642503 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 6•9 years ago
|
||
As a note, the lifetime rules I'm currently implementing are listed here: https://etherpad.mozilla.org/sw-lifetime
Comment on attachment 8642503 [details] [diff] [review]
Disentangle service workers from shared workers and refactor event dispatching code into a separate class.
Review of attachment 8642503 [details] [diff] [review]:
-----------------------------------------------------------------
Please r? this on khuey since it is a workers change and r? from bkelly for the loadgroup bits.
Please update the commit message to summarize the changes this patch introduces and why they are introduced.
Do use reviewboard for further patches.
::: dom/workers/RuntimeService.cpp
@@ +1675,3 @@
> mWindowMap.Enumerate(RemoveSharedWorkerFromWindowMap, aWorkerPrivate);
> }
> + else if (!aWorkerPrivate->IsServiceWorker()) {
Nit: IsDedicatedWorker().
@@ +1678,2 @@
> // May be null.
> nsPIDOMWindow* window = aWorkerPrivate->GetWindow();
It seems like this logic is the same as RemoveSharedWorkerFromWindowMap. Can we collapse the two?
::: dom/workers/ServiceWorker.cpp
@@ +97,4 @@
>
> + nsCOMPtr<nsIDocument> doc = window->GetExtantDoc();
> + MOZ_ASSERT(doc);
> + nsAutoPtr<ServiceWorkerClientInfo> clientInfo(new ServiceWorkerClientInfo(doc, doc->GetWindow()));
Use UniquePtr<> and explicitly Move() it around.
::: dom/workers/ServiceWorkerManager.cpp
@@ +472,4 @@
> {
> nsMainThreadPtrHandle<ContinueLifecycleTask> mTask;
> bool mSuccess;
> + // XXXcatalinb: Is this actually used anywhere?
This was going to be used in a former version of the spec. It can be removed now.
@@ +483,2 @@
> {
> + MOZ_ASSERT(NS_IsMainThread());
Nit: AssertIsOnMainThread();
@@ +595,5 @@
> public:
> explicit ContinueUpdateRunnable(const nsMainThreadPtrHandle<nsISupports> aJob)
> : mJob(aJob)
> {
> + MOZ_ASSERT(NS_IsMainThread());
Nit: AssertIsOnMainThread()
::: dom/workers/ServiceWorkerManager.h
@@ +428,5 @@
> WhichServiceWorker aWhichWorker,
> nsISupports** aServiceWorker);
>
> + ServiceWorkerInfo*
> + GetActiveWorkerForScope(const OriginAttributes& aOriginAttributes,
GetActiveWorkerInfo...
@@ +432,5 @@
> + GetActiveWorkerForScope(const OriginAttributes& aOriginAttributes,
> + const nsACString& aScope);
> +
> + ServiceWorkerInfo*
> + GetActiveWorkerForDocument(nsIDocument* aDocument);
Same here
::: dom/workers/ServiceWorkerPrivate.cpp
@@ +101,5 @@
> +
> + ErrorResult result;
> + result = aWorkerScope->DispatchDOMEvent(nullptr, aEvent, nullptr, nullptr);
> + if (result.Failed() || internalEvent->mFlags.mExceptionHasBeenRisen) {
> + result.SuppressException();
This should at least be logged if it is not allowed to terminate execution.
@@ +818,5 @@
> + MOZ_ASSERT_IF(aWhy == FetchEvent, aLoadGroup);
> +
> + if (mWorkerPrivate) {
> + if (aLoadGroup) {
> + mWorkerPrivate->UpdateOverridenLoadGroup(aLoadGroup);
Please have bkelly review this.
@@ +823,5 @@
> + }
> + return NS_OK;
> + }
> +
> + // TODO(catalinb): Add telemetry for service worker wake-ups.
Nit: Bug number.
@@ +872,5 @@
> +
> + // NOTE: this defaults the SW load context to:
> + // - private browsing = false
> + // - content = true
> + // - use remote tabs = false
This comment should be moved to OverrideLoadInfoLoadGroup's declaration and should be verified to check that the function does exactly what this comment says.
::: dom/workers/ServiceWorkerPrivate.h
@@ +31,5 @@
> +// To spin up the worker thread we own a |WorkerPrivate| object which can be
> +// cancelled if no events are received for a certain amount of time. Note
> +// that, at the moment, we never close the worker due to inactivity.
> +//
> +class ServiceWorkerPrivate final : public nsISupports
Could you add a comment listing the steps needed to add support for a new event type?
@@ +44,5 @@
> + const Optional<Sequence<JS::Value>>& aTransferable,
> + nsAutoPtr<ServiceWorkerClientInfo>& aClientInfo);
> +
> + nsresult
> + CheckScriptEvaluation(nsRunnable* aCallback);
Document that callback is dispatched to the main thread if and only if the worker script evaluated successfully. In fact, this should be renamed to something indicating - ContinueOnSuccessfulScriptEvaluation?
Also note that the failure case is handled by the JS exception handler, which for ServiceWorkers eventually goes to SWM::HandleError.
@@ +78,5 @@
> +
> + // This will cancel the current running worker thread and drop the
> + // workerPrivate reference.
> + // Called by ServiceWorkerInfo when [[Clear Registration]] is invoked
> + // or whenver the spec mandates that we terminate the worker.
Nit: Whenever
@@ +80,5 @@
> + // workerPrivate reference.
> + // Called by ServiceWorkerInfo when [[Clear Registration]] is invoked
> + // or whenver the spec mandates that we terminate the worker.
> + void
> + Close();
The spec uses Terminate() throughout and requires termination and not cancelation. Change the name and the implementation to use Terminate().
Attachment #8642503 -
Flags: review?(nsm.nikhil) → review+
Assignee | ||
Comment 8•9 years ago
|
||
Bug 1188545 - Disentangle service workers from shared workers and refactor event dispatching code into a separate class. r?nsm,bkelly,khuey
Attachment #8646040 -
Flags: review?(nsm.nikhil)
Attachment #8646040 -
Flags: review?(khuey)
Attachment #8646040 -
Flags: review?(bkelly)
Assignee | ||
Comment 9•9 years ago
|
||
Bug 1188545: Cosmetic changes regarding workerPrivate properties shared between shared workers and service workers. r?nsm
Attachment #8646041 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 10•9 years ago
|
||
Bug 1188545: Terminate service workers that have been idle for some time. r?nsm
Attachment #8646042 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Cătălin Badea (:catalinb) from comment #8)
> Created attachment 8646040 [details]
> MozReview Request: Bug 1188545 - Disentangle service workers from shared
> workers and refactor event dispatching code into a separate class.
> r?nsm,bkelly,khuey
>
> Bug 1188545 - Disentangle service workers from shared workers and refactor
> event dispatching code into a separate class. r?nsm,bkelly,khuey
Ben, please take a look at how the worker load group is handled in |SpawnWorkerIfNeeded|.
Kyle, please review the changes from RuntimeService and WorkerPrivate - to make sure I'm not breaking stuff.
Nikhil, could you please take another look at ServiceWorkerPrivate, the part about AllowedWindowInteraction?
Thanks
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Nikhil Marathe [:nsm] (pto Aug7-Aug10) from comment #7)
> @@ +1678,2 @@
> > // May be null.
> > nsPIDOMWindow* window = aWorkerPrivate->GetWindow();
>
> It seems like this logic is the same as RemoveSharedWorkerFromWindowMap. Can
> we collapse the two?
Addressed all comments except this part (sorry, I got nothing).
Comment on attachment 8646040 [details]
MozReview Request: Bug 1188545 - Disentangle service workers from shared workers and refactor event dispatching code into a separate class. r?nsm,bkelly,khuey
I don't review patches in reviewboard. Please attach this to bugzilla.
Attachment #8646040 -
Flags: review?(khuey) → review-
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8646414 -
Flags: review?(khuey)
Assignee | ||
Comment 15•9 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #13)
> Comment on attachment 8646040 [details]
> MozReview Request: Bug 1188545 - Disentangle service workers from shared
> workers and refactor event dispatching code into a separate class.
> r?nsm,bkelly,khuey
>
> I don't review patches in reviewboard. Please attach this to bugzilla.
It's fairly simple, just use the ship it button.
Assignee | ||
Updated•9 years ago
|
Attachment #8642503 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8642500 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8646414 -
Flags: review?(bkelly)
Comment 16•9 years ago
|
||
Comment on attachment 8646414 [details] [diff] [review]
Disentangle service workers from shared workers and refactor event dispatching code into a separate class. r?nsm,bkelly,khuey
Review of attachment 8646414 [details] [diff] [review]:
-----------------------------------------------------------------
The load group handling looks correct to me.
I made some other comments about things we should think about, though. Its unclear to me what the threading model for ServiceWorkerPrivate is. Also, its unclear to me why it needs to be an nsISupports.
::: dom/workers/ServiceWorkerManager.h
@@ +168,5 @@
> public:
> NS_INLINE_DECL_REFCOUNTING(ServiceWorkerInfo)
>
> + ServiceWorkerPrivate*
> + WorkerPrivate() const
Can this be called ServiceWorkerPrivate() to avoid confusion with the WorkerPrivate type?
::: dom/workers/ServiceWorkerPrivate.cpp
@@ +22,5 @@
> +
> +NS_IMPL_ISUPPORTS0(ServiceWorkerPrivate)
> +
> +ServiceWorkerPrivate::ServiceWorkerPrivate(ServiceWorkerInfo* aInfo)
> + : mInfo(aInfo)
What thread owns the ServiceWorkerPrivate? Please assert it here. Its not a THREADSAFE_ISUPPORTS, so that seems pretty important.
Thread asserts in other methods would be helpful as well.
@@ +983,5 @@
> + MOZ_ASSERT_IF(aWhy == FetchEvent, aLoadGroup);
> +
> + if (mWorkerPrivate) {
> + if (aLoadGroup) {
> + mWorkerPrivate->UpdateOverridenLoadGroup(aLoadGroup);
nit: Its safe to pass nullptr aLoadGroup to UpdateOverridenLoadGroup(). So you can lose the if (aLoadGroup) here.
@@ +1039,5 @@
> +
> + AutoJSAPI jsapi;
> + jsapi.Init();
> + ErrorResult error;
> + nsString scriptSpec = NS_ConvertUTF8toUTF16(mInfo->ScriptSpec());
I think you can just do:
NS_ConvertUTF8toUTF16 scriptSpec(mInfo->ScriptSpec());
::: dom/workers/ServiceWorkerPrivate.h
@@ +20,5 @@
> +class LifeCycleEventCallback : public nsRunnable
> +{
> +public:
> + virtual void
> + SetResult(bool aResult) = 0;
What thread can this be called on?
@@ +86,5 @@
> + // workerPrivate reference.
> + // Called by ServiceWorkerInfo when [[Clear Registration]] is invoked
> + // or whenever the spec mandates that we terminate the worker.
> + void
> + TerminateWorker();
Document if this is a no-op if the worker is already stopped?
@@ +111,5 @@
> + MOZ_ASSERT(!mWorkerPrivate);
> + }
> +
> + // The info object owns us - this should never be null.
> + ServiceWorkerInfo* mInfo;
What guarantees that something doesn't AddRef() the ServiceWorkerPrivate extending its life past ServiceWorkerInfo? Like a runnable or something? It seems we should have a way of explicitly clearing this pointer when the ServiceWorkerInfo destructs. We can then either gracefully handle this condition or assert that it doesn't occur.
Or can this be made a unique pointer in ServiceWorkerInfo instead of an nsISupports?
You could also avoid the circular reference between the objects by passing in the scope, scriptSpec, etc. The ServiceWorkerInfo could then call a ServiceWorkerPrivate::SetCacheName() when the cache name changes.
Attachment #8646414 -
Flags: review?(bkelly) → feedback+
Updated•9 years ago
|
Attachment #8646040 -
Flags: review?(bkelly)
Comment on attachment 8646040 [details]
MozReview Request: Bug 1188545 - Disentangle service workers from shared workers and refactor event dispatching code into a separate class. r?nsm,bkelly,khuey
https://reviewboard.mozilla.org/r/15599/#review14173
::: dom/workers/RuntimeService.cpp:285
(Diff revision 1)
> + MOZ_ASSERT(!aScriptSpec.IsEmpty());
Empty script spec doesn't seem to affect correctness of this code.
Attachment #8646040 -
Flags: review?(nsm.nikhil)
Comment on attachment 8646041 [details]
MozReview Request: Bug 1188545: Cosmetic changes regarding workerPrivate properties shared between shared workers and service workers. r?nsm
https://reviewboard.mozilla.org/r/15601/#review14181
::: dom/workers/WorkerPrivate.h:166
(Diff revision 1)
> - nsCString mSharedWorkerName;
> + nsCString mWorkerName;
Add a comment that this is the worker name for shared workers and the worker scope for service workers.
Attachment #8646041 -
Flags: review?(nsm.nikhil)
Comment on attachment 8646040 [details]
MozReview Request: Bug 1188545 - Disentangle service workers from shared workers and refactor event dispatching code into a separate class. r?nsm,bkelly,khuey
https://reviewboard.mozilla.org/r/15599/#review14183
Ship It!
Attachment #8646040 -
Flags: review+
Comment on attachment 8646042 [details]
MozReview Request: Bug 1188545: Terminate service workers that have been idle for some time. r?nsm
https://reviewboard.mozilla.org/r/15603/#review14201
General nits:
Use AssertIsOnMainThread() everywhere instead of MOZ_ASSERT(NS_IsMainThread())
Make the token the second arg of all the runnables since it is passed to the super class.
::: dom/workers/ServiceWorkerManager.cpp:2747
(Diff revision 1)
> aRegistration->Clear();
Clear() should terminate its workers.
::: dom/workers/ServiceWorkerPrivate.h:43
(Diff revision 1)
> +// promises holding tokens, we wait another |SERVICE_WORKER_IDLE_TIMEOUT|
Nit: s/promises/handlers, also it waits another WAITUNTIL timeout
::: dom/workers/ServiceWorkerPrivate.h:44
(Diff revision 1)
> +// seconds before forcefully terminating the worker.
Nit: forcibly.
::: dom/workers/ServiceWorkerPrivate.h:161
(Diff revision 1)
> + // Used to diferentiate between push service workers and normal service
Nit: differentiate.
You may have to change this based on my other comment.
::: dom/workers/ServiceWorkerPrivate.cpp:16
(Diff revision 1)
> +#define SERVICE_WORKER_IDLE_TIMEOUT 30
Nit: Keep the values in ms here.
::: dom/workers/ServiceWorkerPrivate.cpp:40
(Diff revision 1)
> + ++mPrivate->mTokenCount;
mPrivate->AddToken();?
::: dom/workers/ServiceWorkerPrivate.cpp:47
(Diff revision 1)
> + --mPrivate->mTokenCount;
mPrivate->ReleaseToken();
and then
ReleaseToken() {
assert mTokenCount > 0
--mTokenCount
if (!mTokenCount)
TerminateWorker();
}
::: dom/workers/ServiceWorkerPrivate.cpp:67
(Diff revision 1)
> +ServiceWorkerPrivate::~ServiceWorkerPrivate()
> +{
> + MOZ_ASSERT(!mWorkerPrivate);
> + MOZ_ASSERT(!mTokenCount);
> +}
The timer holds a rawptr to the SWP. It could fire after the SWP has been destroyed, so cancel the timer here.
::: dom/workers/ServiceWorkerPrivate.cpp:201
(Diff revision 1)
> + PromiseNativeHandler* aPromiseHandler)
This argument takes away from the function having a specific purpose. Have it return the Promise as before and users can append any handlers they wish too.
::: dom/workers/ServiceWorkerPrivate.cpp:255
(Diff revision 1)
> - : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount)
> + KeepAliveToken* aCancelWorker)
Nit: aCancelWorker -> aToken?
::: dom/workers/ServiceWorkerPrivate.cpp:1082
(Diff revision 1)
> if (mWorkerPrivate) {
> if (aLoadGroup) {
> mWorkerPrivate->UpdateOverridenLoadGroup(aLoadGroup);
> }
> + ResetIdleTimeout();
> +
> return NS_OK;
> }
Shouldn't the wakeup reason be updated here too?
::: dom/workers/ServiceWorkerPrivate.cpp:1179
(Diff revision 1)
> +ServiceWorkerPrivate::MaybeTerminateWorker()
> +{
main thread assertion
::: dom/workers/ServiceWorkerPrivate.cpp:1201
(Diff revision 1)
> + if (mLastWakeUpReason == PushEvent ||
> + mLastWakeUpReason == PushSubscriptionChangeEvent) {
> + return;
It is possible that
1) non-fetch event sent to worker, that promise keeps a token.
2) fetch event sent to worker, last wake up reason updated
3) document goes away, no longer controlling any.
in this case the worker will be forcibly terminated before 5min have passed.
I think you want to capture "kill immediately iff only fetch events had been sent to this worker"
::: dom/workers/ServiceWorkerPrivate.cpp:1227
(Diff revision 1)
> + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(
> + serviceWorkerPrivate->mIdleWorkerTimer->InitWithFuncCallback(
> + ServiceWorkerPrivate::TerminateWorkerCallback,
> + aPrivate,
> + SERVICE_WORKER_WAITUNTIL_TIMEOUT * 1000,
> + nsITimer::TYPE_ONE_SHOT)));
Nit: assign to nsresult to clean up indentation.
::: dom/workers/ServiceWorkerPrivate.cpp:1258
(Diff revision 1)
> + mIdleWorkerTimer->InitWithFuncCallback(ServiceWorkerPrivate::NoteIdleWorkerCallback,
Nit: assign to nsresult and then move the MOZ_ALWAYS_TRUE to its own line.
Attachment #8646042 -
Flags: review?(nsm.nikhil)
Comment on attachment 8646041 [details]
MozReview Request: Bug 1188545: Cosmetic changes regarding workerPrivate properties shared between shared workers and service workers. r?nsm
https://reviewboard.mozilla.org/r/15601/#review14219
Ship It!
Attachment #8646041 -
Flags: review+
This patch is going to need tests for the lifetime.
Assignee | ||
Updated•9 years ago
|
Attachment #8646040 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8646041 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8646042 -
Attachment is obsolete: true
Assignee | ||
Comment 23•9 years ago
|
||
Bug 1188545: Terminate service workers that have been idle for some time. r?nsm
Attachment #8647889 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 24•9 years ago
|
||
Kyle, please have a look at the RuntimeService and WorkerPrivate changes.
Attachment #8646414 -
Attachment is obsolete: true
Attachment #8646414 -
Flags: review?(khuey)
Attachment #8647890 -
Flags: review?(khuey)
Assignee | ||
Comment 25•9 years ago
|
||
Assignee | ||
Comment 26•9 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #16)
> Comment on attachment 8646414 [details] [diff] [review]
> Disentangle service workers from shared workers and refactor event
> dispatching code into a separate class. r?nsm,bkelly,khuey
>
> Review of attachment 8646414 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> The load group handling looks correct to me.
>
> I made some other comments about things we should think about, though. Its
> unclear to me what the threading model for ServiceWorkerPrivate is. Also,
> its unclear to me why it needs to be an nsISupports.
I fixed this and the other issues you raised. You're right, it doesn't need to
be nsISupports, but I need it to be refcounted in the other patches.
> What guarantees that something doesn't AddRef() the ServiceWorkerPrivate
> extending its life past ServiceWorkerInfo? Like a runnable or something?
> It seems we should have a way of explicitly clearing this pointer when the
> ServiceWorkerInfo destructs. We can then either gracefully handle this
> condition or assert that it doesn't occur.
The pointer is explictly cleared in the third patch, which introduces references to the ServiceWorkerPrivate
from worker thread.
Comment on attachment 8647889 [details]
MozReview Request: Bug 1188545 - ServiceWorkerRegistrationInfo::Clear() should terminated workers. r?nsm
https://reviewboard.mozilla.org/r/16081/#review14445
::: dom/workers/ServiceWorkerManager.cpp:2748
(Diff revision 1)
> aRegistration->Clear();
Clear() should terminate workers. Please upload that as another patch.
::: dom/workers/ServiceWorkerPrivate.h:33
(Diff revision 1)
> -// To spin up the worker thread we own a |WorkerPrivate| object which can be
> -// cancelled if no events are received for a certain amount of time. Note
> -// that, at the moment, we never close the worker due to inactivity.
> +// Lifetime management: To spin up the worker thread we own a |WorkerPrivate|
> +// object which can be cancelled if no events are received for a certain
> +// amount of time. The worker is kept alive by holding a |KeepAliveToken|
> +// reference.
> +// Extendable events hold tokens for the duration of their handler execution
> +// and until their waitUntil promise is resolved, while ServiceWorkerPrivate
> +// will hold a token for |SERVICE_WORKER_IDLE_TIMEOUT| seconds after each
> +// new event.
> +//
Could you add a note here that we do all the timer handling on the main thread since script can indefinitely block the worker thread.
::: dom/workers/ServiceWorkerPrivate.h:50
(Diff revision 1)
> // Adding an API function for a new event requires calling |SpawnWorkerIfNeeded|
calling |SpawnWorkerIfNeeded| _with an appropriate reason_
::: dom/workers/ServiceWorkerPrivate.h:167
(Diff revision 1)
> + bool mIsPushWorker : 1;
I don't think this needs to be a bitfield.
::: dom/workers/ServiceWorkerPrivate.cpp:86
(Diff revision 1)
> + // FIXME(catalinb): keep the worker alive when dispatching a message event.
Nit: Bug number
::: dom/workers/ServiceWorkerPrivate.cpp:204
(Diff revision 1)
> + Promise** aWaitUntiPromise)
Typo. Why not just already_AddRefed<Promise> return it?
::: dom/workers/ServiceWorkerPrivate.cpp:595
(Diff revision 1)
> aWorkerPrivate->GlobalScope()->ConsumeWindowInteraction();
I worry that there is a small possibility that GlobalScope() may return null if the native handler is cleaned up after the scope due to CC/GC ordering. Could you check it before calling ConsumeWindowInteraction().
::: dom/workers/ServiceWorkerPrivate.cpp:1176
(Diff revision 1)
> + mIsPushWorker = false;
It may make more sense to reset this in SpawnWorkerIfNeeded when you create a new WorkerPrivate (and update the comment at the declaration accordingly), but I'll leave it up to you.
Attachment #8647889 -
Flags: review?(nsm.nikhil) → review+
Assignee | ||
Comment 28•9 years ago
|
||
Comment on attachment 8647889 [details]
MozReview Request: Bug 1188545 - ServiceWorkerRegistrationInfo::Clear() should terminated workers. r?nsm
Bug 1188545 - ServiceWorkerRegistrationInfo::Clear() should terminated workers. r?nsm
Attachment #8647889 -
Attachment description: MozReview Request: Bug 1188545: Terminate service workers that have been idle for some time. r?nsm → MozReview Request: Bug 1188545 - ServiceWorkerRegistrationInfo::Clear() should terminated workers. r?nsm
Assignee | ||
Updated•9 years ago
|
Attachment #8647889 -
Attachment is obsolete: true
Assignee | ||
Comment 29•9 years ago
|
||
Attachment #8651270 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 30•9 years ago
|
||
Assignee | ||
Comment 31•9 years ago
|
||
Attachment #8651272 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 32•9 years ago
|
||
Attachment #8651273 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 33•9 years ago
|
||
Attachment #8651274 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 34•9 years ago
|
||
Attachment #8651275 -
Flags: review?(nsm.nikhil)
Comment on attachment 8651270 [details] [diff] [review]
ServiceWorkerRegistrationInfo::Clear() should terminated workers. r?nsm
Review of attachment 8651270 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/workers/ServiceWorkerManager.cpp
@@ +317,2 @@
> mInstallingWorker->UpdateState(ServiceWorkerState::Redundant);
> + mInstallingWorker->ServiceWorkerPrivate()->NoteDeadServiceWorkerInfo();
FWIW in the spec, termination is ordered before the call to UpdateState(), here and below.
Attachment #8651270 -
Flags: review?(nsm.nikhil) → review+
Attachment #8651272 -
Flags: review?(nsm.nikhil) → review+
Comment on attachment 8651275 [details] [diff] [review]
Reset network interceptions when the service worker is being terminated with unresolved respondWith promises. r?nsm
Review of attachment 8651275 [details] [diff] [review]:
-----------------------------------------------------------------
You could just make the AutoCancel a member of the RespondWithHandler (change it to not own nsRefPtr<RespondWithHandler>) and reduce the delta here.
::: dom/workers/test/serviceworkers/test_unresolved_fetch_interception.html
@@ +24,5 @@
> + return navigator.serviceWorker.register("unresolved_fetch_worker.js", {scope: "./"})
> + .then((swr) => ({registration: swr}));
> + }
> +
> + function waitForActiveServiceWorker(ctx) {
This does not seem to be used.
@@ +66,5 @@
> + // close worker
> + SpecialPowers.pushPrefEnv({"set": [
> + ["dom.serviceWorkers.idle_extended_timeout", 0]
> + ]}, function() {
> + navigator.serviceWorker.controller.postMessage("shutdown");
Not sure why you are sending this, is it to force reading out the new value of the pref?
@@ +85,5 @@
> +
> + SpecialPowers.pushPrefEnv({"set": [
> + ["dom.serviceWorkers.idle_timeout", 0],
> + ["dom.serviceWorkers.idle_extended_timeout", 299999],
> + ["dom.push.enabled", true],
Don't need this.
@@ +88,5 @@
> + ["dom.serviceWorkers.idle_extended_timeout", 299999],
> + ["dom.push.enabled", true],
> + ["dom.serviceWorkers.exemptFromPerDomainMax", true],
> + ["dom.serviceWorkers.enabled", true],
> + ["dom.serviceWorkers.testing.enabled", true]
I think you may want to enable "dom.serviceWorkers.interception.enabled"
Attachment #8651275 -
Flags: review?(nsm.nikhil) → review+
Attachment #8651273 -
Flags: review?(nsm.nikhil) → review+
Comment on attachment 8651274 [details] [diff] [review]
Add tests for service workers' lifetime management. r?nsm
Review of attachment 8651274 [details] [diff] [review]:
-----------------------------------------------------------------
Please make sure all dump()s in the patch have a newline at the end.
Add another patch that introduces several more tests with the timers set to non-extreme values.
::: dom/push/test/lifetime_worker.js
@@ +1,1 @@
> +var magic_value = "from_scope";
call this |state|?
@@ +2,5 @@
> +var resolvePromiseCallback;
> +
> +onfetch = function(event) {
> + if (event.request.url.indexOf("lifetime_frame.html") >= 0) {
> + event.respondWith(new Response(""));
have the response body be something indicative so it can be used for debugging. |new Response("lifetime_frame")| perhaps.
@@ +18,5 @@
> + magic_value = "update";
> + } else if (event.request.url.indexOf("wait") >= 0) {
> + event.respondWith(new Promise(function(res, rej) {
> + if (resolvePromiseCallback) {
> + error("ERROR: service worker was already waiting on a promise.");
dump(...)
@@ +21,5 @@
> + if (resolvePromiseCallback) {
> + error("ERROR: service worker was already waiting on a promise.");
> + }
> + resolvePromiseCallback = function() {
> + res(new Response(""));
Similarly descriptive here.
@@ +41,5 @@
> + // FIXME(catalinb): we cannot treat these events as extendable
> + // yet. Bug 1143717
> + event.source.postMessage({type: "message", state: magic_value});
> + magic_value = event.data;
> + if (event.data === "release") {
Factor out the block from the "release" case in onfetch and use that here so you check non-null resolvePromiseCallback.
::: dom/push/test/test_serviceworker_lifetime.html
@@ +237,5 @@
> + .then(createIframe)
> + .then(setShutdownObserver(false))
> + .then(checkStateAndUpdate(pushEvent, "from_scope", "wait"))
> + .then(cancelShutdownObserver)
> + .then(setShutdownObserver(true))
Have setShutdownObserver cancel a previous observer if any so test writers don't have to remember to call cancelShutdownObserver.
@@ +300,5 @@
> + .then(closeIframe)
> + }
> + }
> +
> + // ++catalinb on #content.
?
@@ +319,5 @@
> + SpecialPowers.pushPrefEnv({"set": [
> + ["dom.push.enabled", true],
> + ["dom.serviceWorkers.exemptFromPerDomainMax", true],
> + ["dom.serviceWorkers.enabled", true],
> + ["dom.serviceWorkers.testing.enabled", true]
dom.network.interception.enabled.
::: modules/libpref/init/all.js
@@ +146,5 @@
>
> // Allow service workers to intercept opaque (cross origin) responses
> pref("dom.serviceWorkers.interception.opaque.enabled", false);
>
> +// The amount of time service workers keep running after each event.
Please add units in both.
Attachment #8651274 -
Flags: review?(nsm.nikhil) → review+
Assignee | ||
Comment 38•9 years ago
|
||
Attachment #8647890 -
Attachment is obsolete: true
Attachment #8647890 -
Flags: review?(khuey)
Attachment #8656907 -
Flags: review?(khuey)
Assignee | ||
Comment 39•9 years ago
|
||
Attachment #8651270 -
Attachment is obsolete: true
Assignee | ||
Comment 40•9 years ago
|
||
Attachment #8647891 -
Attachment is obsolete: true
Assignee | ||
Comment 41•9 years ago
|
||
Attachment #8651271 -
Attachment is obsolete: true
Assignee | ||
Comment 42•9 years ago
|
||
Attachment #8651272 -
Attachment is obsolete: true
Assignee | ||
Comment 43•9 years ago
|
||
Attachment #8651273 -
Attachment is obsolete: true
Assignee | ||
Comment 44•9 years ago
|
||
Attachment #8651274 -
Attachment is obsolete: true
Assignee | ||
Comment 45•9 years ago
|
||
Attachment #8651275 -
Attachment is obsolete: true
Assignee | ||
Comment 46•9 years ago
|
||
Assignee | ||
Comment 47•9 years ago
|
||
Assignee | ||
Comment 48•9 years ago
|
||
Assignee | ||
Comment 49•9 years ago
|
||
Assignee | ||
Comment 50•9 years ago
|
||
Attachment #8656921 -
Flags: review?(nsm.nikhil)
Attachment #8656921 -
Flags: review?(nsm.nikhil) → review+
Assignee | ||
Comment 51•9 years ago
|
||
Comment on attachment 8656907 [details] [diff] [review]
Disentangle service workers from shared workers and refactor event dispatching code into a separate class.
With this patch, creating a SW doesn't require creating a shared worker, the service worker manager owns the WorkerPrivate instead. In a later patch, the service worker can be terminated if it idles for some time.
Jonas, could you please have a look at this? I'm mainly interested in confirming that the WorkerPrivate/RuntimeService changes are correct and that ServiceWorkerPrivate::SpawnServiceWorkerIfNeeded properly initializes the WorkerPrivate.
Thanks!
Attachment #8656907 -
Flags: review?(khuey) → review?(jonas)
Catalin, this can't land with the current system principal storage workaround since that is just checking for tests being enabled. You'll want to check against the registration principal perhaps?
Blocks: 1203274
Comment on attachment 8656907 [details] [diff] [review]
Disentangle service workers from shared workers and refactor event dispatching code into a separate class.
Review of attachment 8656907 [details] [diff] [review]:
-----------------------------------------------------------------
I don't feel that I know this code well enough to review. Try khuey maybe?
Attachment #8656907 -
Flags: review?(jonas) → review?(khuey)
Assignee | ||
Updated•9 years ago
|
Attachment #8656907 -
Flags: review?(khuey) → review?(mrbkap)
Assignee | ||
Comment 54•9 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #53)
> Comment on attachment 8656907 [details] [diff] [review]
> Disentangle service workers from shared workers and refactor event
> dispatching code into a separate class.
>
> Review of attachment 8656907 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I don't feel that I know this code well enough to review. Try khuey maybe?
I think khuey is in taipei and won't review it for at least another week.
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 55•9 years ago
|
||
Comment on attachment 8656907 [details] [diff] [review]
Disentangle service workers from shared workers and refactor event dispatching code into a separate class.
Review of attachment 8656907 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +98,2 @@
> */
> + [noscript] nsISupports GetDocumentController(in nsIDOMWindow aWindow);
You need to change the IID for this interface.
::: dom/workers/ServiceWorkerPrivate.h
@@ +21,5 @@
> +{
> +public:
> + virtual void
> + // Called on the worker thread.
> + SetResult(bool aResult) = 0;
Uber-nit: it's weird to separate the return value from the function name with a comment. I'd put it on top of the return value.
Attachment #8656907 -
Flags: review?(mrbkap) → review+
\o/ Please land if all tests are green :)
Comment 58•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e94f12b0bcf3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5d05def5b17
https://hg.mozilla.org/integration/mozilla-inbound/rev/4be675dc1b37
https://hg.mozilla.org/integration/mozilla-inbound/rev/032f4c24fc34
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fa2f55727f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a28cb794b23
https://hg.mozilla.org/integration/mozilla-inbound/rev/4473e036b52e
https://hg.mozilla.org/integration/mozilla-inbound/rev/76eb7ffeca2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b6bdf859845
https://hg.mozilla.org/integration/mozilla-inbound/rev/11ff29cc25d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1989893b59de
https://hg.mozilla.org/integration/mozilla-inbound/rev/e04738ee72a3
Assignee | ||
Comment 59•9 years ago
|
||
Backed out for android m12 failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/f39b2e477fbe
https://treeherder.mozilla.org/logviewer.html#?job_id=14986157&repo=mozilla-inbound
Flags: needinfo?(catalin.badea392)
Assignee | ||
Comment 61•9 years ago
|
||
Attachment #8656916 -
Attachment is obsolete: true
Comment 62•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad5a53a875c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9247b2db5721
https://hg.mozilla.org/integration/mozilla-inbound/rev/19efef8453a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/668bc98e20be
https://hg.mozilla.org/integration/mozilla-inbound/rev/55766c348961
https://hg.mozilla.org/integration/mozilla-inbound/rev/97f964c093f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/f33b94206b39
https://hg.mozilla.org/integration/mozilla-inbound/rev/9017fd99327c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b36dd249297
https://hg.mozilla.org/integration/mozilla-inbound/rev/be8ed23a47e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/26644018d831
https://hg.mozilla.org/integration/mozilla-inbound/rev/bec44d498254
Assignee | ||
Comment 63•9 years ago
|
||
(In reply to Wes Kocher (:KWierso) from comment #60)
> Backed out for android m12 failures:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f39b2e477fbe
> https://treeherder.mozilla.org/logviewer.html#?job_id=14986157&repo=mozilla-
> inbound
I've increased the timeout limit for test_fetch_cors.html. The two failing tests take really long to run, so I guess bumped them over the limit on android debug. Running the tests locally didn't reveal any difference between with and without these patches. Please open a bug if test_fetch_cors_sw_reroute.html is still timing out. Thanks!
Flags: needinfo?(catalin.badea392)
Assignee | ||
Comment 64•9 years ago
|
||
Comment 65•9 years ago
|
||
Comment 66•9 years ago
|
||
Assignee | ||
Comment 67•9 years ago
|
||
Looks like test_serviceworker_lifetime.html has a race condition during the final steps of the test.
Going to try and fix this.
https://treeherder.mozilla.org/logviewer.html#?job_id=15010818&repo=mozilla-inbound
Assignee | ||
Comment 68•9 years ago
|
||
Comment 69•9 years ago
|
||
Comment 70•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1ad5a53a875c
https://hg.mozilla.org/mozilla-central/rev/9247b2db5721
https://hg.mozilla.org/mozilla-central/rev/19efef8453a7
https://hg.mozilla.org/mozilla-central/rev/668bc98e20be
https://hg.mozilla.org/mozilla-central/rev/55766c348961
https://hg.mozilla.org/mozilla-central/rev/97f964c093f4
https://hg.mozilla.org/mozilla-central/rev/f33b94206b39
https://hg.mozilla.org/mozilla-central/rev/9017fd99327c
https://hg.mozilla.org/mozilla-central/rev/9b36dd249297
https://hg.mozilla.org/mozilla-central/rev/be8ed23a47e3
https://hg.mozilla.org/mozilla-central/rev/26644018d831
https://hg.mozilla.org/mozilla-central/rev/bec44d498254
https://hg.mozilla.org/mozilla-central/rev/43f8f5ad2383
https://hg.mozilla.org/mozilla-central/rev/19e843a694ed
https://hg.mozilla.org/mozilla-central/rev/f531a0565a17
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment 71•9 years ago
|
||
Catalin: reminder to nominate this for Aurora when you get a chance. Thanks!
Flags: needinfo?(catalin.badea392)
Comment 72•9 years ago
|
||
(In reply to Ehsan Akhgari (don't ask for review please) from comment #71)
> Catalin: reminder to nominate this for Aurora when you get a chance. Thanks!
Ping?
Comment 73•9 years ago
|
||
ni Liz to follow up on this bug that I was pinged about uplifting.
Flags: needinfo?(lhenry)
Comment 74•9 years ago
|
||
Oh, I think we decided at this morning's meeting to wait to request uplift on this bug. Sorry for the confusion.
Flags: needinfo?(lhenry)
Flags: needinfo?(catalin.badea392)
Comment 76•9 years ago
|
||
These changes drastically reduced the run time of service worker mochitests on Android Debug. Those tests are still running slowly on aurora causing regular timeouts on aurora in Android 4.3 Debug mochitest-13 - bug 1208920. From this perspective, uplift would be super!
Comment 77•9 years ago
|
||
(In reply to Geoff Brown [:gbrown] from comment #76)
> These changes drastically reduced the run time of service worker mochitests
> on Android Debug. Those tests are still running slowly on aurora causing
> regular timeouts on aurora in Android 4.3 Debug mochitest-13 - bug 1208920.
> From this perspective, uplift would be super!
Hmm, Catalin, would have expected this?
Flags: needinfo?(catalin.badea392)
Assignee | ||
Comment 78•9 years ago
|
||
(In reply to Ehsan Akhgari (don't ask for review please) from comment #77)
> (In reply to Geoff Brown [:gbrown] from comment #76)
> > These changes drastically reduced the run time of service worker mochitests
> > on Android Debug. Those tests are still running slowly on aurora causing
> > regular timeouts on aurora in Android 4.3 Debug mochitest-13 - bug 1208920.
> > From this perspective, uplift would be super!
>
> Hmm, Catalin, would have expected this?
Not really, my guess is this happened because I disabled some tests on android that were intermittently failing.
Flags: needinfo?(catalin.badea392)
Comment 79•9 years ago
|
||
I think there must be more to it.
Quickly comparing some long-running tests on aurora with inbound:
http://archive.mozilla.org/pub/mobile/tinderbox-builds/mozilla-inbound-android-api-11-debug/1445939905/mozilla-inbound_ubuntu64_vm_armv7_large-debug_test-mochitest-13-bm125-tests1-linux64-build16.txt.gz
http://archive.mozilla.org/pub/mobile/tinderbox-builds/mozilla-aurora-android-api-11-debug/1445857461/mozilla-aurora_ubuntu64_vm_armv7_large-debug_test-mochitest-13-bm124-tests1-linux64-build7.txt.gz
inbound 402 INFO TEST-OK | dom/workers/test/test_fibonacci.html | took 11074ms
aurora 391 INFO TEST-OK | dom/workers/test/test_fibonacci.html | took 118481ms
inbound 422 INFO TEST-OK | dom/workers/test/test_multi_sharedWorker_lifetimes.html | took 49877ms
aurora 411 INFO TEST-OK | dom/workers/test/test_multi_sharedWorker_lifetimes.html | took 286601ms
inbound 438 INFO TEST-OK | dom/workers/test/test_performance_observer.html | took 30223ms
aurora 427 INFO TEST-OK | dom/workers/test/test_performance_observer.html | took 215618ms
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•