Commit 47ab497b authored by Markus Stange's avatar Markus Stange
Browse files

Bug 1781167 - Allow stacking calls to Add/RemoveVsyncDispatcher so that we... 

Bug 1781167 - Allow stacking calls to Add/RemoveVsyncDispatcher so that we survive the sequence Add,Add,Remove. r=jrmuizel, a=RyanVM

This fixes a bug which caused Firefox windows to become frozen after some time.

Full credit goes to Susan and RandyS for bisecting the regressor of this bug, and
to Jeff DeFouw for debugging the issue and finding the cause.

The bug here is a "state race" between the VsyncDispatcher state and
the VsyncSource state. Both are protected by locks, and the code that
runs in those locks respectively can see a different orders of invocations.

VsyncDispatcher::UpdateVsyncStatus does this thing where it updates its state inside
a lock, gathers some information, and then calls methods on VsyncSource *outside* the lock.
Since it calls those methods outside the lock, these calls can end up being executed
in a different order than the state changes were observed inside the lock.

Here's the bad scenario in detail, with the same VsyncDispatcher being used from
two different threads, turning a Remove,Add into an Add,Remove:

```
Thread A                                       Thread B

VsyncDispatcher::UpdateVsync
 |
 |----> Enter VsyncDispatcher lock
 |    |                                         VsyncDispatcher::UpdateVsync
 |    |   state->mIsObservingVsync = false       |
 |    |   (We want to stop listening)            |
 |    |                                          |
 |<---- Exit VsyncDispatcher lock                |
 |                                               |----> Enter VsyncDispatcher lock
 |                                               |    |
 |                                               |    |   state->mIsObservingVsync = true
 |                                               |    |   (We want to start listening)
 |                                               |    |
 |                                               |<----  Exit VsyncDispatcher lock
 |                                               |
 |                                               |----> Enter VsyncSource::AddVsyncDispatcher
 |                                               |    |
 |                                               |    |----> Enter VsyncSource lock
 |                                               |    |    |
 |                                               |    |    |  state->mDispatchers.Contains(aVsyncDispatcher)
 |----> VsyncSource::RemoveVsyncDispatcher       |    |    |  VsyncDispatcher already present in list, not doing anything
 |    |                                          |    |    |
 |    |                                          |    |<---- Exit VsyncSource lock
 |    |                                          |    |
 |    |                                          |<---- Exit VsyncSource::AddVsyncDispatcher
 |    |----> Enter VsyncSource lock
 |    |    |
 |    |    |  Removing aVsyncDispatcher from state->mDispatchers
 |    |    |
 |    |<---- Exit VsyncSource lock
 |    |
 |<---- Exit VsyncSource::AddVsyncDispatcher
```

Now the VsyncDispatcher thinks it is still observing vsync, but it is
no longer registered with the VsyncSource.

This patch makes it so that two calls to AddVsyncDispatcher followed by one call
to RemoveVsyncDispatcher result in the VsyncDispatcher still being registered.
AddVsyncDispatcher is no longer idempotent.

Differential Revision: https://phabricator.services.mozilla.com/D162760
parent 80d37b91

gfx/thebes/VsyncSource.cpp

+32 −5
Original line number Diff line number Diff line
@@ -27,7 +27,7 @@ VsyncSource::~VsyncSource() { MOZ_ASSERT(NS_IsMainThread()); } 
void VsyncSource::NotifyVsync(const TimeStamp& aVsyncTimestamp,

                              const TimeStamp& aOutputTimestamp) {

  VsyncId vsyncId;

  nsTArray<RefPtr<VsyncDispatcher>> dispatchers;

  nsTArray<DispatcherRefWithCount> dispatchers;



  {

    auto state = mState.Lock();
@@ -39,7 +39,7 @@ void VsyncSource::NotifyVsync(const TimeStamp& aVsyncTimestamp, 
  // Notify our listeners, outside of the lock.

  const VsyncEvent event(vsyncId, aVsyncTimestamp, aOutputTimestamp);

  for (const auto& dispatcher : dispatchers) {

    dispatcher->NotifyVsync(event);

    dispatcher.mDispatcher->NotifyVsync(event);

  }

}
@@ -47,8 +47,20 @@ void VsyncSource::AddVsyncDispatcher(VsyncDispatcher* aVsyncDispatcher) { 
  MOZ_ASSERT(aVsyncDispatcher);

  {

    auto state = mState.Lock();

    if (!state->mDispatchers.Contains(aVsyncDispatcher)) {

      state->mDispatchers.AppendElement(aVsyncDispatcher);



    // Find the dispatcher in mDispatchers. If it is already present, increment

    // the count. If not, add it with a count of 1.

    bool found = false;

    for (auto& dispatcherRefWithCount : state->mDispatchers) {

      if (dispatcherRefWithCount.mDispatcher == aVsyncDispatcher) {

        dispatcherRefWithCount.mCount++;

        found = true;

        break;

      }

    }

    if (!found) {

      state->mDispatchers.AppendElement(

          DispatcherRefWithCount{aVsyncDispatcher, 1});

    }

  }
@@ -59,7 +71,22 @@ void VsyncSource::RemoveVsyncDispatcher(VsyncDispatcher* aVsyncDispatcher) { 
  MOZ_ASSERT(aVsyncDispatcher);

  {

    auto state = mState.Lock();

    state->mDispatchers.RemoveElement(aVsyncDispatcher);



    // Find the dispatcher in mDispatchers. If found, decrement the count.

    // If the count becomes zero, remove it from mDispatchers.

    for (auto it = state->mDispatchers.begin(); it != state->mDispatchers.end();

         ++it) {

      if (it->mDispatcher == aVsyncDispatcher) {

        it->mCount--;

        if (it->mCount == 0) {

          state->mDispatchers.RemoveElementAt(it);

        }

        break;

      }

    }



    // In the future we should probably MOZ_RELEASE_ASSERT here that we don't

    // try to remove a dispatcher which isn't in mDispatchers.

  }



  UpdateVsyncStatus();

gfx/thebes/VsyncSource.h

+14 −2
Original line number Diff line number Diff line
@@ -53,7 +53,10 @@ class VsyncSource { 
  virtual void NotifyVsync(const TimeStamp& aVsyncTimestamp,

                           const TimeStamp& aOutputTimestamp);



  // Can be called on any thread

  // Can be called on any thread.

  // Adding the same dispatcher multiple times will increment a count.

  // This means that the sequence "Add, Add, Remove" has the same behavior as

  // "Add, Remove, Add".

  void AddVsyncDispatcher(VsyncDispatcher* aDispatcher);

  void RemoveVsyncDispatcher(VsyncDispatcher* aDispatcher);
@@ -75,12 +78,21 @@ class VsyncSource { 
  // Can be called on any thread

  void UpdateVsyncStatus();



  struct DispatcherRefWithCount {

    // The dispatcher.

    RefPtr<VsyncDispatcher> mDispatcher;

    // The number of add calls minus the number of remove calls for this

    // dispatcher. Should always be > 0 as long as this dispatcher is in

    // mDispatchers.

    size_t mCount = 0;

  };



  struct State {

    // The set of VsyncDispatchers which are registered with this source.

    // At the moment, the length of this array is always zero or one.

    // The ability to register multiple dispatchers is not used yet; it is

    // intended for when we have one dispatcher per widget.

    nsTArray<RefPtr<VsyncDispatcher>> mDispatchers;

    nsTArray<DispatcherRefWithCount> mDispatchers;



    // The vsync ID which we used for the last vsync event.

    VsyncId mVsyncId;