1157057 - Rewrite the handling of the nsITimer object in nrappkitTimerCallback

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[(no longer active)]

This patch makes the code use smart pointers more for dealing with
the nsITimer object, and it also avoid an unneeded AddRef and
Release pair.

Comment 1

[(no longer active)]

Attachment: Rewrite the handling of the nsITimer object in nrappkitTimerCallback

Comment 2

[(no longer active)]

Comment on attachment 8595660 [details] [diff] [review]
Rewrite the handling of the nsITimer object in nrappkitTimerCallback

Randell, can you please help review?  I'd like to land this sooner than later.  Thanks!

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8595660 [details] [diff] [review]
Rewrite the handling of the nsITimer object in nrappkitTimerCallback

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

leaving byron on here - byron, feel free to decide another reviewer isn't needed and remove yourself

::: media/mtransport/nr_timer.cpp
@@ +211,5 @@
>    }
>  
> +  // Move the ownership of the timer to the callback object, which holds the
> +  // timer alive per spec.
> +  callback->SetTimer(timer.forget());

I suppose this is safe since the timer calls us back via Notify on the thread we created the time on (i.e. this one), so even if the timer fired before this, we can't have run Notify() and cleared the timer_ ptr in the callback yet.  I'm not in love with the pattern here as it's based on some hidden assumptions, but those assumptions are valid.  The alternative would be to add another ref and then drop it (SetTimer(rawptr) (adds a ref) before InitWithCallback(), then we drop our ref here at the end of the func)

Instead, add a 1-liner comment about why this is safe

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8595660 [details] [diff] [review]
Rewrite the handling of the nsITimer object in nrappkitTimerCallback

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

::: media/mtransport/nr_timer.cpp
@@ +109,5 @@
>      Release(); // Will cause deletion of this object.
>    }
>  
>   private:
> +  nsCOMPtr<nsITimer> timer_;

This introduces a cycle; the timer impl is holding a ref to us (via nsITimerCallback). As long as either Notify() or Cancel() fires, we're probably ok, but I don't know if we get a guarantee of that in all cases (eg; on shutdown). You're probably going to have to talk to ekr on this one, since he's spent more time thinking about this particular piece of code.

Comment 5

[(no longer active)]

Timers will either be canceled or notified.

Comment 6

[(no longer active)]

ping?

Comment 7

[Eric Rescorla (:ekr)]

Does this patch remedy some functional deficiency or do you just think it's cleaner?

Comment 8

[(no longer active)]

(In reply to Eric Rescorla (:ekr) from comment #7)
> Does this patch remedy some functional deficiency or do you just think it's
> cleaner?

It's the last thing to fix before I can land the analysis in bug 1156084, which is why I care about it.

Comment 9

[Eric Rescorla (:ekr)]

All right. I don't generally think this is an improvement but I'm willing to make this change for that reason.

Comment 10

[Eric Rescorla (:ekr)]

To update, will take a look this week

Comment 11

[Eric Rescorla (:ekr)]

Comment on attachment 8595660 [details] [diff] [review]
Rewrite the handling of the nsITimer object in nrappkitTimerCallback

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

LGTM if you make the indicated style changes.

::: media/mtransport/nr_timer.cpp
@@ +93,5 @@
>    NS_DECL_NSITIMERCALLBACK
>  
>    nrappkitTimerCallback(NR_async_cb cb, void *cb_arg,
> +                        const char *function, int line)
> +      : nrappkitCallback(cb, cb_arg, function, line) {}

please explicitly initialize timer_(nullptr), as that's the convention in this code.

@@ +96,5 @@
> +                        const char *function, int line)
> +      : nrappkitCallback(cb, cb_arg, function, line) {}
> +
> +  void SetTimer(already_AddRefed<nsITimer>&& aTimer)
> +  {

This code follows the Google not Mozilla style guidelines. Convention is:

1. Don't use aFoo
2. Braces are on the opening line.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/756238780243

Comment 13

[Phil Ringnalda (:philor)]

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