1402728 - Reduce occurrences of TrackBuffersManager raw pointers

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement)

Description

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

Bug 1247189 and bug 1402681 show that there may be some issues around TrackBuffersManager's lifetime management.
There are a few places where TrackBuffersManagers are stored or accessed through raw pointers.

To eliminate this potential (yet-unproven) risk, I would like to try and remove most of these raw pointers.

Some of these changes may seem superfluous, as it could be reasoned that bad situations may not happen for them, however I think it's worth reducing raw pointer usage anyway:
- Maybe we haven't thought of something sneaky involving them,
- If we remove raw pointers from the code, at least we won't have to waste time thinking about them again, next time we get strange crash reports,
- In most cases the cost of using RefPtrs should be null or negligible.

(I'm opening a separate bug, and keeping the other two open, because I'm not confident that this will solve the actual problem.)

Comment 1

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

Attachment: Bug 1402728 - Use RefPtr<TrackBuffersManager> instead of raw pointers -

All in MediaSourceDemuxer:
- GetManager returns a RefPtr<TBM>. Probably unneeded as the only call stores
the pointer in a RefPtr, but I'm adding it for peace of mind, and it's very
low cost.
- AttachSourceBuffer stores the given TBM in a RefPtr in the dispatched task,
then DoAttachSourceBuffer takes a RefPtr<TBM>&&. This one might actually be
needed, to ensure that the given TBM cannot be destroyed between the dispatch
and the task running. There should be a tiny cost: Though the TBM is AddRef'd
early, it means we don't AddRef it in the task (thanks to Move); the only cost
is a nullptr check in the added ~RefPtr() in the task.
-  DetachSourceBuffer stores the given TBM in a RefPtr in the dispatched task,
then DoDetachSourceBuffer takes a RefPtr<TBM>&&; And changed the loop to a
safer RemoveElementsBy. Most likely not needed, but cheap enough for the extra
peace of mind (that the TBM won't be destroyed during task transit).

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

Comment 2

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

Oh, I've just noticed&remembered that `NewRunnableMethod<TBM*>` should automatically use RefPtr (because TBM is ref-counted).

I'll keep the changes as it makes this extra clear, and even makes it a tiny bit more optimized.
But I'll update the commit comment to avoid confusion.

Comment 3

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

Comment on attachment 8911620 [details]
Bug 1402728 - Use RefPtr<TrackBuffersManager> instead of raw pointers -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183030/diff/1-2/

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8911620 [details]
Bug 1402728 - Use RefPtr<TrackBuffersManager> instead of raw pointers -

https://reviewboard.mozilla.org/r/183030/#review188234

::: dom/media/mediasource/MediaSourceDemuxer.cpp:170
(Diff revision 2)
>    *crypto = mInfo.mCrypto;
>    return crypto;
>  }
>  
>  void
>  MediaSourceDemuxer::AttachSourceBuffer(TrackBuffersManager* aSourceBuffer)

what about the argument type?
if we want to be extra anal about it.

Comment 5

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

Comment on attachment 8911620 [details]
Bug 1402728 - Use RefPtr<TrackBuffersManager> instead of raw pointers -

https://reviewboard.mozilla.org/r/183030/#review188234

> what about the argument type?
> if we want to be extra anal about it.

I considered it, but I didn't think it'd be useful here: The caller should already own the object during this call -- Whereas the other changes guarantee that we have extended the object lifetime after the dispatch, and until the end of the task.

Though I suppose if I'm going for peace of mind, passing a RefPtr& would give us the assurance that the caller does own the object, at (I think) no cost; So ok I'll add it!
Since you brought it up, I'll assume you approve.

Comment 6

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

Comment on attachment 8911620 [details]
Bug 1402728 - Use RefPtr<TrackBuffersManager> instead of raw pointers -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183030/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05b222ac8a54
Use RefPtr<TrackBuffersManager> instead of raw pointers - r=jya

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/05b222ac8a54

Comment 9

[Karl Tomlinson (:karlt)]

The patches submitted for review are still available at https://hg.mozilla.org/mozreview/gecko/log?rev=1402728

The reviewed change was https://hg.mozilla.org/mozreview/gecko/rev/a26d9a57c76eae37effe9892b82c404c0326935f