Closed Bug 1339677 Opened 3 years ago Closed 3 years ago

Add sanity checks to debug MozPromise crashes

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(6 files)

See bug 1338025. We want to add some checks to clarify if this is a MozPromise bug or some memory corruption.
See Also: → 1338025
Assignee: nobody → jwwang
See Also: → 1257921
Attachment #8837897 - Flags: review?(gsquelart)
Attachment #8837898 - Flags: review?(gsquelart)
Attachment #8837899 - Flags: review?(gsquelart)
Attachment #8837900 - Flags: review?(gsquelart)
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
Attachment #8837897 - Flags: review?(gsquelart) → review+
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 on attachment 8837898 [details]
Bug 1339677. Part 2 - some code cleanup and remove unnecessary scope qualifiers.

https://reviewboard.mozilla.org/r/112896/#review114392
Attachment #8837898 - Flags: review?(gsquelart) → review+
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.
Attachment #8837899 - Flags: review?(gsquelart) → review+
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.
Attachment #8837900 - Flags: review?(gsquelart) → review+
Thanks for the review!
Pushed by jwwang@mozilla.com:
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
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]:
Attachment #8838396 - Flags: review+
Attachment #8838396 - Flags: approval-mozilla-aurora?
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
Attachment #8838397 - Flags: review+
Attachment #8838397 - Flags: approval-mozilla-beta?
Comment on attachment 8838396 [details] [diff] [review]
1339677_aurora_fix.patch

Should be helpful for crash diagnosis, let's uplift.
Attachment #8838396 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8838397 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.