Closed Bug 1424647 Opened 2 years ago Closed 2 years ago

MozPromise::All is racy and could incorrectly return null

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(1 file)

https://searchfox.org/mozilla-central/rev/cd742d763809089925a38178dd2ba5a9069fa855/xpcom/threads/MozPromise.h#337

MozPromise::All will create a AllPromiseHolder and call Then() on each of the promises contained in the array.

it then return holder->Promise() which returns holder::mPromise

However, if any of the promises handling is prior the loop completing, either calling AllPromiseHolder::Resolve or AllPromiseHolder::Reject

then AllPromiseHolder::mPromise is set to nullptr:
Resolve:
https://searchfox.org/mozilla-central/rev/cd742d763809089925a38178dd2ba5a9069fa855/xpcom/threads/MozPromise.h#311
Reject:
https://searchfox.org/mozilla-central/rev/cd742d763809089925a38178dd2ba5a9069fa855/xpcom/threads/MozPromise.h#324

this causes MozPromise::All to incorrectly return nullptr.
Comment on attachment 8936179 [details]
Bug 1424647: Prevent race on AllPromiseHolder::mPromise.

https://reviewboard.mozilla.org/r/206938/#review212764

Good catch! Is this bug causing you any trouble?
Attachment #8936179 - Flags: review?(jwwang) → review+
Comment on attachment 8936179 [details]
Bug 1424647: Prevent race on AllPromiseHolder::mPromise.

https://reviewboard.mozilla.org/r/206938/#review212764

yes...

was causing my new AwaitAll to crash with a null deref.

I guess All wasnt used enough previously to expose the problem.
it is used however by GMP/Widevine, it would likely cause crashes there too.
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d926a50dcf5
Prevent race on AllPromiseHolder::mPromise. r=jwwang
https://hg.mozilla.org/mozilla-central/rev/6d926a50dcf5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.