Open Bug 1436152 Opened 6 years ago Updated 2 years ago

Add a method to nsAPostRefreshObserver to signal that the RefreshDriver is going away to avoid the potential for leaks

Categories

(Core :: Graphics, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: mconley, Unassigned)

References

Details

(Whiteboard: [gfx-noted])

nsAPostRefreshObserver is an abstract class for observers that can be registered with nsRefreshDrivers to be notified when nsRefreshDriver ticks complete.

At this point, management for the lifetime of the observers is up to the consumers of the API. A pattern has cropped up where consumers of the API only ever want to hear about the next tick, and so have their observers unregister themselves after that tick, like so:


class SomeObserver : public nsAPostRefreshObserver
{
public:
  SomeObserver(nsRefreshDriver* aRefreshDriver)
    : mRefreshDriver(aRefreshDriver)
  {
  }
  virtual ~CounterStyleCleaner() {}

  void DidRefresh() final override
  {
    // Do some stuff now that the tick has happened
    // ...
    mRefreshDriver->RemovePostRefreshObserver(this);
    delete this;
  }

private:
  RefPtr<nsRefreshDriver> mRefreshDriver;
};

// and then later one, when observation is requested:
// ...

   someRefreshDriver->AddPostRefreshObserver(
     new SomeObserver(someRefreshDriver);

So the implicit assumption here is that the nsRefreshDriver will tick, and that will end the life of the observer. However, it's not guaranteed that an nsRefreshDriver will tick before it gets destroyed. This means that the nsRefrehsDriver could get deleted, and the observers will never get fired, meaning we're leaking.

We want to probably continue supporting the use-case for consumers of this API to register and then unregister their observers independent of any ticking, so I suspect that putting the lifetime management of these things into the hands of nsRefreshDriver isn't a great idea.

Instead, bz and I were talking about adding a new cleanup method to nsAPostRefreshObserver that gets called on each remaining observer registered with an nsRefreshDriver just before the nsRefreshDriver gets destroyed. That would give each consumer the opportunity to delete its observers if it's using the above pattern.

There are probably other patterns we could use here, but certainly we should try to plug this potential hole for leaking.
Matt will probably want to know about this and may know other people that should be CC'ed here.
Whiteboard: [gfx-noted]
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.