Closed Bug 956338 Opened 6 years ago Closed 3 years ago

Add checks to WeakPtr and related classes to assert single-thread usage

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

Attachments

(1 file, 8 obsolete files)

+++ 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
Attached patch wip1 (obsolete) — Splinter Review
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)
(I'll push to try when it's not so overloaded)
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+
(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)
See Also: → 935778
(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()|
(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!
(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
(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.
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)
Attached patch v1 (obsolete) — Splinter Review
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)
See Also: → 1290975
Attached patch v1.1 (obsolete) — Splinter Review
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)
Depends on: 1290975
See Also: 1290975
Depends on: 1293327
Depends on: 1293328
Attached patch v2 (obsolete) — Splinter Review
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)
Depends on: 1293621
Attached patch v2.1 (obsolete) — Splinter Review
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)
Attached patch v2.1 (obsolete) — Splinter Review
Attachment #8779354 - Attachment is obsolete: true
Attachment #8779354 - Flags: review?(nfroyd)
Attachment #8779356 - Flags: review?(nfroyd)
No longer depends on: 1293328
Depends on: CVE-2017-5392
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+
(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).
Attached patch v2.2 (obsolete) — Splinter Review
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)
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)
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 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+
Attached patch v2.3 (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6a15c99adb4
Attachment #8781565 - Attachment is obsolete: true
Attachment #8782849 - Flags: review+
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+
Depends on: 1297099
Good to go now.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/e2fda83015d8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1299076
Depends on: 1299113
Depends on: 1299116
Depends on: 1299920
Depends on: 1317642
You need to log in before you can comment on or make changes to this bug.