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

NEW
Unassigned

Status

()

P5
normal
4 years ago
3 years ago

People

(Reporter: dholbert, Unassigned)

Tracking

Trunk
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

4 years ago
[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.)
(Reporter)

Updated

4 years ago
See Also: → bug 1142841
Yes, we should convert them to nsRefPtr, and we should fix up MediaTaskQueue to take an already_AddRefed instead of a TemporaryRef.
(Reporter)

Comment 2

4 years ago
(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")
(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.
Switching to nsCOMPtr sounds good to me.
(Reporter)

Comment 5

4 years ago
(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.)
(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).
Component: Audio/Video → Audio/Video: Playback
Is this still an issue?
(Reporter)

Comment 8

3 years ago
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.
The documentation isn't very compelling with its reasoning.
(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.
(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.
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.