1143887 - Media code has a lot of "RefPtr<nsIRunnable>", which goes against rule about "interface pointers should be stored in nsCOMPtr"

Status: NEW

(Core :: Audio/Video: Playback, defect, P5)

Description

[Daniel Holbert [:dholbert]]

[I'm spinning this off of bug 1142841.]

As documented athttps://developer.mozilla.org/en-US/docs/nsRefPtr,
"Any time you are holding an XPCOM interface pointer, you should be using nsCOMPtr."

We have a bunch of code that violates this rule in /dom/media, with RefPtr<nsIRunnable> and TemporaryRef<nsIRunnable>.

This seems to be centered around MediaTaskQueue usage. MediaTaskQueue manages a queue of runnables, and its API is very RefPtr<nsIRunnable> flavored -- e.g. Dispatch takes TemporaryRef<nsIRunnable>.  So, to play nicely with this API, all of its clients use RefPtr / TemporaryRef<nsIRunnable> as a result. Hence, there's a bunch of code using these types.

To the extent that we care about preserving the above-quoted guideline, we should either:
 (1) convert these pointers to nsCOMPtr -- so that we end up with nsCOMPtr<nsIRunnable> and already_AddRefed<nsIRunnable> instead of RefPtr/TemporaryRef.
...or...
 (2) convert the template-argument to be a non-interface-flavored type -- so, so e.g. RefPtr<nsRunnable> (no "I") instead of RefPtr<nsIRunnable>

(I'd lean towards option #1, since last I heard, there was some amount of consensus that RefPtr usage is a Bad Thing and we should just stick to our traditional nsCOMPtr/nsRefPtr types; but #2 is fine as well, if folks working with the code here prefer RefPtr.)

Comment 1

[Bobby Holley (:bholley)]

Yes, we should convert them to nsRefPtr, and we should fix up MediaTaskQueue to take an already_AddRefed instead of a TemporaryRef.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Bobby Holley (:bholley) from comment #1)
> Yes, we should convert them to nsRefPtr

Cool, so sounds like we should go with option (1) then.  (though I think you mean "to nsCOMPtr" (not "to nsRefPtr"), since we've got a pointer-to-interface type "nsIRunnable")

Comment 3

[Bobby Holley (:bholley)]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> (In reply to Bobby Holley (:bholley) from comment #1)
> > Yes, we should convert them to nsRefPtr
> 
> Cool, so sounds like we should go with option (1) then.  (though I think you
> mean "to nsCOMPtr" (not "to nsRefPtr"), since we've got a
> pointer-to-interface type "nsIRunnable")

Yes, sorry.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Switching to nsCOMPtr sounds good to me.

Comment 5

[Daniel Holbert [:dholbert]]

(Note: IIRC TemporaryRef and already_AddRefed have different semantics, but that may not matter for their usages here. I don't recall the details offhand.)

Comment 6

[Bobby Holley (:bholley)]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> (Note: IIRC TemporaryRef and already_AddRefed have different semantics, but
> that may not matter for their usages here. I don't recall the details
> offhand.)

The big difference that I'm aware of is that, if you have RefPtr<T> x, both |x| and |x.forget()| coerce to a TemporaryRef<T>, whereas only .forget() coerces to already_AddRefed. So TemporaryRef here is a bit more flexible in allowing the caller to decide whether or not to donate its ref to the receiver. But in practice I don't think it matters much here, since we basically never want to touch a task object after it's been dispatched (so forcing .forget() is actually desirable here).

Comment 7

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Is this still an issue?

Comment 8

[Daniel Holbert [:dholbert]]

Yes, as shown by:
  http://mxr.mozilla.org/mozilla-central/search?string=RefPtr%3CnsIRunnable&find=dom%2Fmedia
("Found 28 matching lines in 10 files")

These should all be nsCOMPtr, per XPCOM documentation quoted in comment 0.

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

The documentation isn't very compelling with its reasoning.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #9)
> The documentation isn't very compelling with its reasoning.

I believe it largely boils down to code size. Assuming that at least one in-tree consumer uses nsCOMPtr<nsIFoo> (in order to take advantage of do_QueryInterface etc), we should avoid generating redundant-but-slightly-different code for nsCOMPtr<nsIFoo> and nsRefPtr<nsIFoo>.

There's also the business of RefPtr vs nsRefPtr, but I believe those two have been merged by this point.

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Bobby Holley (busy) from comment #10)
> I believe it largely boils down to code size.

We probably have bigger fish to fry.