Ben Kelly [:bkelly, not reviewing] Reporter
	Description • 9 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 1 • 9 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 2 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 3 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 4 • 8 years ago

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.

	Eric Rahm [:erahm]
	Comment 5 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 6 • 8 years ago

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.

	Eric Rahm [:erahm]
	Comment 7 • 8 years ago

Sounds like a nice win.

	Cătălin Badea (:catalinb) Assignee
	Comment 8 • 8 years ago

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

	Cătălin Badea (:catalinb) Assignee
	Comment 10 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 11 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 12 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 13 • 8 years ago

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?

	Cătălin Badea (:catalinb) Assignee
	Comment 14 • 7 years ago

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

	Cătălin Badea (:catalinb) Assignee
	Comment 15 • 7 years ago

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.

	Cătălin Badea (:catalinb) Assignee
	Comment 16 • 7 years ago

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

	Cătălin Badea (:catalinb) Assignee
	Comment 18 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 19 • 7 years ago

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

	Cătălin Badea (:catalinb) Assignee
	Comment 20 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 21 • 7 years ago

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

	Pulsebot
	Comment 22 • 7 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 23 • 7 years ago
bugherder

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