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.

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

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.

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.

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.

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

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

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.

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

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

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