Closed
Bug 1384395
Opened 7 years ago
Closed 7 years ago
Support cooperative threading in mfbt/WeakPtr.h thread safety assertions
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: billm, Assigned: billm)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
4.51 KB,
patch
|
froydnj
:
review+
|
Details | Diff | 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 1•7 years ago
|
||
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)
Assignee | ||
Comment 2•7 years ago
|
||
Thanks for the suggestion. This is much nicer.
Attachment #8890171 -
Attachment is obsolete: true
Attachment #8890552 -
Flags: review?(nfroyd)
Comment 3•7 years ago
|
||
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+
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dd7b9bb53462
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•