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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Honza Bambas (:mayhemer)]

+++ 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

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: wip1

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

Comment 2

[Honza Bambas (:mayhemer)]

(I'll push to try when it's not so overloaded)

Comment 3

[(no longer active)]

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.

Comment 4

[(no longer active)]

(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?

Comment 5

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hopefully something that mimics C++11 thread primitives? Looks like there is a |std::thread::id get_id()|

Comment 7

[Honza Bambas (:mayhemer)]

(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!

Comment 8

[Honza Bambas (:mayhemer)]

(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

[(no longer active)]

(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

[Jeff Walden [:Waldo]]

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.

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: v1

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.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: v1.1

A bit enhanced patch:
- adds check also to the WeakPtr class, check delegated to the referred WeakReference<> object

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v2

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.

Comment 14

[Honza Bambas (:mayhemer)]

Attachment: v2.1

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.

Comment 15

[Honza Bambas (:mayhemer)]

Attachment: v2.1

Comment 16

[Nathan Froyd [:froydnj]]

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.

Comment 17

[Honza Bambas (:mayhemer)]

(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).

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: v2.2

comments updated, plus few minor tweaks.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc9b997ac90b
(ignore android failures, different bug, already patched)

Comment 19

[Honza Bambas (:mayhemer)]

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.

Comment 20

[Honza Bambas (:mayhemer)]

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.

Comment 21

[Nathan Froyd [:froydnj]]

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."

Comment 22

[Honza Bambas (:mayhemer)]

Attachment: v2.3

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6a15c99adb4

Comment 23

[Honza Bambas (:mayhemer)]

Attachment: v2.3 [updated commit message]

All deps landed, last try before checking in:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74cbb9192861

Comment 24

[Honza Bambas (:mayhemer)]

Good to go now.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2fda83015d8
Add checks to WeakPtr/nsWeakReference and related classes to assert single-thread usage. r=nfroyd

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e2fda83015d8