Closed
Bug 956338
Opened 11 years ago
Closed 8 years ago
Add checks to WeakPtr and related classes to assert single-thread usage
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: mayhemer, Assigned: mayhemer)
References
Details
Attachments
(1 file, 8 obsolete files)
11.15 KB,
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #956327 +++ Similarly to how we check thread-safety of our ordinary xpcom objects [1], we must add these checks to weakptr. It's not thread-safe and it's not planned to be made thread-safe in the future. [1] http://hg.mozilla.org/mozilla-central/annotate/325c74addeba/xpcom/glue/nsISupportsImpl.h#l63
![]() |
Assignee | |
Comment 1•11 years ago
|
||
We have 3 implementation of weakptr in the tree: 1. xpcom\glue\nsWeakReference.cpp, well known, this patch adds assertions on thread access to it (only locally tested so far) 2. mfbt\WeakPtr.h, our 'chromium-inspired' implementation, seems like the tree is migrating to it * for this one, I don't know how to easily add thread checks, Ehsan? ; I believe NSPR is forbidden in mfbt, so I need to go natively ; something like [1] would be handy 3. security\sandbox\base\memory\weak_ptr.h, has thread safety checks and assertions, see [2] [1] http://mxr.mozilla.org/mozilla-central/source/security/sandbox/base/sequence_checker_impl.h [2] http://hg.mozilla.org/mozilla-central/annotate/8dc7bf30840d/security/sandbox/base/memory/weak_ptr.h#l48
Attachment #8364518 -
Flags: feedback?(ehsan)
![]() |
Assignee | |
Comment 2•11 years ago
|
||
(I'll push to try when it's not so overloaded)
Comment 3•11 years ago
|
||
Comment on attachment 8364518 [details] [diff] [review] wip1 Review of attachment 8364518 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! Thanks for doing this. ::: xpcom/glue/nsWeakReference.h @@ +20,5 @@ > // controls member method visibility. > #undef IMETHOD_VISIBILITY > #define IMETHOD_VISIBILITY NS_COM_GLUE > > +#if (defined(DEBUG) || (defined(NIGHTLY_BUILD) && !defined(MOZ_PROFILING))) && !defined(XPCOM_GLUE_AVOID_NSPR) Can you please refactor this black magic into a macro which tracks all of these sorts of checks? You can put that in nsDebug.h or somewhere.
Attachment #8364518 -
Flags: feedback?(ehsan) → feedback+
Comment 4•11 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #1) > 2. mfbt\WeakPtr.h, our 'chromium-inspired' implementation, seems like the > tree is migrating to it > * for this one, I don't know how to easily add thread checks, Ehsan? ; I > believe NSPR is forbidden in mfbt, so I need to go natively ; something like > [1] would be handy Hmm, this is a good question. I'm pretty sure NSPR is forbidden in MFBT. Jeff, how should we deal with this kind of thing?
Flags: needinfo?(jwalden+bmo)
Comment 5•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4) > (In reply to Honza Bambas (:mayhemer) from comment #1) > > 2. mfbt\WeakPtr.h, our 'chromium-inspired' implementation, seems like the > > tree is migrating to it > > * for this one, I don't know how to easily add thread checks, Ehsan? ; I > > believe NSPR is forbidden in mfbt, so I need to go natively ; something like > > [1] would be handy > > Hmm, this is a good question. I'm pretty sure NSPR is forbidden in MFBT. > Jeff, how should we deal with this kind of thing? A simple get_thread_id abstraction would probably do the trick here.
Hopefully something that mimics C++11 thread primitives? Looks like there is a |std::thread::id get_id()|
![]() |
Assignee | |
Comment 7•11 years ago
|
||
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #6) > Hopefully something that mimics C++11 thread primitives? Looks like there is > a |std::thread::id get_id()| OK, I think I'll go a simple way and just add this directly to our WeakPtr. Thanks!
![]() |
Assignee | |
Comment 8•11 years ago
|
||
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #6) > Hopefully something that mimics C++11 thread primitives? Looks like there is > a |std::thread::id get_id()| So, I wrote it according [1] but on win (vc10) I'm getting: 'Cannot open include file: 'thread': No such file or directory' When building using vc11 (that has this header), my build fails. Do we yet support vc11 builds? [1] http://en.cppreference.com/w/cpp/thread/get_id
Comment 9•11 years ago
|
||
(In reply to comment #8) > (In reply to ben turner [:bent] (use the needinfo? flag!) from comment #6) > > Hopefully something that mimics C++11 thread primitives? Looks like there is > > a |std::thread::id get_id()| > > So, I wrote it according [1] but on win (vc10) I'm getting: > 'Cannot open include file: 'thread': No such file or directory' > > When building using vc11 (that has this header), my build fails. Do we yet > support vc11 builds? > > [1] http://en.cppreference.com/w/cpp/thread/get_id I think you would need to emulate that header in MFBT for platforms that do not support it. If VC11 ships that header and has a good get_id implementation, then we can use the the native header instead. See mfbt/TypeTraits.h for an example of a header that emulates the standard <type_traits> header.
Comment 10•11 years ago
|
||
Yeah, no NSPR in mfbt. As far as how to do it, bug 956899 is working on writing reasonable C++ things for all the thread stuff. It's starting out in JS-land because of various naming conflicts with the mozilla namespace, then we'll make the two APIs consistent, then we'll move the stuff out of JS into mfbt. That's the ideal solution. But, of course, I'm not sure what the timeline is for that bug's work, and whether it's adequate to fit the timeline here. If that's out, adding a hacked-up set of definitions of Thread::getId() seems fair enough in the short run. I'd keep it as local to WeakPtr.cpp (possibly new) as possible, to avoid people using it in advance of the right API/implementation/interface being added, but that's just details.
Flags: needinfo?(jwalden+bmo)
![]() |
Assignee | |
Comment 11•8 years ago
|
||
So, since we now have std::thread* stuff available on windows (vs15), this can be finally finished. Here is a first review-quality patch that catches the first bug (to be filed and yet be confirmed). This should cover the thread-non-safety checks in both nsWeakReference et al and WeakPtr et al.
Attachment #8364518 -
Attachment is obsolete: true
Attachment #8776633 -
Flags: review?(ehsan)
![]() |
Assignee | |
Comment 12•8 years ago
|
||
A bit enhanced patch: - adds check also to the WeakPtr class, check delegated to the referred WeakReference<> object
Attachment #8776633 -
Attachment is obsolete: true
Attachment #8776633 -
Flags: review?(ehsan)
Attachment #8776989 -
Flags: review?(ehsan)
![]() |
Assignee | |
Updated•8 years ago
|
![]() |
Assignee | |
Comment 13•8 years ago
|
||
a bit loosen patch: do the checks only and only on the proxy object. it can happen we create a weak ref supporting object on one thread, pass it to another and only then create weak refs to it. If that object since then lives only on that thread, we are safe. ehsan didn't respond for a time, Nathan, please take a look.
Attachment #8776989 -
Attachment is obsolete: true
Attachment #8776989 -
Flags: review?(ehsan)
Attachment #8778970 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Comment 14•8 years ago
|
||
I hope you haven't started yet. But these are just minor changes. I'm doing checks on mozilla::WeakReference only when it has not been initialized as an empty placeholder.
Attachment #8778970 -
Attachment is obsolete: true
Attachment #8778970 -
Flags: review?(nfroyd)
Attachment #8779354 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Comment 15•8 years ago
|
||
Attachment #8779354 -
Attachment is obsolete: true
Attachment #8779354 -
Flags: review?(nfroyd)
Attachment #8779356 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Updated•8 years ago
|
Depends on: CVE-2017-5392
![]() |
||
Comment 16•8 years ago
|
||
Comment on attachment 8779356 [details] [diff] [review] v2.1 Review of attachment 8779356 [details] [diff] [review]: ----------------------------------------------------------------- I have no problem with the mechanics of the asserts, which are much-appreciated, but I have some concerns about the surrounding machinery. ::: mfbt/WeakPtr.h @@ +78,5 @@ > > +// Weak referencing is not implemeted as thread safe. Getting a WeakPtr for > +// an object using asWeakPtr(), then access on and addref of the real object > +// obtained from dereferencing the WeakPtr and destruction of the real object > +// must all happen strictly on a signle thread. This whole comment needs some wordsmithing: - Where does "asWeakPtr()" come from? It's not defined in this file. - "access on and addref of the real object" isn't really covered by the checks here, is it? Whether the object itself is safe to be accessed from multiple threads is a policy decision handled elsewhere, correct? I *think* even AddRef/Release of the object on multiple threads is even handled elsewhere... In short, I'm a little confused about what the comment is claiming. @@ +100,5 @@ > + > +#define MOZ_WEAKPTR_DECLARE_THREAD_SAFETY_CHECK > +#define MOZ_WEAKPTR_INIT_THREAD_SAFETY_CHECK() do { } whilte (false) > +#define MOZ_WEAKPTR_ASSERT_THREAD_SAFETY() do { } whilte (false) > +#define MOZ_WEAKPTR_ASSERT_THREAD_SAFETY_DELEGATED(that) do { } whilte (false) "while" is misspelled in all of these macros. @@ +160,5 @@ > > T* MOZ_NON_OWNING_REF mPtr; > + MOZ_WEAKPTR_DECLARE_THREAD_SAFETY_CHECK > +#ifdef MOZ_WEAKPTR_THREAD_SAFETY_CHECKING > + // if it was initialized as a placeholed with ptr = null "If it was initialized as a placeholder with mPtr = nullptr." @@ +228,5 @@ > if (aOther) { > *this = aOther->SelfReferencingWeakPtr(); > } else if (!mRef || mRef->get()) { > // Ensure that mRef is dereferenceable in the uninitialized state. > mRef = new WeakReference(nullptr); My understanding is that the thread safety checks we need are handled by the functions that we call. Is that correct? Should we include a comment here to that effect, to explain to people why we're not asserting in this operator= method when we asserted in the copy assignment operator above? ::: xpcom/glue/nsDebug.h @@ +341,5 @@ > NS_ENSURE_FALSE(outer, NS_ERROR_NO_AGGREGATION) > > /*****************************************************************************/ > > +#if (defined(DEBUG) || (defined(NIGHTLY_BUILD) && !defined(MOZ_PROFILING))) && !defined(XPCOM_GLUE_AVOID_NSPR) Why is there a difference in the checking conditions here vs. the ones in MFBT? I guess the XPCOM_GLUE_AVOID_NSPR is because the owning thread mechanism here depends on PR_Thread, but why MOZ_PROFILING here and not in MFBT? @@ +342,5 @@ > > /*****************************************************************************/ > > +#if (defined(DEBUG) || (defined(NIGHTLY_BUILD) && !defined(MOZ_PROFILING))) && !defined(XPCOM_GLUE_AVOID_NSPR) > + #define MOZ_DO_THREAD_SAFETY_OWNING_CHECK 1 Nit: I think this should be named something like MOZ_THREAD_SAFETY_OWNERSHIP_CHECKS_SUPPORTED (what a mouthful!). ::: xpcom/glue/nsWeakReference.h @@ +45,5 @@ > private: > friend class nsWeakReference; > > // Called (only) by an |nsWeakReference| from _its_ dtor. > + // The thread safety check is made by the caller Nit: periods after sentences, please.
Attachment #8779356 -
Flags: review?(nfroyd) → feedback+
![]() |
Assignee | |
Comment 17•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #16) > Comment on attachment 8779356 [details] [diff] [review] > v2.1 > > Review of attachment 8779356 [details] [diff] [review]: > ----------------------------------------------------------------- > > I have no problem with the mechanics of the asserts, which are > much-appreciated, but I have some concerns about the surrounding machinery. I broke my rule of going trough the patch before submitting it, it has been updated few times, but I never updated the comments. > - Where does "asWeakPtr()" come from? It's not defined in this file. Probably a typo, or some other dimension where asWeakPtr is defined crossed my universe, or result of some mass rename.. not sure :) > - "access on and addref of the real object" isn't really covered by the Simply forget the comment, there will be a new one. > but why MOZ_PROFILING here and not in > MFBT? Bug 919380. mfbt should respect that too (now I understand why it's there).
![]() |
Assignee | |
Comment 18•8 years ago
|
||
comments updated, plus few minor tweaks. https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc9b997ac90b (ignore android failures, different bug, already patched)
Attachment #8779356 -
Attachment is obsolete: true
Attachment #8781565 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Comment 19•8 years ago
|
||
Nathan, MOZ_PROFILING apparently is declared for Android 4.3 API15+ opt tests. Adding it to the condition as an off switch effectively hides the android widget bug 1293709. Not sure what to do here. If to change the tests or simply go with having these thread safety checks also with MOZ_PROFILING declared. The former seems a better thing to do.
Flags: needinfo?(nfroyd)
![]() |
Assignee | |
Comment 20•8 years ago
|
||
There is Android 4.3 API15+ debug suite that runs mochitests. It should cover these checks well. It's just not ran by default on try -a.
Flags: needinfo?(nfroyd)
![]() |
||
Comment 21•8 years ago
|
||
Comment on attachment 8781565 [details] [diff] [review] v2.2 Review of attachment 8781565 [details] [diff] [review]: ----------------------------------------------------------------- Apologies for the delay. This is good, just some small comment adjustments. Please shout if I botched the revised text below or you think it could be made clearer. ::: mfbt/WeakPtr.h @@ +83,5 @@ > +// the referee destruction just between when thread A is storing > +// the referee pointer locally and is about to add a reference to it. > +// Hence, the proxy object must be worked with exclusively on a single > +// thread. It means that first query for a weak reference, dereference > +// of it and referee destruction must all happen on a single thread. This is a better comment, thank you for expanding it. Why is "the first query" called out specifically, but none of the queries after that? That phrasing implies the possibility that subsequent queries could happen on other threads. Do you mean something more like: A non-null WeakPtr is considered to have a single "owning thread": the thread on which it was constructed. All queries of the WeakPtr, dereferences, and destruction of the WeakPtr's referent must happen on the owning thread. ? @@ +84,5 @@ > +// the referee pointer locally and is about to add a reference to it. > +// Hence, the proxy object must be worked with exclusively on a single > +// thread. It means that first query for a weak reference, dereference > +// of it and referee destruction must all happen on a single thread. > +// There are assertions that check that. Nit: "The following macros implement assertions for checking these conditions." @@ +223,5 @@ > } > > WeakPtr(const WeakPtr& aOther) > { > + // Thread safety inside the assignment operator Nit: "The thread safety check is performed inside of the operator= method." @@ +236,5 @@ > // Ensure that mRef is dereferenceable in the uninitialized state. > mRef = new WeakReference(nullptr); > } > + // Thread safety checks are happening either inside SelfReferencingWeakPtr > + // or WeakReference contructor. Nit: "The thread safety check happens inside either SelfReferencingWeakPtr or the WeakReference constructor."
Attachment #8781565 -
Flags: review?(nfroyd) → review+
![]() |
Assignee | |
Comment 22•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6a15c99adb4
Attachment #8781565 -
Attachment is obsolete: true
Attachment #8782849 -
Flags: review+
![]() |
Assignee | |
Comment 23•8 years ago
|
||
All deps landed, last try before checking in: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74cbb9192861
Attachment #8782849 -
Attachment is obsolete: true
Attachment #8782902 -
Flags: review+
Comment 25•8 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e2fda83015d8 Add checks to WeakPtr/nsWeakReference and related classes to assert single-thread usage. r=nfroyd
Keywords: checkin-needed
Comment 26•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e2fda83015d8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•