Change nsIURL setters to use nsIURLMutator instead

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: valentin, Assigned: valentin)

Tracking

(Blocks 1 bug)

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(2 attachments)

nsIURL.{directory,fileName,fileBaseName,fileExtension} will be changed to use a new nsIURLMutator

However, it seems there is no nsIURL.directory implementation that actually does anything, so I will remove it.
Do we use them at all?
(In reply to Masatoshi Kimura [:emk] from comment #1)
> Do we use them at all?

There are only a few of them. Mostly where nsWebBrowserPersist calls SetFileName().
The getters do have a handful of in a bunch of places. If anyone is interested in removing them as a followup, that would be great.
(Assignee)

Updated

a year ago
Blocks: 1432519
Comment hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
Comment on attachment 8944772 [details]
Bug 1432320 - (Part 2) Change calls to nsIURL.{fileName,fileBaseName,fileExtension} setters to use nsIURLMutator

https://reviewboard.mozilla.org/r/214928/#review220562


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


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


::: netwerk/test/unit/test_standardurl.js:324
(Diff revision 1)
> +    { method: "setFileExtension", qi: Ci.nsIURLMutator },
> +    { method: "setFileBaseName", qi: Ci.nsIURLMutator },
> +  ];
> +
> +  for (let prop of setters) {
> +    dump("Testing '"+ prop.method + "'\n");

Error: Infix operators must be spaced. [eslint: space-infix-ops]
(In reply to Valentin Gosu [:valentin] from comment #2)
> There are only a few of them. Mostly where nsWebBrowserPersist calls
> SetFileName().
> The getters do have a handful of in a bunch of places. If anyone is
> interested in removing them as a followup, that would be great.

Apparently netwerk/test/unit/test_standardurl.js is the only JS caller (both in m-c and c-c), so we don' have to expose nsIURLMutator to JS, I think.
In addition, we can also remove SetFileBaseName because no one except nested URI implementations use it.

Comment 8

a year ago
mozreview-review
Comment on attachment 8944772 [details]
Bug 1432320 - (Part 2) Change calls to nsIURL.{fileName,fileBaseName,fileExtension} setters to use nsIURLMutator

https://reviewboard.mozilla.org/r/214928/#review220890
Attachment #8944772 - Flags: review?(honzab.moz) → review+

Comment 9

a year ago
mozreview-review
Comment on attachment 8944771 [details]
Bug 1432320 - (Part 1) Add nsIURLMutator

https://reviewboard.mozilla.org/r/214926/#review220882

::: modules/libjar/nsJARURI.cpp:729
(Diff revision 1)
>  {
> -    return mJAREntry->SetFileName(fileName);
> +    return NS_MutateURI(mJAREntry)
> +             .Apply<nsIURLMutator>(&nsIURLMutator::SetFileName,
> +                                   nsCString(fileName),
> +                                   nullptr)
> +             .Finalize(mJAREntry);

I looked at "Bug 1432519 - Make nsIURL attributes readonly" first (the next in line patch) and this comment may more belonged there, but I'll write it here, since not much is being changed in that next patch anyway:

so if this has to be threadsafe (based on nsJARURI implementing t-safe refcnt) then who protects mJAREntry member from concurrent access?  the reassign, or more likely the whole mutation operation (so that two threads can't mutate the same source), must be mutexed.


applies everywhere where you access mJAREntry...

could be a followup, tho

::: netwerk/base/nsIURL.idl:7
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsIURI.idl"
> +interface nsIURIMutator;

why this predeclar?
(In reply to Honza Bambas (:mayhemer) from comment #9)
> > +    return NS_MutateURI(mJAREntry)
> > +             .Apply<nsIURLMutator>(&nsIURLMutator::SetFileName,
> > +                                   nsCString(fileName),
> > +                                   nullptr)
> > +             .Finalize(mJAREntry);
> 
> I looked at "Bug 1432519 - Make nsIURL attributes readonly" first (the next
> in line patch) and this comment may more belonged there, but I'll write it
> here, since not much is being changed in that next patch anyway:
> 
> so if this has to be threadsafe (based on nsJARURI implementing t-safe
> refcnt) then who protects mJAREntry member from concurrent access?  the
> reassign, or more likely the whole mutation operation (so that two threads
> can't mutate the same source), must be mutexed.
> 
> 
> applies everywhere where you access mJAREntry...

NS_MutateURI clones the original URI, and only mutates that clone.
Once we get rid of all mutators, all nsIURI objects should not change after being finalized, to it's OK to read them on any thread.

> could be a followup, tho
> 
> ::: netwerk/base/nsIURL.idl:7
> > +interface nsIURIMutator;
> 
> why this predeclar?

Because nsIURLMutator.setFileName returns nsIURIMutator
(In reply to Valentin Gosu [:valentin] from comment #10)
> (In reply to Honza Bambas (:mayhemer) from comment #9)
> > > +    return NS_MutateURI(mJAREntry)
> > > +             .Apply<nsIURLMutator>(&nsIURLMutator::SetFileName,
> > > +                                   nsCString(fileName),
> > > +                                   nullptr)
> > > +             .Finalize(mJAREntry);
> > 
> > I looked at "Bug 1432519 - Make nsIURL attributes readonly" first (the next
> > in line patch) and this comment may more belonged there, but I'll write it
> > here, since not much is being changed in that next patch anyway:
> > 
> > so if this has to be threadsafe (based on nsJARURI implementing t-safe
> > refcnt) then who protects mJAREntry member from concurrent access?  the
> > reassign, or more likely the whole mutation operation (so that two threads
> > can't mutate the same source), must be mutexed.
> > 
> > 
> > applies everywhere where you access mJAREntry...
> 
> NS_MutateURI clones the original URI, and only mutates that clone.
> Once we get rid of all mutators, all nsIURI objects should not change after
> being finalized, to it's OK to read them on any thread.

This is not my only point at all, I'm mainly taking about the member access.  explanation:

mJAREntry.filename == "foo";
mJAREntry.extension = "ext1";

T1: clone mJAREntry -> clone.filename = "foo", clone.extension = "ext1"
T1: setFileName("bar") -> clone.filename = "bar", clone.extension = "ext1"
(now assignment to mJAREntry inside finalize(), broken to atomics): 
T1: auto local = mJAREntry.mRawPtr

ctx switch
    
T2: clone mJAREntry -> clone.filename = "foo", clone.extension = "ext1"
T2: setFileExtension("ext2") -> clone.filename = "foo", clone.extension = "ext2" // this removes the filename update...
T2: auto local = mJAREntry.mRawPtr // this is still the first one!
T2: clone.addref() // this will leak, since we will rewrite mJAREntry.mRawPtr on the other thread soon
T2: mJAREntry.mRawPtr = clone
T2: local.release() // first release

ctx switch

T1: clone.addref()
T1: mJAREntry.mRawPtr = clone // throws away the second clone and reverts the extension change
T1: local.release() // double release

result:
broken heap, and
mJAREntry.filename == "bar";
mJAREntry.extension = "ext1";


in other words - https://www.janbambas.cz/illusion-atomic-reference-counting/
Oh, right. This could indeed be a problem, just not in this case, as nsJARURI would also be immutable.
So the call to nsJARURI::SetFileName would be also be applied to a cloned nsJARURI, different from what the other thread might have.

The problem you mention can happen however, if you have an
nsCOMPtr<nsIURI> mURI; in some object A
If you mutate A.mURI and assign the result also to A.mURI there is a race. In that case a mutex should be used.
(In reply to Valentin Gosu [:valentin] from comment #12)
> Oh, right. This could indeed be a problem, just not in this case, as
> nsJARURI would also be immutable.
> So the call to nsJARURI::SetFileName would be also be applied to a cloned
> nsJARURI, different from what the other thread might have.
> 
> The problem you mention can happen however, if you have an
> nsCOMPtr<nsIURI> mURI; in some object A
> If you mutate A.mURI and assign the result also to A.mURI there is a race.
> In that case a mutex should be used.

What is your plan on moving forward here?
Flags: needinfo?(valentin.gosu)
I was intending to land this as it is. The code isn't any more thread-unsafe than the previous code was.
When every setter method is removed, then there should be no problem anymore.
Flags: needinfo?(valentin.gosu)

Comment 15

a year ago
mozreview-review
Comment on attachment 8944771 [details]
Bug 1432320 - (Part 1) Add nsIURLMutator

https://reviewboard.mozilla.org/r/214926/#review222340

OK, let's land this.  as a side work, could we add some thread assertions to these methods and push to try and see?  if we crash on potential concurrency, I would at least open a bug for it to prioritize.
Attachment #8944771 - Flags: review?(honzab.moz) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

a year ago
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/67185e1252eb
(Part 1) Add nsIURLMutator r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/f7e4fd351759
(Part 2) Change calls to nsIURL.{fileName,fileBaseName,fileExtension} setters to use nsIURLMutator r=mayhemer

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/67185e1252eb
https://hg.mozilla.org/mozilla-central/rev/f7e4fd351759
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.