1112922 - [Fetch] Implement request referrer correctly in Fetch API

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:

Initial value for request's referrer  is client:
https://fetch.spec.whatwg.org/#concept-request-referrer

When we create new Request object by Request() constructor request's refferer is still client (step 3.):
https://fetch.spec.whatwg.org/#dom-request

So, for client as request's referrer we should get "about:client" (acording referrer attribute https://fetch.spec.whatwg.org/#dom-request-referrer), but now it's empty string.

Small test:

<script type = "text/javascript">

 var request = new Request("");
 document.write("request.referrer: " + request.referrer + "<br>"); // "" (empty string)
 document.write("request.referrer.length: " + request.referrer. length+ "<br>"); // 0

</script>

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Nikhil, I think this is something we talked about doing on maple, but it never happened.  This goes back to setting the referrer type automatically based on the referrer string.  Right?

Blocking enabling Cache on this because Cache only persists the referrer string and not a separate referrer type enumeration.

Comment 2

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

Repurposing this bug to fix referrer in general:
1) Return appropriate values to JS callers.
2) Get rid of referrer type, use empty string for no-referrer, about:client for client and a url for url.
3) Add referrer policy to FetchDriver so the appropriate referrer is sent on HTTP requests.
4) Ensure fetch() sets referrer url correctly before invoking FetchDriver.

Comment 3

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

Attachment: Implement request referrer correctly in Fetch API

Context is in patch 5 of Bug dom-fetch-api

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I'm sorry, but with the holiday I probably won't get to this review until January.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8540750 [details] [diff] [review]
Implement request referrer correctly in Fetch API

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

Overall looks good.  I think it could use a few more asserts and better usage of nsString.  r=me with the comments addressed.

Thanks!

::: dom/fetch/Fetch.cpp
@@ +375,4 @@
>  {
> +  nsCString originalReferrer = aRequest->GetReferrer();
> +  // If no-referrer, don't change it.
> +  if (originalReferrer.IsEmpty() || !originalReferrer.EqualsASCII("about:client")) {

I believe EqualsLiteral() is preferred to EqualsASCII().  This permits an optimized check against the static string length.

Also, this comment seems incorrect.  Its not just doing a no-referrer check.

I think the code can be simplified to:

  if (!originalReferrer.EqualsLiteral("about:client")) {
    return NS_OK;
  }

Finally, any chance we can define "about:client" as a constant somewhere?  Its used here, FetchDriver.cpp, and InternalRequest.h.

::: dom/fetch/FetchDriver.cpp
@@ +314,5 @@
>  
> +    // Set the referrer.
> +    nsCString referrer = mRequest->GetReferrer();
> +    // The referrer should have already been resolved to a URL by the caller.
> +    MOZ_ASSERT(!referrer.EqualsASCII("about:client"));

EqualsLiteral

::: dom/fetch/InternalRequest.h
@@ +129,5 @@
>  
>    void
>    SetReferrer(const nsACString& aReferrer)
>    {
> +    mReferrer.Assign(aReferrer);

Would it be possible to assert that this is "about:client", empty string, or a valid URL in debug builds?  I think you should be able to use nsStdURLParser to verify the URL without worrying about main thread issues.

@@ +248,5 @@
>  
> +  // Empty string: no-referrer
> +  // "about:client": client (default)
> +  // URL: an URL
> +  nsCString mReferrer;

Can we make this an nsString?  It seems we do a lot of UTF16toUTF8 and back conversions all over the place.  If mReferrer was just UTF16, then we could remove all of these and just let NS_NewURI() handle it.

::: dom/fetch/Request.h
@@ +67,5 @@
>  
>    void
>    GetReferrer(DOMString& aReferrer) const
>    {
> +    aReferrer.AsAString() = NS_ConvertUTF8toUTF16(mRequest->GetReferrer());

Note, the Cache needs to get the referrer and does not have a DOMString.  Can you change this method to use nsAString& instead?  The WebIDL documentation says this is allowed.

  https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#DOMString

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Oh, can we add tests for this as well?  It seems we could at least verify creating a new Request() gets a referrer of "about:client".

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8540750 [details] [diff] [review]
Implement request referrer correctly in Fetch API

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

Plus the Ben's comments. It looks good to me.

::: dom/fetch/Fetch.cpp
@@ +375,2 @@
>  {
> +  nsCString originalReferrer = aRequest->GetReferrer();

nsAutoCString

::: dom/fetch/FetchDriver.cpp
@@ +312,5 @@
>        httpChan->SetRequestHeader(headers[i].mName, headers[i].mValue, false /* merge */);
>      }
>  
> +    // Set the referrer.
> +    nsCString referrer = mRequest->GetReferrer();

nsAutoCString

::: dom/fetch/InternalRequest.h
@@ +48,5 @@
>    explicit InternalRequest()
>      : mMethod("GET")
>      , mHeaders(new InternalHeaders(HeadersGuardEnum::None))
>      , mContextFrameType(FRAMETYPE_NONE)
> +    , mReferrer("about:client")

a const for this?

@@ +121,5 @@
>      aURL.Assign(mURL);
>    }
>  
> +  nsCString
> +  GetReferrer() const

This makes a copy of the object. Can you do:

nsCString& GetReferrer() const

or better:

void GetReferrer(nsACString& aRef) const ?

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini (:baku) from comment #7)
> @@ +121,5 @@
> >      aURL.Assign(mURL);
> >    }
> >  
> > +  nsCString
> > +  GetReferrer() const
> 
> This makes a copy of the object. Can you do:
> 
> nsCString& GetReferrer() const
> 
> or better:
> 
> void GetReferrer(nsACString& aRef) const ?

Btw, I asked about this one on IRC.  I got a split verdict.  Ehsan favored return by-value, while Ollie suggested returning by-const-reference.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9959a5e91de9

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9959a5e91de9