Closed Bug 1110476 Opened 5 years ago Closed 4 years ago

Fetch Request and Response should strip the URL fragment automatically

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: bkelly, Assigned: A-deLuna, Mentored)

References

Details

(Whiteboard: [good first bug][lang=C++])

Attachments

(1 file, 6 obsolete files)

Per the spec, the Fetch Request and Response URL getter should automatically strip the fragment.

This is something Cache is doing manually with nsStdURLParser until this is fixed.  When the fragment stripping is implemented in Request/Response, this should be removed from Cache.
Assignee: nsm.nikhil → nobody
Mentor: nsm.nikhil
Whiteboard: [good first bug][lang=C++]
How to get started?
Flags: needinfo?(nsm.nikhil)
In dom/base/URL.{h,cpp} you'd add GetHrefIgnoringRef() which would call GetSpecIgnoringRef on the mURI, just like URL::GetHref() calls GetSpec().

In dom/workers/URL.{h,cpp} you'll have to create a runnable and add some enum values and stuff. Please see how GetHref() is implemented there and do something similar but changing for IgnoringRef. Do ask here or on IRC if you don't understand something.

Then have the Response and Request classes use the new function.

Please add tests in dom/workers/test/fetch/worker_test_request.js to create Request with a fragment and see that calling .url on it results in url without fragment.
Flags: needinfo?(nsm.nikhil)
okay I am browsing through the code, I will keep you updated with my progress or any queries if they come up.
Sayan, are you still working on this?  Thanks!
Flags: needinfo?(sayan_666)
Sorry got caught with some work, ya I will be working on this bug.
Flags: needinfo?(sayan_666)
Thank you!  Much appreciated.  :-)
Is this bug still open to work?
If no one is working on this bug I'd like to take it. I have a question about writing the test for this, I don't see the file specified above(dom/workers/test/fetch/worker_test_request.js) in the tree. Does it need to be created? If so are there similar tests I can use as a guide to writing this one?
Phil, you are the new assignee. The test in question has been moved to dom/tests/mochitest/fetch/test_request.js
Assignee: nobody → mrphilh
No longer blocks: serviceworker-cache
blocking v3 meta bug
Hi, I'd like to be assigned to this bug. I think I have implemented the changes on dom/base/URL.{h,cpp} and dom/workers/URL.{h,cpp} correctly but am not sure where is the right place to call it from dom/fetch/Request.{h,cpp}
(In reply to Antonio de Luna [:A_deLuna] from comment #11)
> Hi, I'd like to be assigned to this bug. I think I have implemented the
> changes on dom/base/URL.{h,cpp} and dom/workers/URL.{h,cpp} correctly but am
> not sure where is the right place to call it from dom/fetch/Request.{h,cpp}

Hi Antonio!  I think the place to do it would be in Request::GetURL() and Response::GetURL().

Out of curiosity, what changes did you make to URL.*?
I've changed dom/workers/URL.h's Stringify to call GetHrefIgnoringRef because I thought Request calls GetRequestURLFromWorker, which calls Stringify, but I don't think I have this right, help is appreciated, thanks.
Attachment #8633080 - Flags: feedback?
Comment on attachment 8633080 [details] [diff] [review]
bug1110476_FetchRequestResponseStripFragment.diff

Hmm.  It seems you might have attached an empty patch.  Are your changes in a different patch?
Attachment #8633080 - Flags: feedback?
Attachment #8633080 - Attachment is obsolete: true
Sorry, did something stupid with mercurial, still getting used to it.
Attachment #8633107 - Flags: feedback?
Comment on attachment 8633107 [details] [diff] [review]
bug1110476_FetchRequestResponseStripFragmentfix.diff

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

Hi Antonio,

First, let me say thank you for working on this!

I don't think we can change the URL::Stringify() method, however.  That would break behavior for other callers of Stringify() that expect to see the URL anchor hash.

What I think we want to do instead is modify the GetUrl() methods in Request.h and Response.h:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Request.h?from=Request.h&case=true#44
  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Response.h?from=Response.h#53

These currently just get the URL from the InternalRequest and InternalResponse.

Note, the Internal* objects here are used to store extra information that the browser needs, but that web content should not have access to.  So the full URL should be available in the InternalRequest and InternalResponse, but users of Request and Response should have the ref stripped off.

The other thing to understand here is that our URL parsing is dependent on which thread you execute on.  We have this dubious system that lets plugins register javascript to parse URLs, which means nsIURI can only be used on main thread.  The dom::workers::URL class actually freezes the worker thread, executes the parse on the main thread, and then restarts the worker thread.

In practice, this means you have to check which thread you are on and do different things.

I recommend modifying the Request and Response GetUrl() methods to do something like this:

1) Create a new member on Request and Response objects to store the stripped URL string.
2) Do the following steps to parse the end URL in Request::Constructor and Response::Constructor.
3) Check what thread you are on using NS_IsMainThread()
  a) If you are on the main thread then use the NS_NewURI() method to create an nsIURI object for the url string.
  b) You can then call nsIURI::SetRef() to clear the ref section.
  c) Then use nsIURI::GetSpec() to get the resulting URL string and store it in your new member.
3) If you're not on the main thread from the check in (2)
  a) You first need to get an internal data structure called WorkerPrivate by calling workers::GetCurrentThreadWorkerPrivate()
  b) You can then call workers::URL::Constructor() to create a URL object
  c) Use workers::URL::SetHash() to clear the ref section
  d) Use workers::URL::Stringify() to get the resulting URL string and store it in your new member.
4) Modify Request and Response GetUrl() to return the new member.

For something similar to steps 2 and 3 you can look at this code (You shouldn't have to worry about the base URI bits, though):

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Response.cpp#64

For bonus points, you can try to incorporate this logic into the existing Request::Constructor URL parsing.  It does this with some utility methods:

  GetRequestURLFromDocument()
  GetRequestURLFromChrome()
  GetRequestURLFromWorker()

These methods already have an nsIURI or URL object, so you can modify the ref and get your stripped URL at the end of those methods without creating new objects.  You just need to pipe the stripped URL back correctly and get it set on the new Request object.

I'm sorry this is so complicated.  Our URL string parsing really is difficult to use.
Attachment #8633107 - Flags: feedback?
Reassigning this to Antonio since there was no activity from Phil for a month.
Assignee: mrphilh → tonydelun
Hi Ben,

Thanks for being awesome and for the extremely detailed response.

I'm almost done implementing what you said and I have a couple of questions,

What is the best way to pass the uriString argument to the NS_NewURI function in Response::Constructor? In the example you provided Response::Redirect receives aUrl as a parameter that can be passed to NS_newURI, but I'm not sure what to do in this case. I'm considering getting the url from mInternalResponse but it doesn't feel right.

Similar to the above I don't know how to pass the aUrl argument to workers::URL::Constructor in Response::Constructor, which will be done the same as above.

In the bug report you mentioned we needed to remove url parsing from cache, is it as easy as removing this? https://dxr.mozilla.org/mozilla-central/source/dom/cache/CacheStorage.cpp#113

Also I ended up not using the methods I had previously implemented in the previous patch, should I keep them or remove them?

Thanks for all the help, I appreciate it :) .
Flags: needinfo?(bkelly)
(In reply to Antonio de Luna [:A_deLuna] from comment #18)
> Hi Ben,
> 
> Thanks for being awesome and for the extremely detailed response.
> 
> I'm almost done implementing what you said and I have a couple of questions,
> What is the best way to pass the uriString argument to the NS_NewURI
> function in Response::Constructor? In the example you provided
> Response::Redirect receives aUrl as a parameter that can be passed to
> NS_newURI, but I'm not sure what to do in this case. I'm considering getting
> the url from mInternalResponse but it doesn't feel right.

Hmm.  I was wrong about doing this in Response::Constructor().  That method doesn't actually set the URL as far as I can tell.

It seems Response's URL is only set directly on the InternalResponse in two places:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/TypeUtils.cpp#286
  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#581

The first is in the Cache code when we pull a Response out of the database.  This is just restoring what was previously set on the Response.

The second case is where the fetch() sets the URL on the resulting Response object.  The nice thing here is that this code only runs on the main thread.  So we can avoid the need to do the URL::Constructor.

I think what I would recommend would be:

1) Create an InternalResponse::StripFragmentAndSetUrl() method.  This method should AssertIsMainThread() and then use nsIURL to strip the fragment before calling InternalResponse::SetUrl().
2) Modify FetchDriver.cpp line 581 to call your new StripFragmentAndSetUrl() method.
3) Don't do anything in Response::Constructor.

This means the Response will always have the fragment stripped.  So when a Response is stored in the Cache it will also have the fragment stripped and restore back to the right value when pulled out of the Cache as well.

> In the bug report you mentioned we needed to remove url parsing from cache,
> is it as easy as removing this?
> https://dxr.mozilla.org/mozilla-central/source/dom/cache/CacheStorage.cpp#113

Not quite.  The code in question is here:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/TypeUtils.cpp#428

You can remove all references to refPos and refLen there.

You can verify this works correctly by running the cache tests:

  ./mach mochitest dom/cache

And:

  ./mach web-platform-tests service-workers/cache-storage

> Also I ended up not using the methods I had previously implemented in the
> previous patch, should I keep them or remove them?

Please remove them.

> Thanks for all the help, I appreciate it :)

No problem.  Thanks for working on this!
Flags: needinfo?(bkelly)
Mentor: nsm.nikhil → bkelly
This patch compiles and passes 

./mach mochitest dom/cache
./mach web-platform-tests service-workers/cache-storage

also passes the tests I've added by running 
./mach mochitest dom/tests/mochitest/fetch/test_request.html
Attachment #8634467 - Flags: review?(bkelly)
Attachment #8633107 - Attachment is obsolete: true
Comment on attachment 8634467 [details] [diff] [review]
bug1110476_FetchRequestResponseStripFragmentfix.diff

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

This is a great start!  It just needs a few tweaks for error handling and style.

::: dom/fetch/InternalResponse.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "InternalResponse.h"
>  
> +#include "nsIURI.h"

Please move this down just before nsStreamUtils.h include to keep the headers sorted.

@@ +87,5 @@
>    mPrincipalInfo = Move(aPrincipalInfo);
>  }
>  
> +void
> +InternalResponse::StripFragmentAndSetUrl(const nsACString& aUrl)

Please change this to return nsresult.  Then you can return errors from NS_NewURI() and GetSpec().

You can use MOZ_ALWAYS_TRUE(NS_SUCCEEDS()) to valid SetRef() since it should never fail.

@@ +92,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsCOMPtr<nsIURI> iuri;
> +  NS_NewURI(getter_AddRefs(iuri), aUrl, nullptr, nullptr);

I believe you can leave off the two nullptr values.  The NS_NewURI() defaults those arguments to nullptr already.

@@ +95,5 @@
> +  nsCOMPtr<nsIURI> iuri;
> +  NS_NewURI(getter_AddRefs(iuri), aUrl, nullptr, nullptr);
> +
> +  iuri->SetRef(NS_LITERAL_CSTRING(""));
> +  nsAutoCString spec;

Since this is going to be immediately assigned to an nsCString, I think its better to just use nsCString here instead nsAutoCString.  This avoid copying the string twice.

::: dom/fetch/InternalResponse.h
@@ +179,5 @@
>    void
>    SetPrincipalInfo(UniquePtr<mozilla::ipc::PrincipalInfo> aPrincipalInfo);
>  
> +  void
> +  StripFragmentAndSetUrl(const nsACString& aUrl);

Please add a comment to the SetUrl() method that it should only be called when the fragment has already been stripped.

::: dom/fetch/Request.cpp
@@ +64,5 @@
>        return;
>    }
>  
>    nsAutoCString spec;
> +  resolvedURI->SetRef(NS_LITERAL_CSTRING(""));

So I originally thought we needed to keep the unstripped URL in the InternalRequest, but it turns out the spec doesn't need this.  So this code is correct.

Can you just add a comment to InternalRequest.h where it declares mURL and state that it always has the ref stripped?  If the algorithm changes such that we need it then we can start storing it.

@@ +87,5 @@
>      return;
>    }
>  
>    nsAutoCString spec;
> +  uri->SetRef(NS_LITERAL_CSTRING(""));

Please use EmptyCString() here and in other places in the patch.

Calling nsIURI::SetRef() should always succeed for the empty string.  Rather than ignore the result can you do:

  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(uri->SetRef(EmptyCString())));

Here and above with resolvedURI.

@@ +114,5 @@
>    if (NS_WARN_IF(aRv.Failed())) {
>      return;
>    }
>  
> +  url->SetHash(NS_LITERAL_STRING(""),aRv);

nit: space after the comma

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

nit: can you add a blank line before the Stringify call?

::: dom/tests/mochitest/fetch/test_request.js
@@ +220,5 @@
>  }
>  
> +function testUrlFragmentStripping() {
> +  var req = new Request("./request#withfragment");
> +  is(req.url, "http://mochi.test:8888/tests/dom/tests/mochitest/fetch/request","request.url should have stripped the fragment");

I think we can just fix the testUrlFragment() function right above this.  That function is trying to do the check you are doing here, but its got a bug.  It is using ok() instead of is().
Attachment #8634467 - Flags: review?(bkelly)
Attachment #8634467 - Attachment is obsolete: true
Hello Ben,

I've implement the changes and came up with a question,

Now that InternalResponse::StripFragmentAndSetURL returns an nsresult I'm not sure how to handle it in FetchDriver::BeginAndGetFilteredResponse, for the time being I've just added an assertion but I'd appreciate your input on this.

Thanks for the reviews.
Attachment #8635136 - Flags: review?(bkelly)
Comment on attachment 8635136 [details] [diff] [review]
bug1110476_FetchRequestResponseStripFragmentfix.diff

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

Looks good!  Sorry, after asking around I discovered that SetRef() can fail for some URIs.  So we do need to check the error there.  Can you please update and then I should be able to r+.

Thanks!

::: dom/fetch/FetchDriver.cpp
@@ +580,5 @@
>    } else {
>      mRequest->GetURL(reqURL);
>    }
> +  nsresult rv = aResponse->StripFragmentAndSetUrl(reqURL);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

I think this is fair.  The URL should already have been validated by this point.

::: dom/fetch/InternalResponse.cpp
@@ +99,5 @@
> +  if(NS_WARN_IF(NS_FAILED(rv))){
> +    return rv;
> +  }
> +
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(iuri->SetRef(EmptyCString())));

On further, though, I think maybe you should just check the error with:

  if(NS_WARN_IF(NS_FAILED(rv))){
    return rv;
  }

Its possible for the nsIURI object to be implemented by some plugin which might return an error in theory.  Sorry for the back and forth here.

::: dom/fetch/Request.cpp
@@ +64,5 @@
>        return;
>    }
>  
>    nsAutoCString spec;
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(resolvedURI->SetRef(EmptyCString())));

Sorry, these also need to handle the error.  You can do:

  aRv = resolvedURI->SetRef(EmptyCString()));
  if (NS_WARN_IF(aRv.Failed())) {
    return;
  }
Attachment #8635136 - Flags: review?(bkelly)
Great! I'm excited to get my first r+.

I'll be away for one week, in case anything else comes up I'll get working on it as soon as I get back.

Thanks for all the feedback.
Attachment #8635136 - Attachment is obsolete: true
Attachment #8635486 - Flags: review?(bkelly)
Comment on attachment 8635486 [details] [diff] [review]
bug1110476_FetchRequestResponseStripFragmentfix.diff

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

Looks great!  Thanks for working on this!

Do you have permission to push to the try server for a test run?
Attachment #8635486 - Flags: review?(bkelly) → review+
Hmm I don't think so I've never pushed to the try server.
No problem, I did one for you:

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

Basically the process for landing patches is to do a try-build on our tinderbox infrastructure and then mark the bug checkin-needed once its "green".

Here is some more info on the try servers:

  https://wiki.mozilla.org/Build:TryServer

To be able to push to try yourself you need to request level 1 commit access:

  https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/

If you CC me on the bug I'd be happy to vouch for you.

Overall its really handy to be able to use the try servers because they automatically build and run tests on all our platforms (mac, linux, window, android, b2g, etc).
Hi Antonio,

Can you look at the error in that try build?

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

Seems there's an unused rv variable now:

  /builds/slave/try-lx-00000000000000000000000/build/src/dom/fetch/FetchDriver.cpp:583:12: error: unused variable 'rv' [-Werror=unused-variable]

And some of the mochitests are failing:

  test_fetch_basic.html
  test_fetch_basic_sw_reroute.html
  test_sandbox_fetch.html
Status: NEW → ASSIGNED
Flags: needinfo?(tonydelun)
Hello Ben,

First of all sorry for taking to long to respond.

The unused rv variable was a simple fix, but I'm a bit lost as to why fetch('about:config') isn't failing.

I've looked at the spec and found this: "URLs such as "about:config" are handled during navigation and result in a network error in the context of fetching."

So I was wondering if you could point me to the files that handle navigation to have a look.

Thanks.
Flags: needinfo?(tonydelun) → needinfo?(bkelly)
(In reply to Antonio de Luna [:A_deLuna] from comment #29)
> I've looked at the spec and found this: "URLs such as "about:config" are
> handled during navigation and result in a network error in the context of
> fetching."
> 
> So I was wondering if you could point me to the files that handle navigation
> to have a look.

I think about:config should trigger an error here:

https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp?case=true&from=FetchDriver.cpp#209
Flags: needinfo?(bkelly)
When running dom/tests/mochitest/fetch/test_fetch_basic.html I'm getting the following warnings: 

>[8234] WARNING: NS_ENSURE_TRUE(mMutable) failed: file /home/tony/mozilla-central/netwerk/base/nsSimpleURI.cpp, line 389
>[8234] WARNING: 'aRv.Failed()', file /home/tony/mozilla-central/dom/fetch/Request.cpp, line 88
>[8234] WARNING: 'aRv.Failed()', file /home/tony/mozilla-central/dom/fetch/Request.cpp, line 213
>[8234] WARNING: 'aRv.Failed()', file /home/tony/mozilla-central/dom/fetch/Fetch.cpp, line 214

I believe that the problem is that calling fetch('about:config') crashes here https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Fetch.cpp#213, when creating the request and calling SetRef on it here: https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsSimpleURI.cpp#389

Since fetch crashes on line 213 it doesn't have the opportunity to create a FetchDriver here https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Fetch.cpp#249, thus not reporting the appropiate error from https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp?case=true&from=FetchDriver.cpp#209.

Could you please tell me more about what NS_ENSURE_TRUE(mMutable) is about and how could I work around this?

thanks a lot!
Flags: needinfo?(bkelly)
Hmm, I see.  That's tricky.

I think maybe we want to use nsIURI::CloneIgnoringRef() instead of SetRef().  This will create a new URI object, but will safely strip the ref even if the original URI is immutable.

Does that work?
Flags: needinfo?(bkelly) → needinfo?(tonydelun)
Thanks for the suggestion, CloneIgnoringRef worked perfectly. I'll try to push this to try also
Attachment #8635486 - Attachment is obsolete: true
Flags: needinfo?(tonydelun)
Attachment #8645976 - Flags: review?(bkelly)
Comment on attachment 8645976 [details] [diff] [review]
bug1110476_FetchRequestResponseStripFragmentfix.diff

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

Looks good, but I caught on last issue.  See the comment below.  Sorry I missed that before.

::: dom/fetch/Request.cpp
@@ +116,5 @@
>  
> +  nsCOMPtr<nsIURI> uriClone;
> +  // We use CloneIgnoringRef to strip away the fragment even if the original URI
> +  // is immutable.
> +  aRv = uri->CloneIgnoringRef(getter_AddRefs(uriClone));

Ah, I just realized the code you are modifying here is in an #ifdef DEBUG block.  When not DEBUG a simple string assignment is performed.  This used to be safe because no modifications were happening, but no longer works with the ref stripping.

Can you remove the #ifdef DEBUG and the #else clause?
Attachment #8645976 - Flags: review?(bkelly)
Don't worry about it,

I haven't been able to push this patch to try, could you please push it?
Attachment #8645976 - Attachment is obsolete: true
Attachment #8646129 - Flags: review?(bkelly)
Comment on attachment 8646129 [details] [diff] [review]
bug1110476_FetchRequestResponseStripFragmentfix.diff

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

Thanks!  r=me

Here is a try build:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=31653725b979
Attachment #8646129 - Flags: review?(bkelly) → review+
I'm not sure what how to interpret the try results, most errors seem unrelated. Is this ready for checkin?
Flags: needinfo?(bkelly)
They all look unrelated.  I think this is good to land.

Thanks!
Flags: needinfo?(bkelly)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a5c25038d81e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.