1264178 - expose URL fragment on fetch Request.url

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

At the face-to-face we agreed to expose the URL fragment on the fetch Request/Response objects:

  https://github.com/slightlyoff/ServiceWorker/issues/854

This is pretty straightforward for the fetch DOM objects, but requires modifications to Cache API.

For Cache, we need to store the fragment but not include it when matching URLs.  This means we need to pre-parse the fragment out and store it in a separate column.  This will let us continue to index the URL for matching and then re-append the fragment on deserialization.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This should probably build on top of the patches in bug 1243792 since they both touch Request/Response URLs.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Note the spec changes finally landed and are slightly different than originally discussed:

https://github.com/whatwg/fetch/commit/111da37ad6c6485d0b77cfa5437200b34b1f7f0e

Comment 3

[Tom Tung [:tt, :ttung]]

I would like to take this bug.

Comment 4

[Tom Tung [:tt, :ttung]]

Attachment: Expose url fragment to request but not response.

Basically in this patch, I did 
 - Fragment appears on request.url and only store it in the last redirect url if it doesn't have a fragment. 
 - Fragment absent from response.url
 - Store fragment in the cache and ignore it by cache.match.

I'm not sure whether I go on the right direction and whether I did the everything. Could you give me some feedback about that, Ben? Thanks!

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8789696 [details] [diff] [review]
Expose url fragment to request but not response.

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

Its a good start, but I don't think the fragment is restored to the request coming out of the Cache API.  Also, I'm not sure the way the fragment is propagated in InternalRequest is correct.

::: dom/cache/CacheTypes.ipdlh
@@ +55,5 @@
>  {
>    nsCString method;
>    nsCString urlWithoutQuery;
>    nsCString urlQuery;
> +  nsCString urlFragment;

Where is this passed back into the InternalRequest to stitch the fragment back onto the URL?

::: dom/cache/test/mochitest/test_cache_match_request.js
@@ +50,5 @@
>      return Promise.all(
>        ["HEAD", "POST", "PUT", "DELETE", "OPTIONS"]
>          .map(function(method) {
>            var r = new Request(request, {method: method});
> +          ok(r.url.indexOf('#') !== -1, "The request url should have a fragment.");

This seems an unusual place for this check, but ok.

I think we need a test showing `cache.keys()` provides the same request with a fragment on it.

::: dom/fetch/InternalRequest.h
@@ +209,5 @@
> +                                          : mURLList.LastElement().FindChar('#');
> +    if (lastHash != -1) {
> +      if (addURL.FindChar('#') != -1) {
> +        addURL.Append(Substring(mURLList.LastElement(), lastHash + 1,
> +                                mURLList.LastElement().Length()));

This seems really error prone.  Maybe we should just track the fragment separately from the URL?  That would let us more easily set our url's without fragments on the response, etc.

Also, where in the spec does it say to propagate the original hash through the redirection?  I can't find it.

::: dom/fetch/InternalResponse.h
@@ +134,5 @@
> +    // We should not expose fragment to response.
> +    int32_t lastHash = mURLList.IsEmpty() ? -1
> +                                          : mURLList.LastElement().FindChar('#');
> +    if (lastHash != -1) {
> +      mURLList.LastElement().Truncate(lastHash);

Why are we only doing this on the last element?  The assert right below this iterates over the entire list.  Won't that blow up if one of the items in the list has a fragment?

Comment 6

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #5)
> Its a good start, but I don't think the fragment is restored to the request
> coming out of the Cache API.  Also, I'm not sure the way the fragment is
> propagated in InternalRequest is correct.

Thanks for the feedback! I'll rewrite the patch for this bug. I thought we need to propagated the fragment when the redirect URL don't have fragment (a.com#foo -> b.com#foo if a.com is redirected to b.com). Maybe I misunderstood something, please correct me if I was wrong.

> ::: dom/cache/CacheTypes.ipdlh
> @@ +55,5 @@
> >  {
> >    nsCString method;
> >    nsCString urlWithoutQuery;
> >    nsCString urlQuery;
> > +  nsCString urlFragment;
> 
> Where is this passed back into the InternalRequest to stitch the fragment
> back onto the URL?

I passed the fragment back in TypeUtils::ToInternalRequest(const CacheRequest& aIn).
 
> ::: dom/cache/test/mochitest/test_cache_match_request.js
> @@ +50,5 @@
> >      return Promise.all(
> >        ["HEAD", "POST", "PUT", "DELETE", "OPTIONS"]
> >          .map(function(method) {
> >            var r = new Request(request, {method: method});
> > +          ok(r.url.indexOf('#') !== -1, "The request url should have a fragment.");
> 
> This seems an unusual place for this check, but ok.

hmm, I'll try to find another method for this check.
 
> I think we need a test showing `cache.keys()` provides the same request with
> a fragment on it.

Sure. I'll do it in next patch.

> 
> ::: dom/fetch/InternalRequest.h
> @@ +209,5 @@
> > +                                          : mURLList.LastElement().FindChar('#');
> > +    if (lastHash != -1) {
> > +      if (addURL.FindChar('#') != -1) {
> > +        addURL.Append(Substring(mURLList.LastElement(), lastHash + 1,
> > +                                mURLList.LastElement().Length()));
> 
> This seems really error prone.  Maybe we should just track the fragment
> separately from the URL?  That would let us more easily set our url's
> without fragments on the response, etc.

Sure. I thought we don't need to create another string and we could maintain the fragment in the last element of URL list but tracking it separately from the URL seems much better. I'll create a member variable for storing the last URL's fragment.

> Also, where in the spec does it say to propagate the original hash through
> the redirection?  I can't find it.

I followed the anne's comment[1] in the SW issue 854: "reuse if no fragment on redirect URL, otherwise use redirect URL fragment". Did I misunderstand something?

[1] https://github.com/w3c/ServiceWorker/issues/854#issuecomment-206524115 

> ::: dom/fetch/InternalResponse.h
> @@ +134,5 @@
> > +    // We should not expose fragment to response.
> > +    int32_t lastHash = mURLList.IsEmpty() ? -1
> > +                                          : mURLList.LastElement().FindChar('#');
> > +    if (lastHash != -1) {
> > +      mURLList.LastElement().Truncate(lastHash);
> 
> Why are we only doing this on the last element?  The assert right below this
> iterates over the entire list.  Won't that blow up if one of the items in
> the list has a fragment?

I planed to only store the fragment in the last element for the whole URL list. That's why I only removed the last URL's fragment. Thus, the other URLs (expect the last one) should not have fragment.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] from comment #6)
> (In reply to Ben Kelly [:bkelly] from comment #5)
> > Its a good start, but I don't think the fragment is restored to the request
> > coming out of the Cache API.  Also, I'm not sure the way the fragment is
> > propagated in InternalRequest is correct.
> 
> Thanks for the feedback! I'll rewrite the patch for this bug. I thought we
> need to propagated the fragment when the redirect URL don't have fragment
> (a.com#foo -> b.com#foo if a.com is redirected to b.com). Maybe I
> misunderstood something, please correct me if I was wrong.

This could be the case, but I'd like to see the spec text saying this.  I couldn't find it.  Maybe its a spec bug that it doesn't say this?  What do we do with redirected fragments today?

> > Also, where in the spec does it say to propagate the original hash through
> > the redirection?  I can't find it.
> 
> I followed the anne's comment[1] in the SW issue 854: "reuse if no fragment
> on redirect URL, otherwise use redirect URL fragment". Did I misunderstand
> something?
> 
> [1] https://github.com/w3c/ServiceWorker/issues/854#issuecomment-206524115 

Did this make it into the spec text?  If not we might need to write a spec issue.

Comment 8

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #7)
> (In reply to Tom Tung [:tt] from comment #6)
> > (In reply to Ben Kelly [:bkelly] from comment #5)
> This could be the case, but I'd like to see the spec text saying this.  I
> couldn't find it.  Maybe its a spec bug that it doesn't say this?  What do
> we do with redirected fragments today?
> 
> Did this make it into the spec text?  If not we might need to write a spec
> issue.

No, you are right. I cannot find it in neither fetch nor URL spec. Where should I file the issue? fetch or URL?

Comment 9

[Anne (:annevk)]

Currently the specification doesn't copy over fragments for redirects. Only the higher-level navigate algorithm does that, which implements its own redirect logic. Is there a reason it should happen for all fetches?

Comment 10

[Tom Tung [:tt, :ttung]]

(In reply to Anne (:annevk) from comment #9)
Sorry for the really reply. I was working on another bug and on my PTO.

> Currently the specification doesn't copy over fragments for redirects. Only
> the higher-level navigate algorithm does that, which implements its own
> redirect logic. Is there a reason it should happen for all fetches?

I think we should have the same redirect logic for fetch but I am sure about is it necessary for all fetches. What do you think about that, Ben?

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Since navigation redirects don't get processed by fetch(), I think we can avoid any fragment propagation here.  If we ever grow the ability to do a fetch() with mode='navigate' and redirect='follow', then we would need to add the propagation in for that case.  Today, however, Request's with mode 'navigate' always use 'manual' redirection.

Comment 12

[Anne (:annevk)]

I think even if we did allow that, we wouldn't necessarily want to add propagation, since the propagation is not done at the Fetch layer. It's done at the "Navigate" layer.

Comment 13

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #11)
> Since navigation redirects don't get processed by fetch(), I think we can
> avoid any fragment propagation here.  If we ever grow the ability to do a
> fetch() with mode='navigate' and redirect='follow', then we would need to
> add the propagation in for that case.  Today, however, Request's with mode
> 'navigate' always use 'manual' redirection.

(In reply to Anne (:annevk) from comment #12)
> I think even if we did allow that, we wouldn't necessarily want to add
> propagation, since the propagation is not done at the Fetch layer. It's done
> at the "Navigate" layer.

Oh, I see. So I should just make Request expose the fragment and let somewhere that doing propagation to deal with it. I should not propagate fragment for the fetch. Thanks for the feedback!

I'll complete the patch as soon as possible.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] from comment #13)
> Oh, I see. So I should just make Request expose the fragment and let
> somewhere that doing propagation to deal with it. I should not propagate
> fragment for the fetch. Thanks for the feedback!

Yea, we do the propagation down in the guts of necko I believe.  This is different, but equivalent to, the spec.  The end result is the same, though.  We don't want to do any fragment propagation in fetch() itself.  Thanks!

Comment 15

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1264178-v0-Expose url fragment to request but not response.

Hi Ben,

I remove the propagation part and ensure that we can get the cached request with fragment. Could you help me to review this patch? Thanks!

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8801019 [details] [diff] [review]
Bug-1264178-v0-Expose url fragment to request but not response.

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

Thanks, this is going in the right direction.  I think we need to be a bit more careful with the fragment parsing, though.

Also, can you add a test where we do some navigations/fetch() requests with a fragment and then verify the service worker sees the fragment?

Thanks again.

::: dom/fetch/InternalRequest.h
@@ +197,4 @@
>    // If internal code is directly constructing this object they must
>    // strip the fragment first.  Since these should be well formed URLs we
>    // can use a simple check for a fragment here.  The full parser is
>    // difficult to use off the main thread.

This comment is no longer accurate.

@@ +204,5 @@
>      MOZ_ASSERT(!aURL.IsEmpty());
> +
> +    nsAutoCString url(aURL);
> +
> +    int32_t hash = url.IsEmpty() ? -1 : aURL.FindChar('#');

So, the FindChar() was ok for the assertion, but I hesitate to use it for operational code.  URL parsing can be surprising tricky.

I think we should modify this method to take separate URL and fragment strings.  The caller must then do the right thing to get the fragment parsed out using nsIURI, etc.

::: dom/fetch/Request.cpp
@@ +113,5 @@
>    if (NS_WARN_IF(aRv.Failed())) {
>      return;
>    }
>  
>    CopyUTF8toUTF16(spec, aRequestURL);

We should get the fragment and copy it to an out param here.

@@ +153,5 @@
>    if (NS_WARN_IF(aRv.Failed())) {
>      return;
>    }
>  
>    CopyUTF8toUTF16(spec, aRequestURL);

We should get the fragment and copy it to an out param here.

@@ +202,1 @@
>    url->Stringify(aRequestURL, aRv);

We should get the fragment and copy it to an out param here.

::: dom/fetch/Request.h
@@ +47,5 @@
>    GetUrl(nsAString& aUrl) const
>    {
>      nsAutoCString url;
>      mRequest->GetURL(url);
> +    url.Append(mRequest->GetFragment());

I think it would be clearer if InternalRequest::GetURL() returned the URL with the fragment.  Then if we need the URL without the fragment we can have an InternalRequest::GetURLWithoutFragment().

Comment 17

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #16)
> Comment on attachment 8801019 [details] [diff] [review]

Thanks for feedback and review.
I'll address comment but I don't get few of them. I leave them below inline.

> Also, can you add a test where we do some navigations/fetch() requests with
> a fragment and then verify the service worker sees the fragment?

Sure.

> ::: dom/fetch/InternalRequest.h
> @@ +197,4 @@
> >    // If internal code is directly constructing this object they must
> >    // strip the fragment first.  Since these should be well formed URLs we
> >    // can use a simple check for a fragment here.  The full parser is
> >    // difficult to use off the main thread.
> 
> This comment is no longer accurate.

Sorry, I don't understand this one. 
I think the comment is still correct if I set the fragment in Request::Constructor. We want internal code set the fragment and then stripe the fragment in url. We don't want to add the URL with fragment into URLList which may lead the fragment to be exposed in InternalResponse.

> @@ +204,5 @@
> >      MOZ_ASSERT(!aURL.IsEmpty());
> > +
> > +    nsAutoCString url(aURL);
> > +
> > +    int32_t hash = url.IsEmpty() ? -1 : aURL.FindChar('#');
> 
> So, the FindChar() was ok for the assertion, but I hesitate to use it for
> operational code.  URL parsing can be surprising tricky.

Oh, I see. I only consider normal cases. Thanks for telling me that.

> I think we should modify this method to take separate URL and fragment
> strings.  The caller must then do the right thing to get the fragment parsed
> out using nsIURI, etc.

I'll use the function for parsing fragment in nsIURI/URL.

> ::: dom/fetch/Request.cpp
> @@ +113,5 @@
> >    if (NS_WARN_IF(aRv.Failed())) {
> >      return;
> >    }
> >  
> >    CopyUTF8toUTF16(spec, aRequestURL);
> 
> We should get the fragment and copy it to an out param here.

Sure. I'll get the fragment, store it and stripe the fragment and call the constructor as usual. Then, I'll set internalRequest's fragment.

> 
> ::: dom/fetch/Request.h
> @@ +47,5 @@
> >    GetUrl(nsAString& aUrl) const
> >    {
> >      nsAutoCString url;
> >      mRequest->GetURL(url);
> > +    url.Append(mRequest->GetFragment());
> 
> I think it would be clearer if InternalRequest::GetURL() returned the URL
> with the fragment.  Then if we need the URL without the fragment we can have
> an InternalRequest::GetURLWithoutFragment().

Sure, should I change the InternalRequest::GetURLList() to InternalRequest::GetURLListWithoutFragment()? It's a bit strange for me to have a GetURL() to get a URL with fragment and a GetURLList() to get a list of URL without their fragment.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] from comment #17)
> Sure, should I change the InternalRequest::GetURLList() to
> InternalRequest::GetURLListWithoutFragment()? It's a bit strange for me to
> have a GetURL() to get a URL with fragment and a GetURLList() to get a list
> of URL without their fragment.

Naming it GetURLListWithoutFragment() sounds reasonable to me.  Thanks.

Comment 19

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1264178-v1-Expose url fragment to request but not response.

Hi Ben,

I addressed the comment basically. Could you help me to review this patch? Thanks!
In this patch, I do the following things:
  - Set the fragment just after we construct the new internalRequest (Request.cpp/ServiceWorkerPrivate.cpp).
  - Rename the GetURL(), GetURLList() to *WithoutFragment() in InternalRequest and add the GetURL() into it.
  - Rather than adding a test for navigations/fetch(), I find out that we've already have one(fetch-event.https.html) for checking no fragment so I modify that test. 
  - Add some comment.

Besides, I'm thinking about renaming the similar URL's getter function in InternalResponse(..) (e.g. GetURL(), GetURLList()) but I'm not sure about whether to do that. Should I renaming them, too?

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8802161 [details] [diff] [review]
Bug-1264178-v1-Expose url fragment to request but not response.

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

r=me with comments addressed.  Thanks!

::: dom/cache/DBSchema.cpp
@@ +2054,5 @@
>      "SELECT "
>        "request_method, "
>        "request_url_no_query, "
>        "request_url_query, "
> +      "request_url_fragment, "

If you put this column at the end of the SELECT column list you can avoid having to re-number all the result getters.

::: dom/cache/TypeUtils.cpp
@@ +159,5 @@
>    aOut.requestCache() = aIn->GetCacheMode();
>    aOut.requestRedirect() = aIn->GetRedirectMode();
>  
>    aOut.integrity() = aIn->GetIntegrity();
> +  aOut.urlFragment() = aIn->GetFragment();

Can you move this up to where we set the urlWithoutQuery and urlQuery fields?  It would be nice to keep all the URL stuff together.

::: dom/fetch/InternalRequest.h
@@ +190,5 @@
>    {
>      MOZ_RELEASE_ASSERT(!mURLList.IsEmpty(), "Internal Request's urlList should not be empty.");
>  
> +    if (GetFragment().IsEmpty()) {
> +      aURL.Assign(mURLList.LastElement());

Instead of duplicating the assert and the Assign here.  Can you write GetURL() like:

GetURL(nsACString& aURL) const
{
  GetURLWithoutFragment(aURL);
  if (!GetFragment().IsEmpty()) {
    aURL.Append(NS_LITERAL_CSTRING("#");
    aURL.Append(GetFragment());
  }
}

@@ +199,5 @@
> +                GetFragment());
> +  }
> +
> +  void
> +  GetURLWithoutFragment(nsACString& aURL) const

I think you could just make this return const nsCString& like GetFragment().

@@ +401,5 @@
> +    return mFragment;
> +  }
> +
> +  void
> +  SetFragment(const nsACString& aFragment)

Instead of a SetFragment(), please add a second argument to AddURL().  Then override the mFragment every time AddURL() is called.  This ensures we don't accidentally propagate the fragment from one URL to the next if code forgets to call SetFragment() to clear it.

Also, the constructor that takes a URL should also take a fragment argument.

::: dom/fetch/Request.cpp
@@ +314,5 @@
>      nsAutoString input;
>      input.Assign(aInput.GetAsUSVString());
>  
>      nsAutoString requestURL;
> +    nsAutoCString fragment;

Since you are passing the fragment directly to the request which will store it, using an nsCString is slightly better.  It will just store the underlying buffer and not have to copy again.

@@ +332,5 @@
>        return nullptr;
>      }
>  
>      request = new InternalRequest(NS_ConvertUTF16toUTF8(requestURL));
> +    request->SetFragment(fragment);

For example, here it would be a bit nicer to write:

  request = new InternalRequest(NS_ConvertUTF16toUTF8(requestURL), fragment);

Comment 21

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #20)

Hi Ben,

Thanks for your review and your time! I learn a lot from them and I'll address the comment. However, I don't understand two of them. I put them below inline.

> ::: dom/cache/DBSchema.cpp
> @@ +2054,5 @@
> >      "SELECT "
> >        "request_method, "
> >        "request_url_no_query, "
> >        "request_url_query, "
> > +      "request_url_fragment, "
> 
> If you put this column at the end of the SELECT column list you can avoid
> having to re-number all the result getters.

Could I still keep them together? Because, I think it's better to keep URL stuff together.

> ::: dom/fetch/InternalRequest.h
> @@ +199,5 @@
> > +                GetFragment());
> > +  }
> > +
> > +  void
> > +  GetURLWithoutFragment(nsACString& aURL) const
> 
> I think you could just make this return const nsCString& like GetFragment().

Sure, but I think it might be a little strange to be different between GetURL(aURL) and nsCString GetURLFragment(). Should I worry about this or it doesn't matter? 

BTW, if it doesn't matter, I would like to also change the URL's getter function in InternalResponse to return const nsCString& if it's possible.

Comment 22

[Tom Tung [:tt, :ttung]]

Attachment: [WIP] Bug-1264178-v2-Expose url fragment to request but not response.

Address comment 20 except DBSchema.cpp.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] (Out of office during 10/26 ~ 10/30) from comment #21)
> > If you put this column at the end of the SELECT column list you can avoid
> > having to re-number all the result getters.
> 
> Could I still keep them together? Because, I think it's better to keep URL
> stuff together.

Sure.

> > > +  void
> > > +  GetURLWithoutFragment(nsACString& aURL) const
> > 
> > I think you could just make this return const nsCString& like GetFragment().
> 
> Sure, but I think it might be a little strange to be different between
> GetURL(aURL) and nsCString GetURLFragment(). Should I worry about this or it
> doesn't matter? 

Actually, I think you can change the InternalRequest::GetURL() as well.  You can do this:

nsCString GetURL()
{
    nsCString url(GetURLWithoutFragment());
    if (!GetFragment().IsEmpty()) {	
      return url;	
    }
    url.Append(NS_LITERAL_CSTRING("#"));
    url.Append(GetFragment());
    return url;
}
	
This should result in the return-value-optimization doing a move of the string for the return.

> BTW, if it doesn't matter, I would like to also change the URL's getter
> function in InternalResponse to return const nsCString& if it's possible.

Changing InternalResponse::GetURL() to return the URL instead of an out-param sounds good to me.

Comment 24

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #23)

Thanks a lot! I'll complete the patch as soon as possible!

> Actually, I think you can change the InternalRequest::GetURL() as well.  You
> can do this:
> 
> nsCString GetURL()
> {
>     nsCString url(GetURLWithoutFragment());
>     if (!GetFragment().IsEmpty()) {	
>       return url;	
>     }
>     url.Append(NS_LITERAL_CSTRING("#"));
>     url.Append(GetFragment());
>     return url;
> }
>

Thanks for telling so! I forgot that I could return the value... 
I was thinking return nsCString's reference.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] (Out of office during 10/26 ~ 10/30) from comment #24)
> Thanks for telling so! I forgot that I could return the value... 
> I was thinking return nsCString's reference.

Sorry, the conversation on IRC concluded we should stick with GetURL(nsACString& aURL) for now.  The return value optimization is too easy to break currently.

Comment 26

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #25)
> Sorry, the conversation on IRC concluded we should stick with
> GetURL(nsACString& aURL) for now.  The return value optimization is too easy
> to break currently.

So, it's only for InternalRequest::GetURL(..) (change from returning nsString to void) or even InternalRequest::GetURLFragment() and URL's getter in InternalResponse (reference) should do so?

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Its fine to return `const nsCString&` when you have an internal nsCString you are returning.  But the `nsCString` return is not ok.  In that case its better to pass the string to be filled in.

Comment 28

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #27)
> Its fine to return `const nsCString&` when you have an internal nsCString
> you are returning.  But the `nsCString` return is not ok.  In that case its
> better to pass the string to be filled in.

Ok, thanks! So, we don't want to have additional copies on nsCString.

Comment 29

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1264178-v3-Expose url fragment to request but not response.

Hi Ben,

Sorry for long delay, I was attending conference. 

I address the comment, re-base the patch and fix some tests in this patch. Although you have already given previous patch r+, I modify other file(FlyWebPublishedServer.cpp), Constructor(Request.cpp), ctor's caller and modify some testcases in this patch. 

I'm not sure about those changes so I send a review request again. Could you help me to review this patch again? Thanks!

Comment 30

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1264178-P2- Change InternalResponse URL's getter function from PassByReference to ReturnByReference.

Hi Ben,

I create this patch for changing the getter function for InternalReponse's URL from pass argument by reference to return by reference. Could you help me to review this one? Thanks!

Comment 31

[Tom Tung [:tt, :ttung]]

Attachment: interdiff for Bug-1264178-v3 & Bug-1264178-v1

interdiff for 8806286: Bug-1264178-v3 and Attachment #8802161 [details] [diff]: Bug-1264178-v1 (previous r+).

Comment 32

[Tom Tung [:tt, :ttung]]

try result: [1]

It seems that cache.put twice and use cache.keys to check the URL of request immediately will cause intermittent failure in Win and Linux environment. 
```
 cache.put(req1, res1).then(() => cache.put(req2, res2))
  .then(function () {
    cache.keys(req2).then(function(keys) {
      keys.forEach(function(r, index, array) {
        is(req2.url, r.url, "Request URL should be the same"); // this will cause intermittent failure 
      });
    })
    return cache.match(req1);
})
```

However, I don't think this is related to this bug. I will implement a patch to remove the new test in test_cache_match_request.js and add a test for cache.keys in test_cache_keys.js.

Should I file a bug for this(cache.put twice may not be written immediately)?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=13e51413e51c160cf2bd123c1d4dea1fc5cae1f8

Comment 33

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1264178-P3-Add a test in test_cache_keys and remove the added test in P1.

Patch for fixing the test failure in P1. Could you help me to review this patch, Ben? Thanks!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3326547d61bc5560275e2c891ea78649c135c875

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] from comment #32)
> It seems that cache.put twice and use cache.keys to check the URL of request
> immediately will cause intermittent failure in Win and Linux environment. 
> ```
>  cache.put(req1, res1).then(() => cache.put(req2, res2))
>   .then(function () {
>     cache.keys(req2).then(function(keys) {
>       keys.forEach(function(r, index, array) {
>         is(req2.url, r.url, "Request URL should be the same"); // this will
> cause intermittent failure 
>       });
>     })
>     return cache.match(req1);
> })
> ```
> 
> However, I don't think this is related to this bug. I will implement a patch
> to remove the new test in test_cache_match_request.js and add a test for
> cache.keys in test_cache_keys.js.

I believe the cache test framework runs multiple tests in parallel.  Are your cache names suitably unique to avoid different runs of the test stepping on each other?

Comment 35

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #34)
> I believe the cache test framework runs multiple tests in parallel.  Are
> your cache names suitably unique to avoid different runs of the test
> stepping on each other?

Thanks for telling me that but I think the failure isn't caused by the same cache name. 

Rather than adding a whole new test, I add my test inside the existing test. I added my test (which causing failure) for fragment in test_cache_match_request.js, the cache name [1] is different from tests and contexts.

[1] http://searchfox.org/mozilla-central/source/dom/cache/test/mochitest/test_cache_match_request.js#7

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] from comment #32)
> ```
>  cache.put(req1, res1).then(() => cache.put(req2, res2))
>   .then(function () {
>     cache.keys(req2).then(function(keys) {
>       keys.forEach(function(r, index, array) {
>         is(req2.url, r.url, "Request URL should be the same"); // this will
> cause intermittent failure 
>       });
>     })
>     return cache.match(req1);
> })
> ```

Please try making the test wait for the result of the `cache.keys()` code here.  I think its racing with the other parts of the test causing unexpected behavior.

So something like:

   cache.put(req1, res1).then(() => cache.put(req2, res2))
    .then(function () {
      return cache.keys(req2).then(function(keys) {
        keys.forEach(function(r, index, array) {
          is(req2.url, r.url, "Request URL should be the same"); // this will cause intermittent failure 
        });
        return cache.match(req1);
      });
  })

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8806286 [details] [diff] [review]
Bug-1264178-v3-Expose url fragment to request but not response.

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

Overall this looks great.  Thanks for making the changes!

I'm leaving the review open for now, though, because I want to understand if and why we still fail that last WPT test.

::: dom/cache/test/mochitest/test_cache_match_request.js
@@ +37,5 @@
> +
> +  cache.put(req1, res1).then(() => cache.put(req2, res2))
> +  .then(function () {
> +    // Test for ensuring we get the fragment from cache request.
> +    cache.keys(req2).then(function(keys) {

You need to wait for this to complete before proceeding.

::: dom/fetch/InternalRequest.h
@@ +163,4 @@
>    // If internal code is directly constructing this object they must
>    // strip the fragment first.  Since these should be well formed URLs we
>    // can use a simple check for a fragment here.  The full parser is
>    // difficult to use off the main thread.

I think you should probably just rewrite this whole comment block to say "If a fragment is present in the URL it must be stripped and passed in separately."

@@ -155,2 @@
>      mURLList.AppendElement(aURL);
> -    MOZ_ASSERT(mURLList.LastElement().Find(NS_LITERAL_CSTRING("#")) == kNotFound);

We should keep this assertion since we maintain the fragment in a separate variable.  We don't want to get two fragments by accident.

::: dom/fetch/Request.cpp
@@ +234,5 @@
> +    return;
> +  }
> +
> +  // XXX: URL should get the same format with URI when geting Hash/Ref (exclude '#').
> +  // Remove hash character to unify fragment.

I don't think this needs to be a XXX comment.  Just note that URL::GetHash() includes the "#" and we want the fragment with out the hash symbol.

::: testing/web-platform/meta/fetch/api/request/request-init-003.sub.html.ini
@@ -1,4 @@
>  [request-init-003.sub.html]
>    type: testharness
>    [Check request values when initialized from url string]
>      expected: FAIL

Uh, why do we still fail this one?  It seems like we should pass it now.

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8806286 [details] [diff] [review]
Bug-1264178-v3-Expose url fragment to request but not response.

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

Ok, I applied the patch and ran it locally.  Its failing on the referrer value.  So not a problem with this patch.

r=me

::: dom/cache/test/mochitest/test_cache_match_request.js
@@ +37,5 @@
> +
> +  cache.put(req1, res1).then(() => cache.put(req2, res2))
> +  .then(function () {
> +    // Test for ensuring we get the fragment from cache request.
> +    cache.keys(req2).then(function(keys) {

You need to wait for this to complete before proceeding.

::: dom/fetch/InternalRequest.h
@@ +163,4 @@
>    // If internal code is directly constructing this object they must
>    // strip the fragment first.  Since these should be well formed URLs we
>    // can use a simple check for a fragment here.  The full parser is
>    // difficult to use off the main thread.

I think you should probably just rewrite this whole comment block to say "If a fragment is present in the URL it must be stripped and passed in separately."

@@ -155,2 @@
>      mURLList.AppendElement(aURL);
> -    MOZ_ASSERT(mURLList.LastElement().Find(NS_LITERAL_CSTRING("#")) == kNotFound);

We should keep this assertion since we maintain the fragment in a separate variable.  We don't want to get two fragments by accident.

::: dom/fetch/Request.cpp
@@ +234,5 @@
> +    return;
> +  }
> +
> +  // XXX: URL should get the same format with URI when geting Hash/Ref (exclude '#').
> +  // Remove hash character to unify fragment.

I don't think this needs to be a XXX comment.  Just note that URL::GetHash() includes the "#" and we want the fragment with out the hash symbol.

::: testing/web-platform/meta/fetch/api/request/request-init-003.sub.html.ini
@@ -1,4 @@
>  [request-init-003.sub.html]
>    type: testharness
>    [Check request values when initialized from url string]
>      expected: FAIL

Uh, why do we still fail this one?  It seems like we should pass it now.

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8806290 [details] [diff] [review]
Bug-1264178-P2- Change InternalResponse URL's getter function from PassByReference to ReturnByReference.

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

::: dom/fetch/InternalResponse.h
@@ +93,4 @@
>    {
>      // Empty urlList when response is a synthetic response.
>      if (mURLList.IsEmpty()) {
> +      return EmptyCString();

This looks dangerous, but its returning a `const nsAFlatCString&` by ref already, so should be safe.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +245,5 @@
>  
>      nsresult rv;
>      nsCOMPtr<nsIURI> uri;
>      nsAutoCString url;
> +    url.Assign(mInternalResponse->GetUnfilteredURL());

This is doing an extra copy to the auto string.  Just write:

  nsCString url = mInternalResponse->GetUnfilteredURL();

@@ +640,5 @@
>    // to reflect the final URL. Non-opaque responses are either same-origin or CORS-enabled
>    // cross-origin responses, which are treated as same-origin by consumers.
>    nsCString responseURL;
>    if (response->Type() == ResponseType::Opaque) {
> +    responseURL.Assign(ir->GetUnfilteredURL());

responseURL = ir->GetUnfilteredURL();

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8807071 [details] [diff] [review]
Bug-1264178-P3-Add a test in test_cache_keys and remove the added test in P1.

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

I think we could have fixed the other test, but this is less code in the end.  Works for me.  Thanks.

Comment 41

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #37)

Thanks for the reviews and feedback! 

I realized that the test failure happens because I didn't wait long enough for each instruction to finish its job and I fixed the test.

> ::: dom/cache/test/mochitest/test_cache_match_request.js
> @@ +37,5 @@
> > +
> > +  cache.put(req1, res1).then(() => cache.put(req2, res2))
> > +  .then(function () {
> > +    // Test for ensuring we get the fragment from cache request.
> > +    cache.keys(req2).then(function(keys) {
> 
> You need to wait for this to complete before proceeding.

Thanks! That's the main reason why the failure happens.
I rewrote the test [1] and fix it [2]!

[1] https://hg.mozilla.org/try/rev/b37c8906f4c871840922727c7f8251f44d6a5591
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c15bbc653cc979ae23a805f8060bff00cd5c5cc7

> ::: dom/fetch/InternalRequest.h
> @@ -155,2 @@
> >      mURLList.AppendElement(aURL);
> > -    MOZ_ASSERT(mURLList.LastElement().Find(NS_LITERAL_CSTRING("#")) == kNotFound);
> 
> We should keep this assertion since we maintain the fragment in a separate
> variable.  We don't want to get two fragments by accident.

I change this assertion to |MOZ_ASSERT(!aURL.Contains('#'))|, they should have the same behavior. 

> ::: testing/web-platform/meta/fetch/api/request/request-init-003.sub.html.ini
> @@ -1,4 @@
> >  [request-init-003.sub.html]
> >    type: testharness
> >    [Check request values when initialized from url string]
> >      expected: FAIL
> 
> Uh, why do we still fail this one?  It seems like we should pass it now.

I got the following message: 
assert_equals: Check referrer attribute expected "http://web-platform.test:8000/" but got "about:client".

It seems that we still have some issue in request.referrer when request inits.

(In reply to Ben Kelly [:bkelly] from comment #39)
> Comment on attachment 8806290 [details] [diff] [review]
> Bug-1264178-P2- Change InternalResponse URL's getter function from
> PassByReference to ReturnByReference.
> 
> Review of attachment 8806290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/fetch/InternalResponse.h
> @@ +93,4 @@
> >    {
> >      // Empty urlList when response is a synthetic response.
> >      if (mURLList.IsEmpty()) {
> > +      return EmptyCString();
> 
> This looks dangerous, but its returning a `const nsAFlatCString&` by ref
> already, so should be safe.

This should call [1] so it should be safe. 
Do we have other way to return an empty string to make it look dangerous?

[1] http://searchfox.org/mozilla-central/source/xpcom/string/nsReadableUtils.cpp#1260

Comment 42

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #41)
> This should call [1] so it should be safe. 
> Do we have other way to return an empty string to make it look dangerous?
Sorry, it's make it look "not so" dangerous?

Comment 43

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug-1264178-P1- Expose URL fragment to request but not response. r=bkelly.

Addressed comment.

Comment 44

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug-1264178-P2- Change URL's getter function from PassByReference to ReturnByReference. r=bkelly.

Comment 45

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug-1264178-P3- Add a test in test_cache_keys and remove the added test in P1. r=bkelly.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d35a08a69869d767ad7173cae507454f3949ab9c

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7245abb24375
Part 1: Expose URL fragment to request but not response. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/3363a134f720
Part 2: Change URL's getter function from PassByReference to ReturnByReference. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/de39b88af6c2
Part 3: Add a test in test_cache_keys and remove the added test in P1. r=bkelly

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7245abb24375
https://hg.mozilla.org/mozilla-central/rev/3363a134f720
https://hg.mozilla.org/mozilla-central/rev/de39b88af6c2

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

This just shipped in FF52, but we forgot to flag it for MDN updates.  We just got bug 1352914 reported since they thought it was an accidental change.

Comment 49

[Dave Burton]

I just tested this, by passing URLs to pages on one of my web sites to Firefox, and then checking Apache's access_log file to see what was fetched. The #fragmet is still missing. In fact, searching my access_log files finds no cases in which any browsers newer than IE 6 have fetched URLs with #fragments.

So, has this fix gotten reversed? Or am I confused about what this change was supposed to have done?

Why are there no #fragments on the URLs fetched, listed in Apache's access_log?

Comment 50

[Anne (:annevk)]

You are confused. URL fragments are not part of an HTTP request and so will not reach the server. We do expose them on Request (and Response) objects though.