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)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox42 --- affected
firefox44 --- fixed

People

(Reporter: catalinb, Assigned: catalinb)

References

(Blocks 1 open bug)

Details

Attachments

(15 files, 18 obsolete files)

Disentangle service workers from shared workers and refactor event dispatching code into a separate class.
144.67 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
ServiceWorkerRegistrationInfo::Clear() should terminated workers.
2.00 KB, patch
Details | Diff | Splinter Review
Cosmetic changes regarding workerPrivate properties shared between shared workers and service workers.
10.67 KB, patch
Details | Diff | Splinter Review
Terminate service workers that have been idle for some time.
33.99 KB, patch
Details | Diff | Splinter Review
respondWith promise should extend the service worker's lifetime.
3.64 KB, patch
Details | Diff | Splinter Review
Prevent service workers from being terminated while checking if the script evaluation was successful.
1.97 KB, patch
Details | Diff | Splinter Review
Add tests for service workers' lifetime management.
22.49 KB, patch
Details | Diff | Splinter Review
Reset network interceptions when the service worker is being terminated with unresolved respondWith promises.
10.06 KB, patch
Details | Diff | Splinter Review
Make skip-waiting-using-registration.html wpt test pass. a=testonly
1.40 KB, patch
Details | Diff | Splinter Review
Fix wpt test ServiceWorkerGlobalScope/unregister.https.html. a=testonly
2.23 KB, patch
Details | Diff | Splinter Review
Disable unstable test: test_aboutserviceworkers.html. a=testonly
650 bytes, patch
Details | Diff | Splinter Review
Workaround for incorrect subject principal when checking storage access. r?nsm
1.03 KB, patch
nsm
: review+
Details | Diff | Splinter Review
Update expected results for some service worker tests. a=testonly
10.16 KB, patch
Details | Diff | Splinter Review
Disable test_fetch_cors.html & test_fetch_cors_reroute.html on android debug. a=testonly
1.10 KB, patch
Details | Diff | Splinter Review
Fixup for test_serviceworker_lifetime.html a=testonly
2.36 KB, 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.
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: 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+
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+
Blocks: 1179401
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)
Bug 1188545: Cosmetic changes regarding workerPrivate properties shared between shared workers and service workers. r?nsm
Attachment #8646041 - Flags: review?(nsm.nikhil)
Bug 1188545: Terminate service workers that have been idle for some time. r?nsm
Attachment #8646042 - Flags: review?(nsm.nikhil)
(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
(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).
Depends on: 1193133
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-
(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.
Attachment #8642503 - Attachment is obsolete: true
Attachment #8642500 - Attachment is obsolete: true
Attachment #8646414 - Flags: review?(bkelly)
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+
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.
Attachment #8646040 - Attachment is obsolete: true
Attachment #8646041 - Attachment is obsolete: true
Attachment #8646042 - Attachment is obsolete: true
Bug 1188545: Terminate service workers that have been idle for some time. r?nsm
Attachment #8647889 - Flags: review?(nsm.nikhil)
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)
(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+
Blocks: 1179540
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
Attachment #8647889 - Attachment is obsolete: true

        Attachment #8651274 -
        Flags: review?(nsm.nikhil)

    
  
Depends on: 1197421

    
    

    
  
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+

    
    

    
  
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+

    
    

    
  
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+

    
    

    
  
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?

    
    

    
  
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)

    
  

        Attachment #8656907 -
        Flags: review?(khuey) → review?(mrbkap)

    
    

    
  
(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.

    
  
No longer blocks: 1179401
Status: NEW → ASSIGNED

    
    

    
  
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 :)

    
    

    
  
(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)

    
    

    
  
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

    
    

    
  
Catalin: reminder to nominate this for Aurora when you get a chance.  Thanks!
Flags: needinfo?(catalin.badea392)

    
  
Blocks: 1204596

    
    

    
  
(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?

    
    

    
  
ni Liz to follow up on this bug that I was pinged about uplifting.
Flags: needinfo?(lhenry)

    
    

    
  
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)

    
    

    
  
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!

    
    

    
  
(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)

    
    

    
  
(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)

    
    

    
  
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
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: