Implement service worker event handler spec changes for no-fetch optimization

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bkelly, Assigned: catalinb)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

3 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.
(Reporter)

Comment 1

3 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.
Summary: Implement service worker event handler spec changes for no-fetch optimization → Implement service worker disableFetch()
(Reporter)

Comment 2

3 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
Summary: Implement service worker disableFetch() → Implement service worker event handler spec changes for no-fetch optimization
(Reporter)

Comment 3

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

Comment 4

2 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.
Whiteboard: [MemShrink]
Ben, what kind of memory overhead are we talking here?
Flags: needinfo?(bkelly)
(Reporter)

Comment 6

2 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.
Flags: needinfo?(bkelly)
Sounds like a nice win.
Whiteboard: [MemShrink] → [MemShrink:P2]
(Reporter)

Updated

2 years ago
Blocks: 1226983
(Assignee)

Updated

2 years ago
Assignee: nobody → catalin.badea392
(Assignee)

Comment 8

2 years ago
Created attachment 8815738 [details] [diff] [review]
Don't run service workers for fetch events if no fetch handlers were added during script's evaluation.

I'm guessing the warning message needs to be localized, but I don't know how to do
that.
Attachment #8815738 - Flags: review?(bkelly)
(Assignee)

Comment 9

2 years ago
Created attachment 8815739 [details] [diff] [review]
Mochitest for nofetch handler optimization.
Attachment #8815739 - Flags: review?(bkelly)
(Assignee)

Comment 10

2 years ago
Created attachment 8815741 [details] [diff] [review]
WPT test for addEventListener(fetch) throwing behaviour.

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

Comment 11

2 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
Attachment #8815738 - Flags: review?(bkelly) → review-
(Reporter)

Comment 12

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

Comment 13

2 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?
Attachment #8815739 - Flags: review?(bkelly)
(Assignee)

Comment 14

2 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.
(Assignee)

Comment 15

2 years ago
Created attachment 8818249 [details] [diff] [review]
Don't run service workers for fetch events if no fetch handlers were added during script's evaluation.

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.
Attachment #8815738 - Attachment is obsolete: true
Attachment #8818249 - Flags: review?(bkelly)
(Assignee)

Comment 16

2 years ago
Created attachment 8818250 [details] [diff] [review]
Mochitest for nofetch handler optimization.

Removed copy pasted comment. Chrome also doesn't fire the event, even if the SW
is already running.
Attachment #8815739 - Attachment is obsolete: true
Attachment #8818250 - Flags: review?(bkelly)
(Assignee)

Comment 17

2 years ago
Created attachment 8818251 [details] [diff] [review]
Update ServiceWorkerRegistrar gTest.
Attachment #8818251 - Flags: review?(bkelly)
(Assignee)

Comment 18

2 years ago
Created attachment 8818252 [details] [diff] [review]
Fix nsContentUtils::FormatLocalizedString NS_ASSERTION.

nsStringBundle::FormatStringFromName asserts that GetStringFromName
should be used when the error message has no parameters.
Attachment #8818252 - Flags: review?(bkelly)
(Reporter)

Comment 19

2 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())
Attachment #8818249 - Flags: review?(bkelly) → review+
(Reporter)

Updated

2 years ago
Attachment #8818250 - Flags: review?(bkelly) → review+
(Reporter)

Updated

2 years ago
Attachment #8818251 - Flags: review?(bkelly) → review+
(Reporter)

Updated

2 years ago
Attachment #8818252 - Flags: review?(bkelly) → review+
(Assignee)

Comment 20

2 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.
(Reporter)

Comment 21

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

Comment 22

2 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

Comment 23

2 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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Reporter)

Updated

2 years ago
Depends on: 1325101
(Reporter)

Updated

2 years ago
Blocks: 1328293
You need to log in before you can comment on or make changes to this bug.