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)

defect
Not set
normal

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)

Attached 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)
Attached 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+
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd7b9bb53462
Use nsAutoOwningThread for mfbt/WeakPtr.h thread assertions (r=froydnj)
https://hg.mozilla.org/mozilla-central/rev/dd7b9bb53462
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.