Closed Bug 1124021 Opened 5 years ago Closed 5 years ago

Fix dangerous UniquePtr usage pattern in GMP.

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: kinetik, Assigned: kinetik)

References

Details

Attachments

(2 files)

No description provided.
Attached patch patch v0Splinter Review
Comment on attachment 8552160 [details] [diff] [review]
patch v0

As I understand it, the correct (and safe) way to declare a UniquePtr with a custom deleter is to pass the deleter in as a type argument to the UniquePtr.  DefaultDelete seems to be intended as internal-only and to be used if there's no customer deleter.  The main danger with declaring a custom DefaultDelete instead of passing in a deleter is that it's possible to then declare a UniquePtr to that type in code that does not have the appropriate DefaultDelete implementation available, which will result in a leak or crash.

If the DefaultDelete implementation is not visible and you're lucky, you might get a warning about a non-virtual destructor, but otherwise the incorrect behaviour will be used silently.

In this case, I've introduced a templated typedef to make the resulting code more palatable, as otherwise you either need a custom deleter per class or to pass the class twice (e.g. mozilla::UniquePtr<GMPVideoEncodedFrame, DestroyPolicy<GMPVideoEncodedFrame>>).
Attachment #8552160 - Flags: review?(cpearce)
There doesn't seem to be a good way to prevent 'default'. If you forget to define your own custom, the default just kicks in. Is it possible to delete the destructor of GMPVideoFrame so that it is never legal to call |GMPVideoFrame* ptr; delete ptr;|?
Attachment #8552160 - Flags: review?(cpearce) → review+
(In reply to JW Wang [:jwwang] from comment #3)
> There doesn't seem to be a good way to prevent 'default'. If you forget to
> define your own custom, the default just kicks in. Is it possible to delete
> the destructor of GMPVideoFrame so that it is never legal to call
> |GMPVideoFrame* ptr; delete ptr;|?

I thought about changing the destructor to be private, like we did for refcounted classes, but it's trickier here because some of these things (or their subclasses) are also used on the stack.
https://hg.mozilla.org/mozilla-central/rev/e622fbf72551
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
This patch breaks building with gcc 4.6 because it uses template aliasing.
Flags: needinfo?(kinetik)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7)
> This patch breaks building with gcc 4.6 because it uses template aliasing.

Will fix in bug 1131340.
Flags: needinfo?(kinetik)
Depends on: 1131340
Attached patch Beta patchSplinter Review
Patch for beta branch as part of EME platform uplift.
Comment on attachment 8572317 [details] [diff] [review]
Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572317 - Flags: approval-mozilla-beta?
Comment on attachment 8572317 [details] [diff] [review]
Beta patch

Approved for Beta as part of EME platform uplift.
Attachment #8572317 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.