1431204 - Make nsIURI.spec readonly

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Comment 1

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

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

Comment 2

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

Attachment: Bug 1431204 - Change calls to nsIURI.spec setter to use nsIURIMutator instead

* changes call to use nsIURIMutator.setSpec()
* Add new NS_MutateURI constructor that takes new Mutator object
* Make nsSimpleNestedURI::Mutate() and nsNestedAboutURI::Mutate() return mutable URIs

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

Comment 3

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

Attachment: Bug 1431204 - Only addref the mutator if is not null

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

Comment 4

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

Attachment: Bug 1431204 - Make nsIURI.spec readonly

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

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8943428 [details]
Bug 1431204 - Change calls to nsIURI.spec setter to use nsIURIMutator instead

https://reviewboard.mozilla.org/r/213748/#review219780

::: dom/file/nsHostObjectProtocolHandler.cpp:896
(Diff revision 1)
>    nsresult rv;
>  
>    DataInfo* info = GetDataInfo(aSpec);
>  
> -  RefPtr<nsHostObjectURI> uri;
> +  nsCOMPtr<nsIURI> uri;
> +  rv = NS_MutateURI(new nsHostObjectURI::Mutator())

would it make sense and not be to much work to pass info->mPrincipal, info->mBlobImpl as args to this specific Mutator?  or have setter for it on the mutator?

::: dom/file/nsHostObjectProtocolHandler.cpp:901
(Diff revision 1)
> +  rv = NS_MutateURI(new nsHostObjectURI::Mutator())
> +         .SetSpec(aSpec)
> +         .Finalize(uri);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  RefPtr<nsHostObjectURI> hostURI = static_cast<nsHostObjectURI*>(uri.get());

that would nicely remove this static_cast which I'm a personal enemy of

::: dom/url/URLWorker.cpp:718
(Diff revision 1)
>    if (scheme.EqualsLiteral("http") || scheme.EqualsLiteral("https")) {
> -    mStdURL = new nsStandardURL();
> -    aRv = mStdURL->SetSpec(NS_ConvertUTF16toUTF8(aHref));
> +    nsCOMPtr<nsIURI> uri;
> +    aRv = NS_MutateURI(new nsStandardURL::Mutator())
> +            .SetSpec(NS_ConvertUTF16toUTF8(aHref))
> +            .Finalize(uri);
> +    mStdURL = static_cast<net::nsStandardURL*>(uri.get());

if this really must be nsStandardURL* (not sure it has to..) isn't there a templatable NS_MutateURI to work with and return a concrete class instead?

OTOH, NS_MutateURI takes nsIURIMutator which already returns nsIURI...  so, probably not.

::: netwerk/base/nsSimpleNestedURI.cpp:201
(Diff revision 1)
>      if (NS_FAILED(rv)) {
>          return rv;
>      }
> +    // StartClone calls SetMutable(false) but we need the mutator clone
> +    // to be mutable
> +    mutator->ResetMutable();

ok, and does the mutator call mURI->SetMutable(false) again in finish() so that we get the same result as before?

you would need to override it specifically for this case at 
https://searchfox.org/mozilla-central/rev/1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/netwerk/base/nsSimpleNestedURI.h#80

::: netwerk/protocol/about/nsAboutProtocolHandler.cpp:154
(Diff revision 1)
>          nsCOMPtr<nsIURI> inner;
>          rv = NS_NewURI(getter_AddRefs(inner), spec);
>          NS_ENSURE_SUCCESS(rv, rv);
>  
> -        nsSimpleNestedURI* outer = new nsNestedAboutURI(inner, aBaseURI);
> +        RefPtr<nsSimpleNestedURI> outer = new nsNestedAboutURI(inner, aBaseURI);
>          NS_ENSURE_TRUE(outer, NS_ERROR_OUT_OF_MEMORY);

maybe remove this check now?

::: netwerk/protocol/about/nsAboutProtocolHandler.cpp:437
(Diff revision 1)
>      if (NS_FAILED(rv)) {
>          return rv;
>      }
> +    // StartClone calls SetMutable(false) but we need the mutator clone
> +    // to be mutable
> +    mutator->ResetMutable();

probably same complain here as for simpleNestedURI setmutable(false)

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8943429 [details]
Bug 1431204 - Only addref the mutator if is not null

https://reviewboard.mozilla.org/r/213750/#review219794

::: caps/NullPrincipalURI.h:81
(Diff revision 1)
>        return NS_OK;
>      }
>  
>      NS_IMETHOD SetSpec(const nsACString & aSpec, nsIURIMutator** aMutator) override
>      {
> -      NS_ADDREF(*aMutator = this);
> +      if (aMutator) NS_ADDREF(*aMutator = this);

definitely 

if () {
}

and could we go via nsCOMPtr.forget() here?

::: image/decoders/icon/nsIconURI.h:73
(Diff revision 1)
>        mURI.forget(aURI);
>        return NS_OK;
>      }
>  
>      NS_IMETHOD SetSpec(const nsACString & aSpec, nsIURIMutator** aMutator) override {
> -      NS_ADDREF(*aMutator = this);
> +      if (aMutator) NS_ADDREF(*aMutator = this);

...

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8943430 [details]
Bug 1431204 - Make nsIURI.spec readonly

https://reviewboard.mozilla.org/r/213752/#review219798

Comment 8

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

Comment on attachment 8943428 [details]
Bug 1431204 - Change calls to nsIURI.spec setter to use nsIURIMutator instead

https://reviewboard.mozilla.org/r/213748/#review219894

::: dom/file/nsHostObjectProtocolHandler.cpp:896
(Diff revision 1)
>    nsresult rv;
>  
>    DataInfo* info = GetDataInfo(aSpec);
>  
> -  RefPtr<nsHostObjectURI> uri;
> +  nsCOMPtr<nsIURI> uri;
> +  rv = NS_MutateURI(new nsHostObjectURI::Mutator())

That would complicate the logic a bit - the mutator having to keep some resources around, and set it on the final URI just before we finalize it.
IMO this is clear and easy to follow, and I'll likely change it when I get around to making nsHostObjectURI immutable.

Comment 9

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

Comment on attachment 8943428 [details]
Bug 1431204 - Change calls to nsIURI.spec setter to use nsIURIMutator instead

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

Comment 10

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

Comment on attachment 8943429 [details]
Bug 1431204 - Only addref the mutator if is not null

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

Comment 11

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

Comment on attachment 8943430 [details]
Bug 1431204 - Make nsIURI.spec readonly

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

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #8)
> Comment on attachment 8943428 [details]
> Bug 1431204 - Change calls to nsIURI.spec setter to use nsIURIMutator instead
> 
> https://reviewboard.mozilla.org/r/213748/#review219894
> 
> ::: dom/file/nsHostObjectProtocolHandler.cpp:896
> (Diff revision 1)
> >    nsresult rv;
> >  
> >    DataInfo* info = GetDataInfo(aSpec);
> >  
> > -  RefPtr<nsHostObjectURI> uri;
> > +  nsCOMPtr<nsIURI> uri;
> > +  rv = NS_MutateURI(new nsHostObjectURI::Mutator())
> 
> That would complicate the logic a bit - the mutator having to keep some
> resources around, and set it on the final URI just before we finalize it.
> IMO this is clear and easy to follow, and I'll likely change it when I get
> around to making nsHostObjectURI immutable.

OK.  I somehow thought that new nsHostObjectURI::Mutator() was creating the new URI right away.  But it's call to SetSpec that does it (forwards to the mutator ending up at [1]).

Some kind of NS_CreateURI(mutator*, ...) or NS_MutateURI(mutator*).Create(...) would be nice to have, but that might be just an overkill.  Note that the '...' means varargs.

[1] https://searchfox.org/mozilla-central/rev/2031c0f517185b2bd0e8f6f92f9491b3410c1f7f/netwerk/base/nsIURIMutator.idl#75

Comment 13

[Honza Bambas (:mayhemer)]

Hmm.. could you give the mutator a constructed URI to mutate?  Like:

NS_MutateURI(new nsHostObjectURI::Mutator(), new nsHostObjectURI(bla, bla)).SetSpec(..) ?

Comment 14

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

(In reply to Honza Bambas (:mayhemer) from comment #13)
> Hmm.. could you give the mutator a constructed URI to mutate?  Like:
> 
> NS_MutateURI(new nsHostObjectURI::Mutator(), new nsHostObjectURI(bla,
> bla)).SetSpec(..) ?

My concern with this is that you could also call this API in the following way:

```
NS_MutateURI(new nsHostObjectURI::Mutator(), new nsHostObjectURI(bla, bla)).Finalize(uri)
```

In this case the `uri` will be a newly constructed nsHostObjectURI, but without setting its spec, it's not really a valid one.

The way to fix this is by having nsHostObjectURI::Mutator extend nsIBlobMutator that has a method to set the blob. And similar for the principal. 

I filed bug 1431760 to do this for nsHostObjectURI. I need to do it for all other interfaces extended by URI implementations anyway:
https://gist.github.com/16de067dea8c507d4a33addf74ebdb13

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 8943428 [details]
Bug 1431204 - Change calls to nsIURI.spec setter to use nsIURIMutator instead

https://reviewboard.mozilla.org/r/213748/#review220028

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24e867a67e1f
Change calls to nsIURI.spec setter to use nsIURIMutator instead r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/4070ee4b8315
Only addref the mutator if is not null r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/318a2d06a2d0
Make nsIURI.spec readonly r=mayhemer

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/24e867a67e1f
https://hg.mozilla.org/mozilla-central/rev/4070ee4b8315
https://hg.mozilla.org/mozilla-central/rev/318a2d06a2d0