1520955 - Add ref qualifier to DataMutex for more safety

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Jeff Muizelaar [:jrmuizel]]

We lose some sugar but gain some safety. This seems like the right trade. If people want sugar they should use Rust.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1520955. Add ref qualifier to DataMutex for more safety.

We lose some sugar but gain some safety. This seems like the right
trade. If people want sugar they should use Rust.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f73ecaf9f40
Add ref qualifier to DataMutex for more safety. r=froydnj

Comment 3

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/0f73ecaf9f40

Comment 4

[Simon Giesecke [:sg] [he/him]]

What safety is gained through these changes?

Comment 5

[Jeff Muizelaar [:jrmuizel]]

I've actually forgotten. Perhaps the lock can be dropped too early in some situations. I guess I should've written a better commit message. Maybe Nathan remembers.

Comment 6

[Nathan Froyd [:froydnj]]

I think the concern is T& x = mMember.Lock().ref(); or similar doesn't actually hold the lock as long as you would like. I know there are rules about const references extending lifetimes of temporaries (I can't find the standardese for them right now), but I don't think they would apply in this case (even if x was const T&).

Comment 7

[Simon Giesecke [:sg] [he/him]]

I was more thinking about operator-> which was also changed by the attached patch to disallow writing something like:

mDataMutex.Lock()->DoSomething()

which is not very convenient. The similar class boost::synchronized_value allows this, e.g.

Maybe the deletions of operator-> (and maybe also operator*) can be removed again?