1329385 - GeckoMediaPluginServiceParent::GetContentParent risks leaking a UniquePtr by passing it raw to lambdas, which could potentially not run

Status: RESOLVED FIXED

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

Description

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

In C++11, it is not possible to pass a UniquePtr to a lambda, because captures can only be done by copy, not move.

GeckoMediaPluginServiceParent::GetContentParent currently goes around this limitation by extracting a raw pointer and passing it to two lambdas in a MozPromise.Then(), one of which should eventually run and put the raw pointer back into a UniquePtr.
I am not sure whether it is possible in this particular case, but the Then() could theoretically be disconnected, so no lambda would run and the data pointed at by the raw pointer would leak.

To fix this, we first need two features:
- Bug 1322964 will allow using only 1 lambda instead of two, so only one capture should exist, which can uniquely own the pointee.
- Bug 1325632 will enable the use of C++14, which is necessary to move the initial UniquePtr into the lambda capture (through an initialized by-copy capture).

For reference, an initial implementation (using an nsAutoPtr-like hack) can be seen there: https://reviewboard.mozilla.org/r/98244/diff/3/#0

Comment 1

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

Attachment: Bug 1329385 - Fix leak in GMPServiceParent::GetContentParent -

Using a single-function MozPromise::Then, and capturing a UniquePtr (using
C++14 initialized lambda captures), the PromiseHolder cannot leak anymore.

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

Comment 2

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 3

[Bulk Bug Changes for mreavy's org]

Mass change P1->P2 to align with new Mozilla triage process

Comment 4

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

Comment on attachment 8827658 [details]
Bug 1329385 - Fix leak in GMPServiceParent::GetContentParent -

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

Comment 5

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

Comment on attachment 8827658 [details]
Bug 1329385 - Fix leak in GMPServiceParent::GetContentParent -

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

Comment 6

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

Comment on attachment 8827658 [details]
Bug 1329385 - Fix leak in GMPServiceParent::GetContentParent -

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

Comment 7

[James Cheng[:JamesCheng]]

Comment on attachment 8827658 [details]
Bug 1329385 - Fix leak in GMPServiceParent::GetContentParent -

https://reviewboard.mozilla.org/r/105278/#review208786

LGTM. Thanks.

::: dom/media/gmp/GMPServiceParent.cpp:376
(Diff revision 4)
> -  nsCString nodeIdString(aNodeIdString);
> -  nsTArray<nsCString> tags(aTags);
> -  nsCString api(aAPI);
> -  RefPtr<GMPCrashHelper> helper(aHelper);
> -  EnsureInitialized()->Then(
> -    thread,
> +    nodeIdString = nsCString(aNodeIdString),
> +    api = nsCString(aAPI),
> +    tags = nsTArray<nsCString>(aTags),
> +    helper = RefPtr<GMPCrashHelper>(aHelper),
> +    holder = Move(holder)
> +  ](const GenericPromise::ResolveOrRejectValue& aValue) mutable->void {

"mutable -> void"

::: dom/media/gmp/GMPServiceParent.cpp:378
(Diff revision 4)
> -  nsCString api(aAPI);
> -  RefPtr<GMPCrashHelper> helper(aHelper);
> -  EnsureInitialized()->Then(
> -    thread,
> -    __func__,
> -    [self, tags, api, nodeIdString, helper, rawHolder]() -> void {
> +    tags = nsTArray<nsCString>(aTags),
> +    helper = RefPtr<GMPCrashHelper>(aHelper),
> +    holder = Move(holder)
> +  ](const GenericPromise::ResolveOrRejectValue& aValue) mutable->void {
> +    if (aValue.IsReject()) {
> +      NS_WARNING("GMPService::EnsureInitialized failed.");

Please log down the nsresult by aValue.RejectValue().

Comment 8

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

Comment on attachment 8827658 [details]
Bug 1329385 - Fix leak in GMPServiceParent::GetContentParent -

https://reviewboard.mozilla.org/r/105278/#review208786

> "mutable -> void"

That's what clang-format produced! (See https://reviewboard.mozilla.org/r/105278/diff/3-4/ for what clang-format did.)
But I agree that it's ugly, so I'll change it back.

> Please log down the nsresult by aValue.RejectValue().

Overall the code should have exactly the same side effects as before; adding logging would be out-of-scope for this bug.
If you want more logging, could you please open another bug, and work on it?

Comment 9

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

Comment on attachment 8827658 [details]
Bug 1329385 - Fix leak in GMPServiceParent::GetContentParent -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105278/diff/4-5/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68cf31510d21
Fix leak in GMPServiceParent::GetContentParent - r=JamesCheng

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/68cf31510d21