Support cooperative threading in mfbt/WeakPtr.h thread safety assertions

RESOLVED FIXED in Firefox 56

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch patch (obsolete) — Splinter Review
Most of our thread safety assertions just use GetCurrentVirtualThread from nsThreadUtils.h. They work with cooperative threads. But there is one assertion in WeakPtr.h that can't use this because it's in MFBT.

Originally I wrote a patch that added a somewhat general thread safety checker to mfbt and factored out the WeakPtr code into that. However, it ended up being a bit awkward to use in WeakPtr because it was overly general for that use case. So this patch just implements the code directly as part of WeakPtr. I can go the other way if you want, though.
Attachment #8890171 - Flags: review?(nfroyd)
Comment on attachment 8890171 [details] [diff] [review]
patch

Review of attachment 8890171 [details] [diff] [review]:
-----------------------------------------------------------------

It would be kind of nice if we could use XPCOM's nsAutoOwningThread machinery here instead of reinventing the wheel; we used std::thread IDs in MFBT because there's not a good cross-platform abstraction in MFBT itself.  But reusing XPCOM's (via whatever mechanism we use XPCOM's refcount logging in RefCounted.h) type here seems a little nicer than dragging in the concepts of virtual threads to MFBT.  WDYT?

::: mfbt/WeakPtr.h
@@ +108,5 @@
> +
> +namespace mozilla {
> +
> +// This function should be called at application startup to initialize TLS.
> +MFBT_API bool InitWeakPtrThreadSafetyChecking();

I think these functions should all live in mozilla::detail.  I could see an argument for the *CurrentVirtualStdThread functions just living in mozilla::, though.  WDYT?
Attachment #8890171 - Flags: review?(nfroyd)
Posted patch patch v2Splinter Review
Thanks for the suggestion. This is much nicer.
Attachment #8890171 - Attachment is obsolete: true
Attachment #8890552 - Flags: review?(nfroyd)
Comment on attachment 8890552 [details] [diff] [review]
patch v2

Review of attachment 8890552 [details] [diff] [review]:
-----------------------------------------------------------------

I'm so glad this turned out so nicely.  r=me with the below change.

::: xpcom/base/nsISupportsImpl.cpp
@@ +44,5 @@
>    }
>  }
> +
> +bool
> +nsAutoOwningThread::IsCurrentThread() const

Can you make AssertCurrentThreadOwnsMe use this function?
Attachment #8890552 - Flags: review?(nfroyd) → review+

Comment 4

2 years ago
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd7b9bb53462
Use nsAutoOwningThread for mfbt/WeakPtr.h thread assertions (r=froydnj)

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd7b9bb53462
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.