The service worker spec was recently changed to require event handlers to be statically registered when the script is first evaluated. This allows the browser to avoid spinning up the worker for events that won't be processed. https://github.com/slightlyoff/ServiceWorker/issues/718#issuecomment-119052515 Since this is an optimization, I'm going to put it under v3 for now.

I believe the plan is to switch this to an explicit disableFetch(), but its not spec'd yet: https://github.com/slightlyoff/ServiceWorker/issues/718 This one is probably lower priority.

And we're back to the original proposal in comment 0. https://github.com/slightlyoff/ServiceWorker/issues/718#issuecomment-175249520 In addition: 1) self.addEventListener() only works on first script evaluation 2) during script evaluation on install we record which event handlers are registered or not 3) async calls to addEventListener() throw

Note, we should still trigger service worker script update checks even if we don't fire the fetch event. See: https://github.com/slightlyoff/ServiceWorker/issues/905

I think this is kind of a memshrink issue. We are going to be spinning up more workers for no reason as sites like facebook roll out push. I already see a SW running a lot for twitter since they started using push. Every time we allocate memory for this worker that isn't used we're risking memory fragmentation.

Ben, what kind of memory overhead are we talking here?

Not huge. 1.5MB for twitter, for example. But it could help avoid fragmentation by not allocating and then deallocating workers over and over for no purpose.

Sounds like a nice win.

I'm guessing the warning message needs to be localized, but I don't know how to do that.

This is the wpt test I wrote for the throwing addEventListener('fetch'). Attaching this here just in case it might be useful in the future. Note, the fetch interception subtest might not have the proper assertions.

Comment on attachment 8815738 [details] [diff] [review] Don't run service workers for fetch events if no fetch handlers were added during script's evaluation. Review of attachment 8815738 [details] [diff] [review]: ----------------------------------------------------------------- Good progress, but a few big omissions still: 1. It needs to trigger an update check even if the fetch event is not fired. 2. The fetch handling flag still needs to be persisted to disk. ::: dom/workers/ServiceWorkerInfo.cpp @@ +171,5 @@ > ServiceWorkerInfo::ServiceWorkerInfo(nsIPrincipal* aPrincipal, > const nsACString& aScope, > const nsACString& aScriptSpec, > + const nsAString& aCacheName, > + bool aHandlesFetch) Instead of a bool please use an enumeration. It makes the code a lot easier to read by avoid bare true/false values at the call sites. Also, I think we could add some safety by having a tri-state here: enum class FetchEventHandling { Unknown, Enabled, Disabled }; Then we can assert that SetHandlesFetch() is only called on "unknown" values. Once we set a value we should never change it. ::: dom/workers/ServiceWorkerPrivate.cpp @@ +146,5 @@ > + > + bool fetchHandlerWasAdded = aWorkerPrivate->FetchHandlerWasAdded(); > + nsCOMPtr<nsIRunnable> runnable = NewRunnableMethod<bool>(this, > + &CheckScriptEvaluationWithCallback::MainThreadRun, fetchHandlerWasAdded); > + MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(runnable.forget())); Why is this done as a separate runnable. ReportScriptEvaluationResult() also dispatches a main thread runnable. It seems like this information could be included there, no? @@ +1690,5 @@ > + if (NS_WARN_IF(!mInfo)) { > + return NS_ERROR_FAILURE; > + } > + > + if (!mInfo->HandlesFetch()) { Maybe add a comment pointing at the spec steps for the no-fetch optimization. @@ +1691,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + if (!mInfo->HandlesFetch()) { > + aChannel->ResetInterception(); This still needs to trigger an update check. ::: dom/workers/ServiceWorkerRegistrarTypes.ipdlh @@ +12,5 @@ > struct ServiceWorkerRegistrationData > { > nsCString scope; > nsCString currentWorkerURL; > + bool currentWorkerHandlesFetch; So, this is inadequate to persist the value. You need to modify the on-disk schema in: https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerRegistrar.cpp#636

Comment on attachment 8815738 [details] [diff] [review] Don't run service workers for fetch events if no fetch handlers were added during script's evaluation. Review of attachment 8815738 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerScope.cpp @@ +651,5 @@ > + nsString message; > + message.AppendLiteral("Fetch event handlers must be added during the worker" > + " script's initial evaluation."); > + swm->ReportToAllClients(mScope, message, NS_ConvertUTF8toUTF16(mSourceSpec), > + EmptyString(), mLine, mColumn, nsIScriptError::warningFlag); As you mentioned, this should be localized. See: https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#2005 The localization string is defined in dom.properties: https://dxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/dom/dom.properties#233

Comment on attachment 8815739 [details] [diff] [review] Mochitest for nofetch handler optimization. Review of attachment 8815739 [details] [diff] [review]: ----------------------------------------------------------------- Can you confirm chrome behavior here? Sorry I don't have time to test it right now. ::: dom/workers/test/serviceworkers/test_nofetch_handler.html @@ +3,5 @@ > +<!-- > + Test that an unresolved respondWith promise will reset the channel when > + the service worker is terminated due to idling, and that appropriate error > + messages are logged for both the termination of the serice worker and the > + resetting of the channel. This comment seems copy pasted. @@ +29,5 @@ > + ["dom.serviceWorkers.enabled", true], > + ["dom.serviceWorkers.testing.enabled", true], > + // Make sure the event handler during the install event persists. This ensures > + // the reason for which the interception doesn't occur is because of the > + // handlesFetch=false flag from ServiceWorkerInfo. So this is actually different behavior from chrome. They *do* fire the fetch event if the service worker is still alive, but they won't start the server worker if its not running. I think anyway. Can you double check the behavior?

(In reply to Ben Kelly [:bkelly] from comment #13) > @@ +29,5 @@ > > + ["dom.serviceWorkers.enabled", true], > > + ["dom.serviceWorkers.testing.enabled", true], > > + // Make sure the event handler during the install event persists. This ensures > > + // the reason for which the interception doesn't occur is because of the > > + // handlesFetch=false flag from ServiceWorkerInfo. > > So this is actually different behavior from chrome. They *do* fire the > fetch event if the service worker is still alive, but they won't start the > server worker if its not running. I think anyway. Can you double check the > behavior? I tested this on my machine, chrome doesn't fire the event.

Addressed comments with the exception of the double runnable dispatch. The callback runnable is external to SWPrivate and I'd have to modify its usage in quite a few places. I'll write a different patch for it.

Removed copy pasted comment. Chrome also doesn't fire the event, even if the SW is already running.

nsStringBundle::FormatStringFromName asserts that GetStringFromName should be used when the error message has no parameters.

Comment on attachment 8818249 [details] [diff] [review] Don't run service workers for fetch events if no fetch handlers were added during script's evaluation. Review of attachment 8818249 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. Thanks! ::: dom/workers/ServiceWorkerInfo.h @@ +140,5 @@ > mState = aState; > } > > void > + SetHandlesFetch(bool aHandlesFetch) I was hoping to get rid of the boolean in the method signature, but ok. @@ +143,5 @@ > void > + SetHandlesFetch(bool aHandlesFetch) > + { > + AssertIsOnMainThread(); > + MOZ_ASSERT(mHandlesFetch == Unknown); Can you make this a diagnostic assert as well? ::: dom/workers/ServiceWorkerPrivate.cpp @@ +146,5 @@ > + > + bool fetchHandlerWasAdded = aWorkerPrivate->FetchHandlerWasAdded(); > + nsCOMPtr<nsIRunnable> runnable = NewRunnableMethod<bool>(this, > + &CheckScriptEvaluationWithCallback::ReportFetchFlag, fetchHandlerWasAdded); > + MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(runnable.forget())); aWorkerPrivate->DispatchToMainThread(runnable.forget()) @@ +1719,5 @@ > + aChannel->ResetInterception(); > + > + // Trigger soft updates if necessary. > + nsCOMPtr<nsIRunnable> runnable = NewRunnableMethod(registration, > + &ServiceWorkerRegistrationInfo::MaybeScheduleTimeCheckAndUpdate); Can you remove RegistrationUpdateRunnable and use NewRunnableMethod() where its currently used? AFAICT it never gets called with aNeedTimeCheck=false any more. So we can just call MaybeScheduleTimeCheckAndUpdate like you do here. @@ +1720,5 @@ > + > + // Trigger soft updates if necessary. > + nsCOMPtr<nsIRunnable> runnable = NewRunnableMethod(registration, > + &ServiceWorkerRegistrationInfo::MaybeScheduleTimeCheckAndUpdate); > + MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(runnable.forget())); Please use WorkerPrivate::DispatchToMainThread() here. ::: dom/workers/ServiceWorkerRegistrar.cpp @@ +354,5 @@ > > + nsAutoCString fetchFlag; > + GET_LINE(fetchFlag); > + if (!fetchFlag.EqualsLiteral(SERVICEWORKERREGISTRAR_TRUE) && > + !fetchFlag.EqualsLiteral(SERVICEWORKERREGISTRAR_FALSE)) { nit: Can you align the indenting of the two conditionals? ::: dom/workers/WorkerScope.cpp @@ +664,5 @@ > + > + if (aCallback) { > + if (mWorkerPrivate->WorkerScriptExecutedSuccessfully()) { > + RefPtr<Runnable> r = new ReportFetchListenerWarningRunnable(mScope); > + MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(r)); mWorkerPrivate->DispatchToMainThread(r.forget()) @@ +684,5 @@ > + mWorkerPrivate->AssertIsOnWorkerThread(); > + > + if (mWorkerPrivate->WorkerScriptExecutedSuccessfully()) { > + RefPtr<Runnable> r = new ReportFetchListenerWarningRunnable(mScope); > + MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(r)); mWorkerPrivate->DispatchToMainThread(r.forget())

Thanks for the review! (In reply to Ben Kelly [:bkelly] from comment #19) > Can you remove RegistrationUpdateRunnable and use NewRunnableMethod() where > its currently used? AFAICT it never gets called with aNeedTimeCheck=false > any more. So we can just call MaybeScheduleTimeCheckAndUpdate like you do > here. Filed bug 1323482. > > @@ +1720,5 @@ > > + > > + // Trigger soft updates if necessary. > > + nsCOMPtr<nsIRunnable> runnable = NewRunnableMethod(registration, > > + &ServiceWorkerRegistrationInfo::MaybeScheduleTimeCheckAndUpdate); > > + MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(runnable.forget())); > > Please use WorkerPrivate::DispatchToMainThread() here. This is on the main thread.

(In reply to Cătălin Badea (:catalinb) from comment #20) > > @@ +1720,5 @@ > > > + > > > + // Trigger soft updates if necessary. > > > + nsCOMPtr<nsIRunnable> runnable = NewRunnableMethod(registration, > > > + &ServiceWorkerRegistrationInfo::MaybeScheduleTimeCheckAndUpdate); > > > + MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(runnable.forget())); > > > > Please use WorkerPrivate::DispatchToMainThread() here. > This is on the main thread. Oh, actually I meant to suggest just calling "MaybeScheduleTimeCheckAndUpdate()" directly here. Since you are already on the main thread you don't need to fire a runnable at all.

Pushed by catalin.badea392@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e87c846f9809 Don't run service workers for fetch events if no fetch handlers were added during script's evaluation. r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/1ccd58a14a6a Mochitest for nofetch handler optimization. r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/5c871d92c76f Update ServiceWorkerRegistrar gTest. r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/c8160e0e3959 Fix nsContentUtils::FormatLocalizedString NS_ASSERTION. r=bkelly

https://hg.mozilla.org/mozilla-central/rev/e87c846f9809 https://hg.mozilla.org/mozilla-central/rev/1ccd58a14a6a https://hg.mozilla.org/mozilla-central/rev/5c871d92c76f https://hg.mozilla.org/mozilla-central/rev/c8160e0e3959