1181887 - We need to fall back to go to the network if attempting to start a service worker for handling fetch events fails

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[(no longer active)]

See bug 1180765.  It seems like if we fail to start up the service worker, we essentially give up and don't try to go to the network in order to load the website.  We need to properly fall back to loading stuff from the network if anything goes wrong when starting up a service worker.  Basically, we should never get into the state where failing to load a service worker breaks loading a website.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Fall back to network if ServiceWorker script fails to load. r=ehsan r=khuey

This patch lets me fallback to network correctly when I purposely break my Cache storage directory.  Not sure how to test that in automation.

Kyle, can you review the changes to ScriptLoader.cpp, WorkerPrivate.*, and RuntimeService.*?

Ehsan, can you review the service worker changes?

Thanks!

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5eb6a089428

Comment 3

[(no longer active)]

Comment on attachment 8632268 [details] [diff] [review]
Fall back to network if ServiceWorker script fails to load. r=ehsan r=khuey

Review of attachment 8632268 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

::: dom/workers/WorkerPrivate.cpp
@@ +4764,5 @@
>    , mCancelAllPendingRunnables(false)
>    , mPeriodicGCTimerRunning(false)
>    , mIdleGCTimerRunning(false)
>    , mWorkerScriptExecutedSuccessfully(false)
> +  // TODO

??

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8632268 [details] [diff] [review]
Fall back to network if ServiceWorker script fails to load. r=ehsan r=khuey

Review of attachment 8632268 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/WorkerPrivate.cpp
@@ +4764,5 @@
>    , mCancelAllPendingRunnables(false)
>    , mPeriodicGCTimerRunning(false)
>    , mIdleGCTimerRunning(false)
>    , mWorkerScriptExecutedSuccessfully(false)
> +  // TODO

Stale reminder to set mLoadFailedRunnable. I'll remove.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8632268 [details] [diff] [review]
Fall back to network if ServiceWorker script fails to load. r=ehsan r=khuey

Review of attachment 8632268 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerManager.cpp
@@ +1063,5 @@
>  
> +    // Call FailScopeUpdate on main thread if the SW script load fails below.
> +    nsCOMPtr<nsIRunnable> failRunnable = NS_NewRunnableMethodWithArgs
> +      <StorensRefPtrPassByPtr<ServiceWorkerManager>, nsCString>
> +      (this, &ServiceWorkerRegisterJob::FailScopeUpdate, swm, scopeKey);

This can result in releasing ServiceWorkerManager on the worker thread, which would violate your threading assumptions.

::: dom/workers/WorkerPrivate.cpp
@@ +5465,5 @@
>  }
>  
>  void
> +WorkerPrivate::MaybeDispatchLoadFailedRunnable()
> +{

AssertIsOnWorkerThread();

::: dom/workers/WorkerPrivate.h
@@ +956,5 @@
>  
>    nsRefPtrHashtable<nsUint64HashKey, MessagePort> mWorkerPorts;
>  
> +  // fired on the main thread if the worker script fails to load
> +  nsCOMPtr<nsIRunnable> mLoadFailedRunnable;

Can this go in WorkerLoadInfo? And maybe name it mLoadFailedAsync(hronously?)Runnable to make it more clear that it's only invoked if the load fails asynchronously?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #5)
> Comment on attachment 8632268 [details] [diff] [review]
> > +    // Call FailScopeUpdate on main thread if the SW script load fails below.
> > +    nsCOMPtr<nsIRunnable> failRunnable = NS_NewRunnableMethodWithArgs
> > +      <StorensRefPtrPassByPtr<ServiceWorkerManager>, nsCString>
> > +      (this, &ServiceWorkerRegisterJob::FailScopeUpdate, swm, scopeKey);
> 
> This can result in releasing ServiceWorkerManager on the worker thread,
> which would violate your threading assumptions.

Kyle, this code runs in the main thread.  There is an AssertIsOnMainThread() here:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp?from=ServiceWorkerManager.cpp&case=true#1003

And the runnable is being dispatched to the main thread.

Can you explain how this could trigger release on the worker?

I'll fix the other two issues tomorrow.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Hmm.  I guess if the runnable is never executed and the WorkerPrivate is destroyed on the worker thread.  I suppose I could main thread doom the runnable in LoadInfo to avoid this.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The WorkerPrivate is always destroyed on the parent thread.

The threading issue comes from MaybeDispatchLoadFailedRunnable, which refcounts the runnable on the worker thread.  The runnable can execute on the main thread before NS_DispatchToMainThread returns, resulting in the last ref being released on the worker thread.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Hmm.  Do we have a way to pass already_AddRefed to NS_DispatchToMaimThread now?

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Fall back to network if ServiceWorker script fails to load. r=ehsan r=khuey

Updated to store the runnable in LoadInfo and always release it on the main thread.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8633636 [details] [diff] [review]
Fall back to network if ServiceWorker script fails to load. r=ehsan r=khuey

Actually... I see a small change to make to the patch.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Fall back to network if ServiceWorker script fails to load. r=ehsan r=khuey

Removed some unnecessary changes left over from the previous implementation approach.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0548c9269ee

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

I tried provoking this in a test with a malformed script, but this seems to be caught by existing checks.  With out building some way to force an "unexpected" error, this may be difficult to test in automation.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8633638 [details] [diff] [review]
Fall back to network if ServiceWorker script fails to load. r=ehsan r=khuey

Review of attachment 8633638 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +13,3 @@
>  interface nsIURI;
>  
> +[scriptable, uuid( 8d80dd18-597b-4378-b41e-768bfe48dd4f)]

This is a uuid change on the wrong interface ...

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #15)
> > +[scriptable, uuid( 8d80dd18-597b-4378-b41e-768bfe48dd4f)]
> 
> This is a uuid change on the wrong interface ...

Ouch, yes.  Fixed.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0462f22ccc70

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0462f22ccc70