1404742 - UnscaledFont can't support WeakPtr anymore

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect, P1)

Description

[Bas Schouten (:bas.schouten)]

Since OMTP allows font objects to be destroyed off the main thread. They can't implement SupportWeakPtr (thread-safe support for SupportWeakPtr has not been implemented yet as far as I can tell).

Comment 1

[Bas Schouten (:bas.schouten)]

As requested I'm assigning this to Lee. We believe we have a work-around for the moment (bug 1404749). Although it's suboptimal, we'll want to fix this before OMTP rides the trains.

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: add ThreadSafeWeakPointer to MFBT

This is based off the idea of a very cheap read-write spinlock that allows low overhead/low memory usage synchronization of the underlying weak reference, as compared to much heavier weight primitives like mutexes or critical sections. This guarantees that releasing of the object and creation of new references is properly synchronized so as to avoid races.

This will allow us to properly track font lifetimes with OMTP.

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: track ScaledFont and UnscaledFont lifetimes with ThreadSafeWeakPointer

Now that there is a ThreadSafeWeakPointer, actually use the thing. Just some simple replacements in WR and thebes where we were previously using WeakPtr.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8914832 [details] [diff] [review]
add ThreadSafeWeakPointer to MFBT

Review of attachment 8914832 [details] [diff] [review]:
-----------------------------------------------------------------

Some kind of tests for this would be great.  We have gtests for mfbt stuff, so that would at least solve the threads part.

I think I understand all this.  Some comments below.

::: mfbt/ThreadSafeWeakPtr.h
@@ +8,5 @@
> +
> +/**
> + * Derive from SupportsThreadSafeWeakPtr to allow thread-safe weak pointers to an
> + * atomically refcounted derived class. These thread-safe weak pointers may be safely
> + * accessed and converted to strong pointers on multiple threads.

Thanks for this comment.

We may want to make explicit that we have separated ThreadSafeWeakPtr and friends from WeakPtr to minimize overhead?

@@ +72,5 @@
> +#endif
> +
> +namespace detail {
> +
> +// A cheap multiple reader, single writer spin-lock

It would be nice if the algorithm used for this lock were described in some detail up front, rather than spread out over four or five places.

I think I believe your algorithm works, but I also did some digging to see if anybody else has come up with such an idea before and found:

https://jfdube.wordpress.com/2014/01/03/implementing-a-recursive-read-write-spinlock/
http://joeduffyblog.com/2009/01/29/a-singleword-readerwriter-spin-lock/

which splits the counter up into a writer bit (sign bit) and # of readers (everything else).  IIUC, this is your scheme, but a little more clearly expressed.

@@ +85,5 @@
> +  void readLock()
> +  {
> +    while (mCounter++ < 0) {
> +      mCounter--;
> +    }

I think it'd be clearer to phrase this as:

bool success;
do {
  success = false;
  CounterType old = mCounter & 0x7fffffff;
  CounterType new = old + 1;
  // maybe MOZ_ASSERT that we didn't overflow--though we have to be careful about the compiler
  // optimizing away the assertion, given that `old + 1` overflowing would be undefined behavior...
  success = mCounter.compareExchange(old, new);
} while (!success);

which I guess might be a little more expensive on ARM in the success case?  The above loop avoids spurious writes, though, which seems like a nice property.  We could rewrite this using std::atomic so the first access was relaxed, which I *think* would be OK.

@@ +102,5 @@
> +    return mCounter.compareExchange(0, std::numeric_limits<CounterType>::min());
> +  }
> +
> +  // Subtract off the negative value (in case it got increment temporarily) so the counter
> +  // is now positive again.

Nit: "in case it got incremented temporarily", yes?

@@ +105,5 @@
> +  // Subtract off the negative value (in case it got increment temporarily) so the counter
> +  // is now positive again.
> +  void writeUnlock()
> +  {
> +    mCounter -= std::numeric_limits<CounterType>::min();

Really, this is just:

bool success = mCounter.compareExchange(std::numeric_limits<CounterType>::min(), 0);
MOZ_ASSERT(success);

right?--at least assuming that people have always successfully done tryWriteLock() prior to calling writeUnlock().  I guess the compareExchange doesn't work if you're going to speculatively write to mCounter in readLock(), but I think you shouldn't be doing that (see above).  This formulation feels clearer to me.

@@ +125,5 @@
> +  }
> +
> +  T* get() const
> +  {
> +    return mPtr;

Why are we willing to give out weak pointers to this thing as a public interface?  I realize it's in ::detail, so people shouldn't be using it, but even so, whyyyy?

@@ +134,5 @@
> +  template<typename U>
> +  U* getDerived() const
> +  {
> +    static_assert(IsBaseOf<T, U>::value, "U must derive from T");
> +    return static_cast<U*>(get());

Same here, this feels like it should at least be private.

@@ +186,5 @@
> +
> +} // namespace detail
> +
> +template<typename T>
> +class SupportsThreadSafeWeakPtr : public external::AtomicRefCounted<T>

It would be great to be able to assert that T didn't declare its own set of AddRef/Release different than the ones it's inheriting from this class.  I'm not quite sure how to do that, though.

@@ +191,5 @@
> +{
> +  typedef external::AtomicRefCounted<T> AtomicRefCounted;
> +  typedef detail::ThreadSafeWeakReference<T> ThreadSafeWeakReference;
> +
> +public:

This feels like these should be protected, since this class is meant to be inherited from and doesn't really have a public API.

@@ +304,5 @@
> +
> +  MOZ_IMPLICIT ThreadSafeWeakPtr(decltype(nullptr))
> +  {}
> +
> +  MOZ_IMPLICIT operator bool() const

Why is this MOZ_IMPLICIT instead of explicit?

@@ +353,5 @@
> +
> +  // Get a pointer to the tracked object, downcasting to the proper type T.
> +  // Note that this operation is unsafe as it may cause races if downwind
> +  // code depends on the value not to change after reading.
> +  T* get() const

Why are we exposing this as a public API?  It's not only racy, it's an opening for UAF bugs.

Comment 5

[Lee Salzman [:lsalzman]]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> Comment on attachment 8914832 [details] [diff] [review]
> add ThreadSafeWeakPointer to MFBT
> 
> Review of attachment 8914832 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some kind of tests for this would be great.  We have gtests for mfbt stuff,
> so that would at least solve the threads part.
> 
> I think I understand all this.  Some comments below.
> 
> ::: mfbt/ThreadSafeWeakPtr.h
> @@ +8,5 @@
> > +
> > +/**
> > + * Derive from SupportsThreadSafeWeakPtr to allow thread-safe weak pointers to an
> > + * atomically refcounted derived class. These thread-safe weak pointers may be safely
> > + * accessed and converted to strong pointers on multiple threads.
> 
> Thanks for this comment.
> 
> We may want to make explicit that we have separated ThreadSafeWeakPtr and
> friends from WeakPtr to minimize overhead?
> 
> @@ +72,5 @@
> > +#endif
> > +
> > +namespace detail {
> > +
> > +// A cheap multiple reader, single writer spin-lock
> 
> It would be nice if the algorithm used for this lock were described in some
> detail up front, rather than spread out over four or five places.
> 
> I think I believe your algorithm works, but I also did some digging to see
> if anybody else has come up with such an idea before and found:
> 
> https://jfdube.wordpress.com/2014/01/03/implementing-a-recursive-read-write-
> spinlock/
> http://joeduffyblog.com/2009/01/29/a-singleword-readerwriter-spin-lock/
> 
> which splits the counter up into a writer bit (sign bit) and # of readers
> (everything else).  IIUC, this is your scheme, but a little more clearly
> expressed.
> 
> @@ +85,5 @@
> > +  void readLock()
> > +  {
> > +    while (mCounter++ < 0) {
> > +      mCounter--;
> > +    }
> 
> I think it'd be clearer to phrase this as:
> 
> bool success;
> do {
>   success = false;
>   CounterType old = mCounter & 0x7fffffff;
>   CounterType new = old + 1;
>   // maybe MOZ_ASSERT that we didn't overflow--though we have to be careful
> about the compiler
>   // optimizing away the assertion, given that `old + 1` overflowing would
> be undefined behavior...
>   success = mCounter.compareExchange(old, new);
> } while (!success);
> 
> which I guess might be a little more expensive on ARM in the success case? 
> The above loop avoids spurious writes, though, which seems like a nice
> property.  We could rewrite this using std::atomic so the first access was
> relaxed, which I *think* would be OK.
> 
> @@ +102,5 @@
> > +    return mCounter.compareExchange(0, std::numeric_limits<CounterType>::min());
> > +  }
> > +
> > +  // Subtract off the negative value (in case it got increment temporarily) so the counter
> > +  // is now positive again.
> 
> Nit: "in case it got incremented temporarily", yes?
> 
> @@ +105,5 @@
> > +  // Subtract off the negative value (in case it got increment temporarily) so the counter
> > +  // is now positive again.
> > +  void writeUnlock()
> > +  {
> > +    mCounter -= std::numeric_limits<CounterType>::min();
> 
> Really, this is just:
> 
> bool success =
> mCounter.compareExchange(std::numeric_limits<CounterType>::min(), 0);
> MOZ_ASSERT(success);
> 
> right?--at least assuming that people have always successfully done
> tryWriteLock() prior to calling writeUnlock().  I guess the compareExchange
> doesn't work if you're going to speculatively write to mCounter in
> readLock(), but I think you shouldn't be doing that (see above).  This
> formulation feels clearer to me.
> 

In the implementation I wrote, I was trying to avoid read contention. When multiple readers are trying to acquire the lock at once. In the compareExchange version, there will potentially be a spin, whereas there is no spin with the atomic increment (I suppose not quite true on ARM, but at least on x86). The spin is limited to only when there is a write lock being held.

I could rewrite it that way if you really prefer, but I rather liked the idea of avoiding the read spinning.

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: add ThreadSafeWeakPointer to MFBT

Incorporated all feedback.

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: track ScaledFont and UnscaledFont lifetimes with ThreadSafeWeakPointer

Fixed interface rot.

Comment 8

[Lee Salzman [:lsalzman]]

Attachment: add test for ThreadSafeWeakPtr

This test is mostly adapted from TestWeakPtr

Comment 9

[Lee Salzman [:lsalzman]]

Attachment: add ThreadSafeWeakPointer to MFBT

Clang and MSVC were unhappy with the new friend/privacy setup, so I had to restructure things to make them eat it.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8917579 [details] [diff] [review]
add ThreadSafeWeakPointer to MFBT

Review of attachment 8917579 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b9e12aef734
add ThreadSafeWeakPointer to MFBT. r=nfroyd
https://hg.mozilla.org/integration/mozilla-inbound/rev/efd9a7070a08
add test for ThreadSafeWeakPtr. r=nfroyd
https://hg.mozilla.org/integration/mozilla-inbound/rev/16a80fc4e102
track ScaledFont and UnscaledFont lifetimes with ThreadSafeWeakPointer. r=bas

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6b9e12aef734
https://hg.mozilla.org/mozilla-central/rev/efd9a7070a08
https://hg.mozilla.org/mozilla-central/rev/16a80fc4e102