Closed Bug 1329385 Opened 5 years ago Closed 4 years ago

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

Categories

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

49 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

Details

(Keywords: stale-bug)

Attachments

(1 file)

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
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.
Keywords: stale-bug
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Depends on: 1418047
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().
Attachment #8827658 - Flags: review?(jacheng) → review+
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?
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/68cf31510d21
Fix leak in GMPServiceParent::GetContentParent - r=JamesCheng
https://hg.mozilla.org/mozilla-central/rev/68cf31510d21
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.