1293922 - Auto-cancelling nsITimer handle

Status: NEW

(Core :: XPCOM, defect, P3)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Idea by :jwwang in bug 1293145 comment 14:
In many cases, a timer held by an object will have their callback operate on that object. If the object is destroyed before the timer fires, the callback would later trigger a UAF, and so in this case the object has to ensure that the timer is cancelled before being destroyed.

To help with this pattern, a modified nsITimer handle could automatically cancel the timer when the handler gets destroyed.

Implementation thoughts: Small class derived from nsCOMPtr<nsITimer>, calling this->cancel() from the destructor if there is an actual timer.
If useful and simple enough, utility methods could be added, e.g.: Easy timer creation, destruction-time callback, etc.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1293922 - TimerUtils for easier-to-use timers -

TimerUtils provides a number of classes that help with managing timers, from a
simple handle class managing an nsCOMPtr<nsITimer> to classes that accept
lambda callbacks and can safely auto-cancel themselves (if needed) on
destruction.

Review commit: https://reviewboard.mozilla.org/r/88304/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88304/

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1293922 - Use AutoCancelingTimerHandleWithFunction in htmlmediaelement -

Review commit: https://reviewboard.mozilla.org/r/88306/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88306/

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8804213 [details]
Bug 1293922 - TimerUtils for easier-to-use timers -

https://reviewboard.mozilla.org/r/88304/#review87360

I'm kind of skeptical about this, but willing to discuss.  Some comments below.

::: xpcom/glue/TimerUtils.h:17
(Diff revision 1)
> +
> +#include "mozilla/Function.h"
> +
> +// Encapsulates an nsCOMPtr<nsITimer>, with utility methods to create and access
> +// the timer.
> +class TimerHandle

All of these classes should go in `namespace mozilla`.

This is supposed to be a base class that doesn't get used, correct?  If so, it should go in `mozilla::detail::`.

::: xpcom/glue/TimerUtils.h:21
(Diff revision 1)
> +  TimerHandle(const TimerHandle&) = default;
> +  TimerHandle& operator=(const TimerHandle&) = default;

Copying a handle doesn't seem like the right API here: you'd be copying the reference to the timer, not creating another timer.  Either way seems unhelpful.

::: xpcom/glue/TimerUtils.h:23
(Diff revision 1)
> +  TimerHandle(TimerHandle&&) = default;
> +  TimerHandle& operator=(TimerHandle&&) = default;

In a similar vein, I'm not sure if we want to allow moving...what if the timer's callback has a handle to a given class, and we move the handle somewhere new?  I guess the handle to the initial owner is probably refcounted, and therefore the initial owner won't go away until the timer fires.

::: xpcom/glue/TimerUtils.h:27
(Diff revision 1)
> +  // Create a timer instance, override previous one if any.
> +  nsresult Create();
> +  // Create a timer instance if there isn't already one.
> +  nsresult Ensure();
> +  // Cancel timer, if any.
> +  nsresult Cancel();
> +  // Release timer from this handle.
> +  void Release();
> +  // If there is a timer, cancel it and then release it from this handle.
> +  nsresult CancelAndRelease();

I think `Create` and `Ensure` should be `MOZ_MUST_USE`.  `Cancel` and `CancelAndRelease` should be too, though many people don't check the return value of `nsITimer::Cancel` in the first place.  We can encourage better habits with this class.

::: xpcom/glue/TimerUtils.h:55
(Diff revision 1)
> +               "You can't dereference a NULL TimerHandle with operator*().");
> +    return *get();
> +  }
> +
> +private:
> +  nsCOMPtr<nsITimer> mTimerCOMPtr;

Nit: I think we can just call this `mTimer`.

::: xpcom/glue/TimerUtils.h:58
(Diff revision 1)
> +// Timer handle that will cancel its timer when destroyed.
> +// Useful when storing a timer in a class, when that timer callback works on
> +// that class, to avoid UAF.
> +class AutoCancelingTimerHandle : public TimerHandle

I wonder if we shouldn't just make something like:

```
class TimerHandle {
public:
  enum Cancellation {
    CancelOnDestruction,
    RequiresManualCancellation,
  };
  
  Create();
  Ensure();
  Cancel();
  Release();

protected:
  TimerHandle();

};

template<TimerHandle::Cancellation C>
class Timer
{
  ...
};
```

That would make things a little more obvious at the declaration site, maybe a little shorter, and so forth.

::: xpcom/glue/TimerUtils.h:97
(Diff revision 1)
> +  };
> +
> +  // Start a timer, aF will be called if/when the timer fires, and also when
> +  // the timer resource is destroyed (e.g., when the timer is first cancelled,
> +  // or destroyed).
> +  nsresult InitWithCleanup(uint32_t aDelay_ms,

Could this just be an `Init` overload?

Oh, hm, this is test-only.  That's unfortunate.

::: xpcom/glue/TimerUtils.h:110
(Diff revision 1)
> +    NS_DECL_THREADSAFE_ISUPPORTS
> +    NS_DECL_NSITIMERCALLBACK
> +    template <typename AF>
> +    explicit Callback(AF&& aF) : mF(mozilla::Forward<AF>(aF)) {}
> +  private:
> +    virtual ~Callback() = default;

Typically destructors for concrete classes that implement refcounted interfaces don't have to be virtual, because we'll never call `delete` on a interface pointer; `delete` will always be called from a virtual `Release` method, and therefore will always have the correct pointer type.

::: xpcom/glue/TimerUtils.h:122
(Diff revision 1)
> +    NS_DECL_THREADSAFE_ISUPPORTS
> +    NS_DECL_NSITIMERCALLBACK
> +    template <typename AF>
> +    explicit CallbackAndCleaner(AF&& aF) : mF(mozilla::Forward<AF>(aF)) {}
> +  private:
> +    virtual ~CallbackAndCleaner()

Likewise here.

::: xpcom/tests/TestTimers.cpp:467
(Diff revision 1)
> +  rv = timer.Ensure();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsIEventTarget* target = static_cast<nsIEventTarget*>(testThread);
> +
> +  rv = timer->SetTarget(target);

Once you do this, you're not permitted to `Cancel` the timer from a thread other than the target thread.  This happens to work for this test, but adding stuff like this utility class and this test will make it difficult to actually enforce that condition. :(

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8804214 [details]
Bug 1293922 - Use AutoCancelingTimerHandleWithFunction in htmlmediaelement -

https://reviewboard.mozilla.org/r/88306/#review87472

Assuming we can come to some sort of agreement on the first part...sure, I guess.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8804213 [details]
Bug 1293922 - TimerUtils for easier-to-use timers -

https://reviewboard.mozilla.org/r/88304/#review87360

Thank you very much for the thorough review, even though you are not yet convinced about my efforts!

Skeptical about what? If this work has low chances of being accepted, I'd prefer to know now.

This work spawned from bug 1293145, where I added a timer in HTMLMediaElement but forgot to cancel it from the destructor (but luckily it was found in time and backed-out before polluting central), then jwwang suggested an auto-cancelling timer.
And as I worked on it in my spare time, it grew to have a few more features that I think could be useful, especially taking lambdas as callbacks.

> All of these classes should go in `namespace mozilla`.
> 
> This is supposed to be a base class that doesn't get used, correct?  If so, it should go in `mozilla::detail::`.

Ok I'll put everything in 'namespace mozilla'.

I intentionally kept TimerHandle public, as I thought it could be useful on its own, mostly thanks to the simpler Create method that hides COM-specific stuff.
In my dreams, this could be a way to distance ourselves from COM timers in the future -- that's a Good Thing, right?

> Copying a handle doesn't seem like the right API here: you'd be copying the reference to the timer, not creating another timer.  Either way seems unhelpful.

As the class is a "handle", it seemed to me that copying it would imply we copy the handle but not the targeted timer itself, just like the old nsCOMPtr would work. Or should I use "Pointer" or "Reference" in the name to make it more obvious?

Copying would be useful if (say) we wanted to capture (with ref-counting) the timer handle in a lambda.

> In a similar vein, I'm not sure if we want to allow moving...what if the timer's callback has a handle to a given class, and we move the handle somewhere new?  I guess the handle to the initial owner is probably refcounted, and therefore the initial owner won't go away until the timer fires.

It would allow TimerHandle being used in a class that itself can be moved.

But I see that it could defeat the initial reason for creating this whole mess, namely having timers that survive their owner.
(And same potential problem with the copy methods actually, as a handle could be copied and then the original could be destroyed.)

So how about the following:
- TimerHandle being a simple handle, we would allow copying&moving, just like the nsCOMPtr it holds.
- AutoCancellingTimerHandle[WithFunction] will *not* allow copying&moving, as this could defeat the auto-cancelling feature.
- TimerHandleWithFunction, not too sure there, probably just disallow copying&moving for now, it's safer. And it someone wants them later on, they/we can revisit...
And for those types we can't move/copy, I suppose we could drop 'Handle' from the name, as this detail wouldn't matter anymore.

> I think `Create` and `Ensure` should be `MOZ_MUST_USE`.  `Cancel` and `CancelAndRelease` should be too, though many people don't check the return value of `nsITimer::Cancel` in the first place.  We can encourage better habits with this class.

Good idea, I'll sprinkle MOZ_MUST_USE wherever something is returned. Yes, it should probably be the default for new code, and later we can remove those that are really annoying!

Though the first use in HTMLMediaElement in the next patch ends up with 'Unused<<' everywhere, as there's nothing I can really do if these calls fail! (What could we possibly do if Cancel fails?)
But I guess at least I had to think about these uses, in case I could actually do something meaningful.

> I wonder if we shouldn't just make something like:
> 
> ```
> class TimerHandle {
> public:
>   enum Cancellation {
>     CancelOnDestruction,
>     RequiresManualCancellation,
>   };
>   
>   Create();
>   Ensure();
>   Cancel();
>   Release();
> 
> protected:
>   TimerHandle();
> 
> };
> 
> template<TimerHandle::Cancellation C>
> class Timer
> {
>   ...
> };
> ```
> 
> That would make things a little more obvious at the declaration site, maybe a little shorter, and so forth.

Not sure about shorter ('Timer<TimerHandler::CancelOnDestruction>' vs 'AutoCancelingTimer'), but it may allow more flexibility in the future, e.g. support sets of features, etc.
I'll look into it...

> Could this just be an `Init` overload?
> 
> Oh, hm, this is test-only.  That's unfortunate.

It cannot be an Init overload, because the compiler cannot pick which one to use when given a lambda -- I've tried!
I also tried using some templating magic to guess whether the given function object had a parameter or not, but couldn't manage to find a solution.

'this is test-only', what do you mean?

> Once you do this, you're not permitted to `Cancel` the timer from a thread other than the target thread.  This happens to work for this test, but adding stuff like this utility class and this test will make it difficult to actually enforce that condition. :(

I copied another test above, that had this SetTarget.
I'll try and rework the test, so that creation, firing, and cancellation all happen on the same thread.

Comment 6

[Nathan Froyd [:froydnj]]

Apologies for the delay in responding to this!

(In reply to Gerald Squelart [:gerald] from comment #5)
> Thank you very much for the thorough review, even though you are not yet
> convinced about my efforts!

You're welcome!

> Skeptical about what? If this work has low chances of being accepted, I'd
> prefer to know now.

I guess I'm skeptical because part 2 doesn't seem like a huge win; maybe it would look different if a lot of timers got updated to use this snazzy new API.  We also lose things with this API: the second part takes away the name of one of the timers, for instance, so we can't get helpful logging output from that.

OTOH, I am a fan of less COM usage, so if we can manage to create something nicer to use from C++, that would be a bonus.

> This work spawned from bug 1293145, where I added a timer in
> HTMLMediaElement but forgot to cancel it from the destructor (but luckily it
> was found in time and backed-out before polluting central), then jwwang
> suggested an auto-cancelling timer.
> And as I worked on it in my spare time, it grew to have a few more features
> that I think could be useful, especially taking lambdas as callbacks.
> 
> I intentionally kept TimerHandle public, as I thought it could be useful on
> its own, mostly thanks to the simpler Create method that hides COM-specific
> stuff.
> In my dreams, this could be a way to distance ourselves from COM timers in
> the future -- that's a Good Thing, right?

I think this is a good thing, yes.

> > Copying a handle doesn't seem like the right API here: you'd be copying the reference to the timer, not creating another timer.  Either way seems unhelpful.
> 
> As the class is a "handle", it seemed to me that copying it would imply we
> copy the handle but not the targeted timer itself, just like the old
> nsCOMPtr would work. Or should I use "Pointer" or "Reference" in the name to
> make it more obvious?
> 
> Copying would be useful if (say) we wanted to capture (with ref-counting)
> the timer handle in a lambda.
> 
> > In a similar vein, I'm not sure if we want to allow moving...what if the timer's callback has a handle to a given class, and we move the handle somewhere new?  I guess the handle to the initial owner is probably refcounted, and therefore the initial owner won't go away until the timer fires.
> 
> It would allow TimerHandle being used in a class that itself can be moved.
> 
> But I see that it could defeat the initial reason for creating this whole
> mess, namely having timers that survive their owner.
> (And same potential problem with the copy methods actually, as a handle
> could be copied and then the original could be destroyed.)
> 
> So how about the following:
> - TimerHandle being a simple handle, we would allow copying&moving, just
> like the nsCOMPtr it holds.
> - AutoCancellingTimerHandle[WithFunction] will *not* allow copying&moving,
> as this could defeat the auto-cancelling feature.
> - TimerHandleWithFunction, not too sure there, probably just disallow
> copying&moving for now, it's safer. And it someone wants them later on,
> they/we can revisit...

I suppose we're disallowing it in TimerHandleWithFunction because we're not sure what to do with the mozilla::function instance?

> And for those types we can't move/copy, I suppose we could drop 'Handle'
> from the name, as this detail wouldn't matter anymore.

Yeah, I am all for making this mozilla::Timer and having it be the canonical way to interact with timers.

> Though the first use in HTMLMediaElement in the next patch ends up with
> 'Unused<<' everywhere, as there's nothing I can really do if these calls
> fail! (What could we possibly do if Cancel fails?)
> But I guess at least I had to think about these uses, in case I could
> actually do something meaningful.

Yeah, Unused<< at least shows that you've thought about the error behavior a little bit (even if it's just "I have to make the compiler happy so I can compile my code!").

> Not sure about shorter ('Timer<TimerHandler::CancelOnDestruction>' vs
> 'AutoCancelingTimer'), but it may allow more flexibility in the future, e.g.
> support sets of features, etc.
> I'll look into it...

My main concern is that when you have a class hierarchy and you wind up having a bunch of leaf classes that add on the same feature, or you end up with FooWithX, FooWithY, etc., it's usually an indication that something is not right in the design.

> > Could this just be an `Init` overload?
> > 
> > Oh, hm, this is test-only.  That's unfortunate.
> 
> It cannot be an Init overload, because the compiler cannot pick which one to
> use when given a lambda -- I've tried!
> I also tried using some templating magic to guess whether the given function
> object had a parameter or not, but couldn't manage to find a solution.

Ah, OK, that's fair.

> 'this is test-only', what do you mean?

I mean that it's unfortunate that we have a public API, but parts of it are only used in tests; it would be better in some sense to have that API be solely in test code.  Maybe you would have to make part of the class protected so test code could inherit from it, or something.  I can live with things being test-only if it makes testing easier, I guess.

> > Once you do this, you're not permitted to `Cancel` the timer from a thread other than the target thread.  This happens to work for this test, but adding stuff like this utility class and this test will make it difficult to actually enforce that condition. :(
> 
> I copied another test above, that had this SetTarget.
> I'll try and rework the test, so that creation, firing, and cancellation all
> happen on the same thread.

Hm, I guess there are other tests that do SetTarget; TestTargetedTimers doesn't Cancel the timer, and neither does TestTimerWithStoppedTarget.  I think FuzzTestTimers does, though. :(

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(Not enough time for this, de-assigning myself in case someone else wants to work on it. -- Sorry for the waste of time Nathan, hopefully someone can reuse this later...)