1442242 - Make the constructos of URI implementations private

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

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

Not allowing code to directly instantiate the URI implementations will help with ensuring that no mutation is possible, and that URIs are always in a defined state when returned by the mutators.

Comment 1

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

Attachment: Bug 1442242 - Add nsIURIMutator method to mark when the URI implementation should support nsIFileURL

* This is needed in order to make the constructors of URI implementations private

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

Comment 2

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

Attachment: Bug 1442242 - Make the constructor of nsJARURI private

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

Comment 3

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

Attachment: Bug 1442242 - Make the constuctors of URI implementations private

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

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 8957171 [details]
Bug 1442242 - Add nsIURIMutator method to mark when the URI implementation should support nsIFileURL

https://reviewboard.mozilla.org/r/226112/#review234256

::: netwerk/base/nsStandardURL.h:467
(Diff revision 1)
>                  mutator.forget(aMutator);
>              }
>              return BaseURIMutator<T>::mURI->SetFileExtensionInternal(aFileExtension);
>          }
>  
> +        virtual T* Create() override

nit: we should not use virtual and override together anymore

::: netwerk/protocol/file/nsFileProtocolHandler.cpp:259
(Diff revision 1)
>  nsFileProtocolHandler::NewFileURIMutator(nsIFile *aFile, nsIURIMutator **aResult)
>  {
>      NS_ENSURE_ARG_POINTER(aFile);
>      nsresult rv;
>  
> -    nsCOMPtr<nsIURI> url = new nsStandardURL(true);
> +    nsCOMPtr<nsIURIMutator> mutator = new nsStandardURL::Mutator();

was the |true| argument lost here?

::: netwerk/protocol/res/SubstitutingProtocolHandler.h:140
(Diff revision 1)
>    public:
>      explicit Mutator() = default;
>    private:
>      virtual ~Mutator() = default;
> +
> +    virtual SubstitutingURL* Create() override

again, remove virtual

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8957172 [details]
Bug 1442242 - Make the constructor of nsJARURI private

https://reviewboard.mozilla.org/r/226114/#review234260

::: modules/libjar/nsIJARURI.idl:44
(Diff revision 1)
> +
> +[builtinclass, uuid(d66df117-eda7-4324-b4e4-1f670ff6718e)]
> +interface nsIJARURIMutator : nsISupports
> +{
> +  void setSpecBaseCharset(in AUTF8String aSpec, in nsIURI aBase, in string aCharset);
> +};

comments on any new interface please

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8957173 [details]
Bug 1442242 - Make the constuctors of URI implementations private

https://reviewboard.mozilla.org/r/226116/#review234310

::: netwerk/base/nsSimpleNestedURI.h:34
(Diff revision 1)
>  class nsSimpleNestedURI : public nsSimpleURI,
>                            public nsINestedURI
>  {
>  protected:
> -    ~nsSimpleNestedURI() {}
> -
> +    ~nsSimpleNestedURI() = default;
> +    nsSimpleNestedURI() = default;

ctor first?

::: widget/tests/unit/test_taskbar_jumplistitems.js
(Diff revision 1)
>    Assert.equal(link.uriHash, "iSx6UH1a9enVPzUA9JZ42g==");
> -
> -  var uri3 = Cc["@mozilla.org/network/simple-uri;1"]
> -            .createInstance(Ci.nsIURI);
> -  link.uri = uri3;
> -  Assert.equal(link.uriHash, "hTrpDwNRMkvXPqYV5kh1Fw==");

why this removal?

Comment 7

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

Comment on attachment 8957171 [details]
Bug 1442242 - Add nsIURIMutator method to mark when the URI implementation should support nsIFileURL

https://reviewboard.mozilla.org/r/226112/#review234256

> was the |true| argument lost here?

It wasn't needed. Calling nsStandardURL::Mutator::SetFile will create a new nsStandardURL(true)

Comment 8

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

Comment on attachment 8957173 [details]
Bug 1442242 - Make the constuctors of URI implementations private

https://reviewboard.mozilla.org/r/226116/#review234310

> why this removal?

This computes the hash of an uninitialized instance of nsSimpleURI. We don't allow that anymore.

Comment 9

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

Comment on attachment 8957171 [details]
Bug 1442242 - Add nsIURIMutator method to mark when the URI implementation should support nsIFileURL

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

Comment 10

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

Comment on attachment 8957172 [details]
Bug 1442242 - Make the constructor of nsJARURI private

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

Comment 11

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

Comment on attachment 8957173 [details]
Bug 1442242 - Make the constuctors of URI implementations private

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

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8957171 [details]
Bug 1442242 - Add nsIURIMutator method to mark when the URI implementation should support nsIFileURL

https://reviewboard.mozilla.org/r/226112/#review234678

should we mark some of the mutator classes as final?  (not in this bug, probably)

::: netwerk/base/nsIFileURL.idl:33
(Diff revision 2)
>  
>  [scriptable, builtinclass, uuid(a588b6f2-d2b9-4024-84c7-be3368546b57)]
>  interface nsIFileURLMutator : nsISupports
>  {
>      /*
> +     *  - Marks the inner URI implementation as one that supports nsIFileURL.

there should be a comment this has to be used as the very first call on the mutator, otherwise the information will not be applied (the uri will already be created w/ the flag=false)

::: netwerk/protocol/file/nsFileProtocolHandler.cpp:260
(Diff revision 2)
> -    nsCOMPtr<nsIURIMutator> mutator;
> -    rv = url->Mutate(getter_AddRefs(mutator));
> -    if (NS_FAILED(rv)) {
> -        return rv;
> -    }
>      nsCOMPtr<nsIFileURLMutator> fileMutator = do_QueryInterface(mutator, &rv);

I think you can:

nsCOMPtr<nsIFileURLMutator> fileMutator = new nsStandardURL::Mutator();

?

Comment 13

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

Comment on attachment 8957171 [details]
Bug 1442242 - Add nsIURIMutator method to mark when the URI implementation should support nsIFileURL

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

Comment 14

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

Comment on attachment 8957172 [details]
Bug 1442242 - Make the constructor of nsJARURI private

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

Comment 15

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

Comment on attachment 8957173 [details]
Bug 1442242 - Make the constuctors of URI implementations private

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88130a542fb3
Add nsIURIMutator method to mark when the URI implementation should support nsIFileURL r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/65b8727bfe76
Make the constructor of nsJARURI private r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/9f37df3c62e1
Make the constuctors of URI implementations private r=mayhemer

Comment 17

[Noemi Erli[:noemi_erli]]

Backed out 3 changesets (bug 1442242) for fatal error LNK1120: 1 unresolved externals bustages on a CLOSED TREE
Push with failures: 
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9f37df3c62e1c821d646cde969a278357741ba2b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&filter-resultStatus=running
Log failure example:
https://treeherder.mozilla.org/logviewer.html#?job_id=169206220&repo=autoland&lineNumber=38052
Backout: 
https://hg.mozilla.org/integration/autoland/rev/daef726185bbd890c5801641c7b103910778b64f

Comment 18

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

Comment on attachment 8957173 [details]
Bug 1442242 - Make the constuctors of URI implementations private

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

Comment 19

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

Comment on attachment 8957173 [details]
Bug 1442242 - Make the constuctors of URI implementations private

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226116/diff/4-5/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12eb3cffb3a5
Add nsIURIMutator method to mark when the URI implementation should support nsIFileURL r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/60b94194d25c
Make the constructor of nsJARURI private r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/6e828540dc35
Make the constuctors of URI implementations private r=mayhemer

Comment 21

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/12eb3cffb3a5
https://hg.mozilla.org/mozilla-central/rev/60b94194d25c
https://hg.mozilla.org/mozilla-central/rev/6e828540dc35