1394906 - Implement immutable, threadsafe MozURL

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[Valentin Gosu [:valentin] (he/him)]

This would be the first stage towards OMT-nsIURI

The second stage would mean centralizing the URL parsing, and creating a wrapper for MozURL that also implements all the nsIURI/nsIURL interfaces, so that it's usable from JS.

However, this would be a step forward for code that is currently using strings to keep URLs OMT, and have to bounce to the main thread and parse it again in order to change it in any way.

MozURL is usable in all cpp code, is backed by rust-url, and is immutable by default.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1394906 - Implement immutable, threadsafe MozURL

Also adds the necessary methods to rust-url-capi and fixes a bug in rusturl_set_username

Review commit: https://reviewboard.mozilla.org/r/173924/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173924/

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Note on the performance of rust-url compared to that of nsStandardURL:
I did some micro-benchmarking, and it seems that while the rust-url getters are about as fast as nsStandardURL, the setters are a bit or much slower for rust-url:
    - setHost is about 60x slower
    - setPath is about 3x slower
    - setQuery is about 5x slower
    - setFragment is about 4x slower

I think this is mostly due to the extra processing we do on the input in rust-url, also because that extra processing sometimes allocates a bunch of tiny strings. There is definitely room for improvement.

Comment 3

[Simon Sapin (:SimonSapin)]

There’s definitely low-hanging fruit, in particular in the IDNA code.

Comment 4

[Gary Chen [:xeonchen]]

Comment on attachment 8902358 [details]
Bug 1394906 - Implement immutable, threadsafe MozURL

https://reviewboard.mozilla.org/r/173924/#review179332

::: netwerk/base/MozURL.h:21
(Diff revision 1)
> +// The constructor is private. One can instantiate the object by
> +// calling the Init() method as such:
> +//
> +// RefPtr<MozURL> url;
> +// nsAutoCString href("http://example.com/path?query#ref");
> +// nsresult rv = MozURL::Init(, getter_AddRefs(url));

don't forget |href|

Comment 5

[Firefox Bug Husbandry Bot]

Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 6

[Valentin Gosu [:valentin] (he/him)]

review ping

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8902358 [details]
Bug 1394906 - Implement immutable, threadsafe MozURL

https://reviewboard.mozilla.org/r/173924/#review191882

Nice piece of work :)

::: netwerk/base/MozURL.h:28
(Diff revision 1)
> +//
> +// When changing the URL is needed, you need to call the Mutate() method.
> +// This gives you a Mutator object, on which you can perform setter operations.
> +// Calling Finalize() on the Mutator will result in a new MozURL and a status
> +// code. If any of the setter operations failed, it will be reflected in the
> +// status code, and a null MozURL.

thanks, this is the API I had in mind :)

::: netwerk/base/MozURL.h:29
(Diff revision 1)
> +// When changing the URL is needed, you need to call the Mutate() method.
> +// This gives you a Mutator object, on which you can perform setter operations.
> +// Calling Finalize() on the Mutator will result in a new MozURL and a status
> +// code. If any of the setter operations failed, it will be reflected in the
> +// status code, and a null MozURL.
> +class MozURL : public nsISupports

why ISupports and not just a general object with AddRef/Release implemented inline?  if this is going to be carried via nsISupports* arguments, then we should have QueryObject to get MozURI back.  Or rather not implement nsISupports at all and always pass MozURI* or RefPtr<MozURI>&

what do you think?

::: netwerk/base/MozURL.h:72
(Diff revision 1)
> +    // returned here. It will also return an error code if Finalize is called
> +    // more than once on the Mutator.
> +    nsresult Finalize(MozURL** aURL);
> +
> +    // These setter methods will return a reference to `this` so that you may
> +    // chain setter operations as such:

yummy..:)

::: netwerk/base/MozURL.h:116
(Diff revision 1)
> +    bool mFinalized;
> +    nsresult mStatus;
> +    friend class MozURL;
> +  };
> +
> +  Mutator Mutate() { return Mutator(this); }

can we force use of a Mutator's move ctor (do we need to define it?) for the result?  I don't know the syntax or if this is enough.. :'(

::: netwerk/base/MozURL.cpp:85
(Diff revision 1)
> +// This macro ensures that the mutator is still valid, meaning it hasn't been
> +// finalized, and none of the setters have returned an error code.
> +#define ENSURE_VALID()                          \
> +  PR_BEGIN_MACRO                                \
> +    if (mFinalized) {                           \
> +        mStatus = NS_ERROR_NOT_AVAILABLE;       \

nit: indent

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8902358 [details]
Bug 1394906 - Implement immutable, threadsafe MozURL

https://reviewboard.mozilla.org/r/173924/#review192244

::: netwerk/base/MozURL.h:36
(Diff revision 1)
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +
> +  static nsresult Init(const nsACString& aSpec, MozURL** aURL);
> +
> +  nsresult GetScheme(nsACString& aScheme);
> +  nsresult GetSpec(nsACString& aSpec);

btw, could we add comments on which form is used to return the strings here?  is it the same as GetAsciiSpec works?  or is it UTF-8 encoded?  or some other form like punycode or a specific charset?


Which also opens the question - are we still keeping and working with a charset on MozURI?

Comment 9

[Valentin Gosu [:valentin] (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #8)
> > +  nsresult GetScheme(nsACString& aScheme);
> > +  nsresult GetSpec(nsACString& aSpec);
> 
> btw, could we add comments on which form is used to return the strings here?
> is it the same as GetAsciiSpec works?  or is it UTF-8 encoded?  or some
> other form like punycode or a specific charset?
> 
> Which also opens the question - are we still keeping and working with a
> charset on MozURI?

Technically it should only return ascii. Maybe we should change the name to reflect that?
Also, it doesn't work with any other charset apart from utf-8. I'll add comments to signal that.

Comment 10

[Masatoshi Kimura [:emk]]

We are no longer keeping a charset even on nsIURI. (bug 1322874)

Comment 11

[Valentin Gosu [:valentin] (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> should have QueryObject to get MozURI back.  Or rather not implement
> nsISupports at all and always pass MozURI* or RefPtr<MozURI>&
> 
> what do you think?

Agreed.

> > +  Mutator Mutate() { return Mutator(this); }
> 
> can we force use of a Mutator's move ctor (do we need to define it?) for the
> result?  I don't know the syntax or if this is enough.. :'(

I think we can let the compiler use RVO to optimize this.

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8902358 [details]
Bug 1394906 - Implement immutable, threadsafe MozURL

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173924/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/125125b32aaa
Implement immutable, threadsafe MozURL r=mayhemer

Comment 14

[Code Review Bot [:reviewbot]]

Comment on attachment 8902358 [details]
Bug 1394906 - Implement immutable, threadsafe MozURL

https://reviewboard.mozilla.org/r/173924/#review192530


C/C++ static analysis found 10 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: netwerk/base/MozURL.h:52
(Diff revision 2)
> +  nsresult GetFilePath(nsACString& aPath);
> +  nsresult GetQuery(nsACString& aQuery);
> +  nsresult GetRef(nsACString& aRef);
> +
> +private:
> +  MozURL(rusturl* rawPtr)

Error: Bad implicit conversion constructor for 'mozurl' [clang-tidy: mozilla-implicit-constructor]

::: netwerk/base/MozURL.h:107
(Diff revision 2)
> +    //   rv = mut.SetHostname(host).Finalize(getter_AddRefs(url2));
> +    // }
> +    // if (NS_SUCCEEDED(rv)) { /* use url2 */ }
> +    nsresult GetStatus() { return mStatus; }
> +  private:
> +    Mutator(MozURL* url)

Error: Bad implicit conversion constructor for 'mutator' [clang-tidy: mozilla-implicit-constructor]

::: netwerk/test/gtest/TestMozURL.cpp:9
(Diff revision 2)
> +#include "nsCOMPtr.h"
> +#include "../../base/MozURL.h"
> +
> +using namespace mozilla::net;
> +
> +TEST(TestMozURL, Getters)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: netwerk/test/gtest/TestMozURL.cpp:9
(Diff revision 2)
> +#include "nsCOMPtr.h"
> +#include "../../base/MozURL.h"
> +
> +using namespace mozilla::net;
> +
> +TEST(TestMozURL, Getters)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: netwerk/test/gtest/TestMozURL.cpp:47
(Diff revision 2)
> +  ASSERT_EQ(MozURL::Init(NS_LITERAL_CSTRING(""), getter_AddRefs(url)),
> +            NS_ERROR_FAILURE);
> +  ASSERT_EQ(url, nullptr);
> +}
> +
> +TEST(TestMozURL, MutatorChain)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: netwerk/test/gtest/TestMozURL.cpp:47
(Diff revision 2)
> +  ASSERT_EQ(MozURL::Init(NS_LITERAL_CSTRING(""), getter_AddRefs(url)),
> +            NS_ERROR_FAILURE);
> +  ASSERT_EQ(url, nullptr);
> +}
> +
> +TEST(TestMozURL, MutatorChain)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: netwerk/test/gtest/TestMozURL.cpp:68
(Diff revision 2)
> +
> +  ASSERT_EQ(url2->GetSpec(out), NS_OK);
> +  ASSERT_TRUE(out.EqualsLiteral("https://newuser:newpass@test/new/file/path?bla#huh"));
> +}
> +
> +TEST(TestMozURL, MutatorFinalizeTwice)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: netwerk/test/gtest/TestMozURL.cpp:68
(Diff revision 2)
> +
> +  ASSERT_EQ(url2->GetSpec(out), NS_OK);
> +  ASSERT_TRUE(out.EqualsLiteral("https://newuser:newpass@test/new/file/path?bla#huh"));
> +}
> +
> +TEST(TestMozURL, MutatorFinalizeTwice)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: netwerk/test/gtest/TestMozURL.cpp:88
(Diff revision 2)
> +  url2 = nullptr;
> +  ASSERT_EQ(mut.Finalize(getter_AddRefs(url2)), NS_ERROR_NOT_AVAILABLE);
> +  ASSERT_EQ(url2, nullptr);
> +}
> +
> +TEST(TestMozURL, MutatorErrorStatus)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: netwerk/test/gtest/TestMozURL.cpp:88
(Diff revision 2)
> +  url2 = nullptr;
> +  ASSERT_EQ(mut.Finalize(getter_AddRefs(url2)), NS_ERROR_NOT_AVAILABLE);
> +  ASSERT_EQ(url2, nullptr);
> +}
> +
> +TEST(TestMozURL, MutatorErrorStatus)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for static failures:

https://hg.mozilla.org/integration/autoland/rev/aca4a023f4df2a2293c5c31ae93aa86eba1ffaaa

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=125125b32aaa45acc2c8fbd69b4236919ea414d5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=135494328&repo=autoland

[task 2017-10-07T09:48:23.490Z] 09:48:23     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/netwerk/test/gtest/Unified_cpp_netwerk_test_gtest0.cpp:20:
[task 2017-10-07T09:48:23.490Z] 09:48:23     INFO -  In file included from /builds/worker/workspace/build/src/netwerk/test/gtest/TestMozURL.cpp:5:
[task 2017-10-07T09:48:23.490Z] 09:48:23     INFO -  /builds/worker/workspace/build/src/netwerk/test/gtest/../../base/MozURL.h:52:3: error: bad implicit conversion constructor for 'MozURL'
[task 2017-10-07T09:48:23.490Z] 09:48:23     INFO -    MozURL(rusturl* rawPtr)
[task 2017-10-07T09:48:23.490Z] 09:48:23     INFO -    ^
[task 2017-10-07T09:48:23.491Z] 09:48:23     INFO -  /builds/worker/workspace/build/src/netwerk/test/gtest/../../base/MozURL.h:52:3: note: consider adding the explicit keyword to the constructor
[task 2017-10-07T09:48:23.491Z] 09:48:23     INFO -  /builds/worker/workspace/build/src/netwerk/test/gtest/../../base/MozURL.h:107:5: error: bad implicit conversion constructor for 'Mutator'
[task 2017-10-07T09:48:23.492Z] 09:48:23     INFO -      Mutator(MozURL* url)
[task 2017-10-07T09:48:23.492Z] 09:48:23     INFO -      ^
[task 2017-10-07T09:48:23.493Z] 09:48:23     INFO -  /builds/worker/workspace/build/src/netwerk/test/gtest/../../base/MozURL.h:107:5: note: consider adding the explicit keyword to the constructor

Comment 16

[Valentin Gosu [:valentin] (he/him)]

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

Comment 17

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8902358 [details]
Bug 1394906 - Implement immutable, threadsafe MozURL

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173924/diff/2-3/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89256faa37a9
Implement immutable, threadsafe MozURL r=mayhemer

Comment 19

[Code Review Bot [:reviewbot]]

Comment on attachment 8902358 [details]
Bug 1394906 - Implement immutable, threadsafe MozURL

https://reviewboard.mozilla.org/r/173924/#review192546


C/C++ static analysis found 8 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: netwerk/test/gtest/TestMozURL.cpp:9
(Diff revision 3)
> +#include "nsCOMPtr.h"
> +#include "../../base/MozURL.h"
> +
> +using namespace mozilla::net;
> +
> +TEST(TestMozURL, Getters)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: netwerk/test/gtest/TestMozURL.cpp:9
(Diff revision 3)
> +#include "nsCOMPtr.h"
> +#include "../../base/MozURL.h"
> +
> +using namespace mozilla::net;
> +
> +TEST(TestMozURL, Getters)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: netwerk/test/gtest/TestMozURL.cpp:47
(Diff revision 3)
> +  ASSERT_EQ(MozURL::Init(NS_LITERAL_CSTRING(""), getter_AddRefs(url)),
> +            NS_ERROR_FAILURE);
> +  ASSERT_EQ(url, nullptr);
> +}
> +
> +TEST(TestMozURL, MutatorChain)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: netwerk/test/gtest/TestMozURL.cpp:47
(Diff revision 3)
> +  ASSERT_EQ(MozURL::Init(NS_LITERAL_CSTRING(""), getter_AddRefs(url)),
> +            NS_ERROR_FAILURE);
> +  ASSERT_EQ(url, nullptr);
> +}
> +
> +TEST(TestMozURL, MutatorChain)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: netwerk/test/gtest/TestMozURL.cpp:68
(Diff revision 3)
> +
> +  ASSERT_EQ(url2->GetSpec(out), NS_OK);
> +  ASSERT_TRUE(out.EqualsLiteral("https://newuser:newpass@test/new/file/path?bla#huh"));
> +}
> +
> +TEST(TestMozURL, MutatorFinalizeTwice)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: netwerk/test/gtest/TestMozURL.cpp:68
(Diff revision 3)
> +
> +  ASSERT_EQ(url2->GetSpec(out), NS_OK);
> +  ASSERT_TRUE(out.EqualsLiteral("https://newuser:newpass@test/new/file/path?bla#huh"));
> +}
> +
> +TEST(TestMozURL, MutatorFinalizeTwice)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: netwerk/test/gtest/TestMozURL.cpp:88
(Diff revision 3)
> +  url2 = nullptr;
> +  ASSERT_EQ(mut.Finalize(getter_AddRefs(url2)), NS_ERROR_NOT_AVAILABLE);
> +  ASSERT_EQ(url2, nullptr);
> +}
> +
> +TEST(TestMozURL, MutatorErrorStatus)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: netwerk/test/gtest/TestMozURL.cpp:88
(Diff revision 3)
> +  url2 = nullptr;
> +  ASSERT_EQ(mut.Finalize(getter_AddRefs(url2)), NS_ERROR_NOT_AVAILABLE);
> +  ASSERT_EQ(url2, nullptr);
> +}
> +
> +TEST(TestMozURL, MutatorErrorStatus)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/89256faa37a9