1112073 - [Fetch] Response.redirect() method is wrong implemented

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Arkadiusz Michalski (Spirit)]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141125180439

Steps to reproduce:

Algorithm from spec:
https://fetch.spec.whatwg.org/#dom-response-redirect

Actually when invoke redirect("someURL", status) we can see that:

- "someURL" argument isn't parse, passing wrong URL not throw
- status argument is not taken into account
- default value for response's status is 200 (but should be 302)
- "someURL" argument isn't set in headers list as value of `Location' header

Looks like that now redirect() method wokrs the same as Response() constructor.

Comment 1

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

Attachment: Implement Response.redirect

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8541714 [details] [diff] [review]
Implement Response.redirect

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

Overall this looks good and the test nicely shows that it does what the spec says.  Just a few small issues.  r=me with these addressed.

Also flagging Andrea to look it over as I'm not a peer.

::: dom/fetch/Response.cpp
@@ +56,2 @@
>  {
> +  nsString parsedURL;

nsAutoString

@@ +57,5 @@
> +  nsString parsedURL;
> +
> +  if (NS_IsMainThread()) {
> +    nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports());
> +    MOZ_ASSERT(window);

This assert is unneeded as nsCOMPtr<> de-ref will crash on null.

@@ +59,5 @@
> +  if (NS_IsMainThread()) {
> +    nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports());
> +    MOZ_ASSERT(window);
> +    nsCOMPtr<nsIURI> docURI = window->GetDocumentURI();
> +    nsCString spec;

nsAutoCString

@@ +74,5 @@
> +
> +    url->Stringify(parsedURL, aRv);
> +    if (aRv.Failed()) {
> +      return nullptr;
> +    }

You could move this last failure check from here and the else condition to just after the else block.  This would remove a little duplicate code.

@@ +80,5 @@
> +    workers::WorkerPrivate* worker = workers::GetCurrentThreadWorkerPrivate();
> +    MOZ_ASSERT(worker);
> +    worker->AssertIsOnWorkerThread();
> +
> +    nsString baseURL = NS_ConvertUTF8toUTF16(worker->GetLocationInfo().mHref);

Does this create an extra string ref count?  You can avoid the issue by just making the local have NS_ConvertUTF8toUTF16 directly:

  NS_ConvertUTF8toUTF16 baseURL(worker->GetLocationInfo().mHref);

@@ +96,5 @@
> +
> +  if (aStatus != 301 && aStatus != 302 && aStatus != 303 && aStatus != 307 && aStatus != 308) {
> +    aRv.Throw(NS_ERROR_RANGE_ERR);
> +    return nullptr;
> +  }

It would be nice if the spec allowed us to do this first so we didn't have to waste time parsing the URL.

@@ +98,5 @@
> +    aRv.Throw(NS_ERROR_RANGE_ERR);
> +    return nullptr;
> +  }
> +
> +  Optional<ArrayBufferOrArrayBufferViewOrBlobOrUSVStringOrURLSearchParams> body;

Holy type name, batman!

::: dom/webidl/Response.webidl
@@ +11,5 @@
>   Exposed=(Window,Worker),
>   Func="mozilla::dom::Headers::PrefEnabled"]
>  interface Response {
>    [NewObject] static Response error();
> +  [NewObject, Throws] static Response redirect(USVString url, optional unsigned short status = 302);

The feedback I recently received on webidl was to put moz-specific things like Throws on their own line.  The goal is to make the webidl as close to the spec as possible so copy/paste comparisons can be mode.

So maybe this instead:

  [Throws,
   NewObject] static Response redirect(USVString url, optional unsigned short status = 302);

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8541714 [details] [diff] [review]
Implement Response.redirect

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

Nothing more to add to bkelly's comments.

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9932fb743bf8

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed5d8f2baead

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9932fb743bf8
https://hg.mozilla.org/mozilla-central/rev/ed5d8f2baead