1113582 - Fix ServiceWorker error handling during installation.

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

ServiceWorker parse/network errors should reject the promise returned by register().

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: ServiceWorker parse errors are now properly handled during the Update phase

Folded:
Enable network error detection test. Fix scopes for network test and parse...

... test to deal with https://github.com/slightlyoff/ServiceWorker/issues/547

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8539211 [details] [diff] [review]
ServiceWorker parse errors are now properly handled during the Update phase

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

Can you ask someone to take a look the JS part?

::: dom/workers/ServiceWorkerManager.cpp
@@ +308,5 @@
> +    mPromise->MaybeReject(cx, error);
> +  }
> +};
> +
> +class ContinueUpdateRunnable : public nsRunnable

MOZ_FINAL

@@ +314,5 @@
> +  nsMainThreadPtrHandle<nsISupports> mJob;
> +public:
> +  explicit ContinueUpdateRunnable(const nsMainThreadPtrHandle<nsISupports> aJob)
> +    : mJob(aJob)
> +  { }

ASSERT worker thread

@@ +327,5 @@
> +  CheckWorkerEvaluationAndContinueUpdateWorkerRunnable(WorkerPrivate* aWorkerPrivate,
> +                                                       const nsMainThreadPtrHandle<nsISupports> aJob)
> +    : WorkerRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount)
> +    , mJob(aJob)
> +  { }

In which thread can this obj be created? Can we have an assertion?

@@ +479,5 @@
> +        new nsMainThreadPtrHolder<nsISupports>(upcasted));
> +
> +    nsRefPtr<CheckWorkerEvaluationAndContinueUpdateWorkerRunnable> r =
> +      new CheckWorkerEvaluationAndContinueUpdateWorkerRunnable(serviceWorker->GetWorkerPrivate(), handle);
> +    AutoSafeJSContext cx;

AutoJSAPI here.

@@ +481,5 @@
> +    nsRefPtr<CheckWorkerEvaluationAndContinueUpdateWorkerRunnable> r =
> +      new CheckWorkerEvaluationAndContinueUpdateWorkerRunnable(serviceWorker->GetWorkerPrivate(), handle);
> +    AutoSafeJSContext cx;
> +    bool ok = r->Dispatch(cx);
> +    if (!ok) {

NS_WARN_IF(!ok)

@@ +546,5 @@
> +
> +    nsRefPtr<InstallEventRunnable> r =
> +      new InstallEventRunnable(serviceWorker->GetWorkerPrivate(), handle, mRegistration->mScope);
> +
> +    AutoSafeJSContext cx;

AutoJSAPI

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8539211 [details] [diff] [review]
ServiceWorker parse errors are now properly handled during the Update phase

Boris, in UpdateFailed(const ErrorEventInit& aErrorDesc), I'm converting the dictionary to a JS error and rejecting a Promise. Could you verify I'm not making any JSAPI boo-boo.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8539211 [details] [diff] [review]
ServiceWorker parse errors are now properly handled during the Update phase

If ToJSValue returns false you need to either report the exception on the cx or clear it.  Probably the latter in your case.

Same thing if CreateError returns false.

Apart from that, UpdateFailed looks fine.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bf6b6a31b6a

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3bf6b6a31b6a