1614395 - MozPromiseHolder::operator=(MozPromiseHolder&&) shouldn't copy its mPromise member

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

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

The move assignment operator should move the member instead.

Comment 1

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

NB: The assertions in the move assignment operator seem a bit odd, as they mean that it places custom preconditions on when an instance can be the target of a move. I am not sure if this is a good idea, as this makes the use error-prone and prevents the use of the class in generic templates (e.g. a nsTArray, which might move its elements in situations unaware of these assertions). It might be a better idea to provide a named member function instead, and delete the move operations altogether, if MozPromiseHolder is not intended to be a general movable type.

Comment 2

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

Attachment: Bug 1614395 - MozPromiseHolder::operator=(MozPromiseHolder&&) shouldn't copy its mPromise member. r=froydnj

Comment 3

[Nathan Froyd [:froydnj]]

I don't know the exact rationale behind the assertions in MozPromiseHolder. But looking at them, I would guess:

1. mMonitor essentially makes another type of MozPromiseHolder, so the mMonitor assertions are making sure that you're not moving from a monitor-requiring instance to a non-monitor requiring instance -- that wouldn't make a lot of sense. (There are other combinations, but that seems like the easiest to explain.)
2. We're also ensuring that we don't overwrite an existing promise. Overwriting an existing promise here...what would that even mean?

I guess we could use a separately-named member function here, but it's going to be a weird class that supports move construction but not move assignment. We already have used MozPromiseHolder in generic containers and it doesn't seem to have caused too many problems yet...

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e028937a94a8
MozPromiseHolder::operator=(MozPromiseHolder&&) shouldn't copy its mPromise member. r=froydnj

Comment 5

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/e028937a94a8

Comment 6

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

(In reply to Nathan Froyd [:froydnj] from comment #3)

I don't know the exact rationale behind the assertions in MozPromiseHolder. But looking at them, I would guess:

1. mMonitor essentially makes another type of MozPromiseHolder, so the mMonitor assertions are making sure that you're not moving from a monitor-requiring instance to a non-monitor requiring instance -- that wouldn't make a lot of sense. (There are other combinations, but that seems like the easiest to explain.)

This is only used at two places: https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla16MozPromiseHolder10SetMonitorEPNS_7MonitorE&redirect=false

Maybe this really should be a separate type (maybe via an additional template parameter), which is syntactically non-movable then. All the other uses don't need the mMonitor member at all.

2. We're also ensuring that we don't overwrite an existing promise. Overwriting an existing promise here...what would that even mean?

You mean, because, if Resolve, Reject or Steal have been called, mPromise will be set to nullptr, and this ensures that a promise is not replaced without having dealt with it in either of these ways? Maybe that assertion is fine, still it looks a bit odd.

I guess we could use a separately-named member function here, but it's going to be a weird class that supports move construction but not move assignment.

Well, this would be odd, but less odd than. But never mind, the move assignment is probably needed due to:

We already have used MozPromiseHolder in generic containers and it doesn't seem to have caused too many problems yet...

Sorry, I didn't check before. Indeed, there are several uses of nsTArray<MozPromise>. I checked a few of them, they make only a very limited use of nsTArray. If they remove elements, they always move out the element first, which ensures that the precondition holds, given the current implementation of nsTArray. Interestingly, there are also some uses of nsTArray<UniquePtr<MozPromise>>, which don't make too much sense. They might be a workaround around issues due to the move preconditions though.