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>

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.

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.

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

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

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

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

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