Closed Bug 1431204 Opened 6 years ago Closed 6 years ago

Make nsIURI.spec readonly

Categories

(Core :: Networking, enhancement, P2)

Product:
Core
Component:
Networking
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(3 files)

Bug 1431204 - Change calls to nsIURI.spec setter to use nsIURIMutator instead
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1431204 - Only addref the mutator if is not null
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1431204 - Make nsIURI.spec readonly
59 bytes, text/x-review-board-request
mayhemer
: review+
Details 
      No description provided.
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 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);

...
Attachment #8943429 - Flags: review?(honzab.moz) → review+
Comment on attachment 8943430 [details]
Bug 1431204 - Make nsIURI.spec readonly

https://reviewboard.mozilla.org/r/213752/#review219798
Attachment #8943430 - Flags: review?(honzab.moz) → review+
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.
(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
Hmm.. could you give the mutator a constructed URI to mutate?  Like:

NS_MutateURI(new nsHostObjectURI::Mutator(), new nsHostObjectURI(bla, bla)).SetSpec(..) ?
(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 on attachment 8943428 [details]
Bug 1431204 - Change calls to nsIURI.spec setter to use nsIURIMutator instead

https://reviewboard.mozilla.org/r/213748/#review220028
Attachment #8943428 - Flags: review?(honzab.moz) → review+
Pushed by valentin.gosu@gmail.com:
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
Depends on: 1433801
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: