Spec does not allow redirects for SW scripts - http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#update-algorithm step 4.3.8

/r/3627 - Bug 1131271 - Set ServiceWorker script redirect limit to zero. Pull down this commit: hg pull review -r 22ad0e6c8503897a70c98bca39b7783bedebf653

Nikhil, where does it say redirects should not be allowed? That section you link only checks redirect count if there was already an error.

Good catch! It sounds weird now though, why do we discriminate between a network error and one that is specifically a redirect? Anne, is this some Fetch thing?

I filed https://github.com/slightlyoff/ServiceWorker/issues/618

Comment on attachment 8561658 [details] MozReview Request: bz://1131271/nsm Dropping review flag until we sort out the spec.

The spec has been modified. It says if the request is a network error or response is not in range 200-299, fail with type error. In accordance with that I've changed the failure codes. The GetResponseSucceeded() on the channel checks for the 200-299 case. The spec says to set the manual redirection flag on the request. What this does in the fetch algorithm is on a 302-308 code, respond with that as a response rather than internally handling the redirect. Necko would allow us to do something like this using the async redirect callbacks used by FetchDriver and nsCORSListenerProxy, but setting the limit to zero is the easier way.

Comment on attachment 8576287 [details] [diff] [review] Set ServiceWorker script redirect limit to zero Review of attachment 8576287 [details] [diff] [review]: ----------------------------------------------------------------- r=m with comments addressed. Mainly curious if we want all errors to be type errors or only redirect limit errors. Ehsan, can you look this over as neither Nikhil or myself are DOM peers? Thanks. ::: dom/workers/ServiceWorkerManager.cpp @@ +532,5 @@ > nsresult aStatus, uint32_t aLen, > const uint8_t* aString) MOZ_OVERRIDE > { > if (NS_WARN_IF(NS_FAILED(aStatus))) { > + Fail(NS_ERROR_DOM_TYPE_ERR); Do we want to throw TYPE_ERR on all these? Or do we want to look explicitly for NS_ERROR_REDIRECT_LOOP and convert to a TYPE_ERR only in that case?

Comment on attachment 8576287 [details] [diff] [review] Set ServiceWorker script redirect limit to zero Ben's review should be enough.

Nikhil, this needs a test. Were you planning to write one?

(In reply to Ben Kelly [:bkelly] from comment #7) > Comment on attachment 8576287 [details] [diff] [review] > Set ServiceWorker script redirect limit to zero > > Review of attachment 8576287 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=m with comments addressed. Mainly curious if we want all errors to be > type errors or only redirect limit errors. > > Ehsan, can you look this over as neither Nikhil or myself are DOM peers? > Thanks. > > ::: dom/workers/ServiceWorkerManager.cpp > @@ +532,5 @@ > > nsresult aStatus, uint32_t aLen, > > const uint8_t* aString) MOZ_OVERRIDE > > { > > if (NS_WARN_IF(NS_FAILED(aStatus))) { > > + Fail(NS_ERROR_DOM_TYPE_ERR); > > Do we want to throw TYPE_ERR on all these? Or do we want to look explicitly > for NS_ERROR_REDIRECT_LOOP and convert to a TYPE_ERR only in that case? Any NetworkError in fetching a SW leads to failure with TypeError in the spec. This captures that. I've added a test and will push once try agrees.