1339677 - Add sanity checks to debug MozPromise crashes

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[JW Wang [:jwwang] [:jw_wang]]

See bug 1338025. We want to add some checks to clarify if this is a MozPromise bug or some memory corruption.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1339677. Part 1 - assert DoResolveOrReject() is called on the target thread and use stronger assertions.

Review commit: https://reviewboard.mozilla.org/r/112894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112894/

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1339677. Part 2 - some code cleanup and remove unnecessary scope qualifiers.

Review commit: https://reviewboard.mozilla.org/r/112896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112896/

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1339677. Part 3 - add some sanity checks.

Review commit: https://reviewboard.mozilla.org/r/112898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112898/

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1339677. Part 4 - check if mMutex.mLock is tampered.

Review commit: https://reviewboard.mozilla.org/r/112900/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112900/

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8837897 [details]
Bug 1339677. Part 1 - assert DoResolveOrReject() is called on the target thread and use stronger assertions.

https://reviewboard.mozilla.org/r/112894/#review114386

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8837897 [details]
Bug 1339677. Part 1 - assert DoResolveOrReject() is called on the target thread and use stronger assertions.

https://reviewboard.mozilla.org/r/112894/#review114390

Will you uplift the new MOZ_DIAGNOSTIC_ASSERT's? (Since they would be more useful in Aurora and Beta)

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8837898 [details]
Bug 1339677. Part 2 - some code cleanup and remove unnecessary scope qualifiers.

https://reviewboard.mozilla.org/r/112896/#review114392

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8837899 [details]
Bug 1339677. Part 3 - add some sanity checks.

https://reviewboard.mozilla.org/r/112898/#review114396

r+ with suggestions:

::: xpcom/threads/MozPromise.h:428
(Diff revision 1)
>        }
>      }
>  
>      RefPtr<AbstractThread> mResponseTarget; // May be released on any thread.
> +#ifdef PROMISE_DEBUG
> +    uint32_t mMagic1 = 0xdeadbeef;

0xDEADBEEF is usually used to indicate dead memory, but here you're using it as a "live" indicator.
More seriously, it's used elsewhere in our code to poison memory, so in the worst case your tests could pass when the promise has been deliberately overwritten with DEADBEEFs!

So I would suggest using another random value, through a constant.

I would also suggest you implement destructors that set these members to a different special value, to catch UAFs when the promise's memory has not (yet) been overwritten.

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8837900 [details]
Bug 1339677. Part 4 - check if mMutex.mLock is tampered.

https://reviewboard.mozilla.org/r/112900/#review114398

r+ with suggestion:

::: xpcom/threads/Mutex.h:32
(Diff revision 1)
> +template<typename, typename, bool>
> +class MozPromise;

I believe you don't actually need to forward-declare MozPromise here, the friend declaration inside OffTheBooksMutex should be enough.

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8837899 [details]
Bug 1339677. Part 3 - add some sanity checks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112898/diff/1-2/

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8837900 [details]
Bug 1339677. Part 4 - check if mMutex.mLock is tampered.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112900/diff/1-2/

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8837899 [details]
Bug 1339677. Part 3 - add some sanity checks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112898/diff/2-3/

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8837900 [details]
Bug 1339677. Part 4 - check if mMutex.mLock is tampered.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112900/diff/2-3/

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/241efbf6fcc9
Part 1 - assert DoResolveOrReject() is called on the target thread and use stronger assertions. r=gerald
https://hg.mozilla.org/integration/autoland/rev/dc5b70d15e11
Part 2 - some code cleanup and remove unnecessary scope qualifiers. r=gerald
https://hg.mozilla.org/integration/autoland/rev/d1807b8f31dc
Part 3 - add some sanity checks. r=gerald
https://hg.mozilla.org/integration/autoland/rev/df87366fda49
Part 4 - check if mMutex.mLock is tampered. r=gerald

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/241efbf6fcc9
https://hg.mozilla.org/mozilla-central/rev/dc5b70d15e11
https://hg.mozilla.org/mozilla-central/rev/d1807b8f31dc
https://hg.mozilla.org/mozilla-central/rev/df87366fda49

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1339677_aurora_fix.patch

Approval Request Comment
[Feature/Bug causing the regression]:1339677
[User impact if declined]: a debugging patch for bug 1257921
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:no
[Why is the change risky/not risky?]:This patch adds some assertions to detect memory corruption and doesn't any functionality.
[String changes made/needed]:

Comment 18

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1339677_beta_fix.patch

Approval Request Comment
[Feature/Bug causing the regression]:1339677
[User impact if declined]:a debugging patch for bug 1257921
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:no
[Why is the change risky/not risky?]:This patch adds some assertions to detect memory corruption and doesn't any functionality.
[String changes made/needed]:none

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8838396 [details] [diff] [review]
1339677_aurora_fix.patch

Should be helpful for crash diagnosis, let's uplift.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8838397 [details] [diff] [review]
1339677_beta_fix.patch

A little scary but let's take this for beta 8 to help with crash diagnosis.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/104592a2b9be

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d5c64168f67f

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/d5c64168f67f