1322095 - replace nsAutoPtr with UniquePtr in media code base

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

UniquePtr is safer, clearer to read and nsAutoPtr is deprecated...

Comment 1

[Eric Rahm [:erahm]]

I have a WIP that I've been slowly iterating on which I'll post shortly. This isn't as straightforward as one might hope, our use of RunnableWrapper complicates things:

1. Various callbacks take an nsAutoPtr which has a copy constructor that steals the contents of the copied nsAutoPtr (so the old-school pretend copy is a move style).
2. UniquePtr has an explicitly deleted copy constructor, so it can't be used in as a drop-in replacement
3. We can modify the runnable_args helpers use std::apply which should work alright with moving the args, that'll make UniquePtr happy
4. That means we need to switch from mozilla::Tuple to std::tuple as std::apply expects a few attributes (it's possible we could add that to Tuple but I try to avoid mfbt)
5. std::tuple throws a fit if you try to store a type that has a non-const copy constructor, which of course nsAutoPtr has as noted in point 1.
6. That means we can't update things incrementally, hence 1 large patch
7. Another side-effect is that the callback params can't be non-const refs as a result of moving the args, this also means things that expected RefPtr to auto-convert to it's pointer type can no longer do so because we explicitly delete the T* operator for r-values

So all that said the patch does a few things:

• Updates WrapRunnables so that they move their args when running and adds/updates some tests
• Converts anything that was using WrapRunnable with nsAutoPtr to UniquePtr (this hits a few things outside of media/)
• Converts anything that was using a non-const ref as a param to either a const ref or a by-val copy
• Converts the remaining nsAutoPtr usage to UniquePtr

Comment 2

[Eric Rahm [:erahm]]

Attachment: Bug 1322095 - Part 1a: Move WrapRunnable's args when running. r=jya

In order to support UniquePtr as an arg type for WrapRunnable as well as
avoid unnecessary copies in the future we need to switch to moving args when
invoking a runnable.

This updates WrapRunnables so that they move their args when running and
adds/updates some tests. To accomplish this std::apply is swapped in for our
bespoke implementation and std::tuple is used to hold the args. We then
std::move the args when Run is called. We also needed to support an r-value
Class param for the runnable method on a bound object versions could work
with UniquePtr as the holder class.

Comment 3

[Eric Rahm [:erahm]]

Attachment: Bug 1322095 - Part 1b: Update callbacks to support moved args and convert nsAutoPtr usage. r=jya

In order to support the update to move args when invoking callbacks we:

• Convert anything that was using WrapRunnable with nsAutoPtr to UniquePtr
• Convert anything that was using a non-const ref as a param to either a
  const ref or a by-val copy

Addtionally we convert the remaining nsAutoPtr usage to UniquePtr.

Comment 4

[Eric Rahm [:erahm]]

Attachment: Bug 1322095 - Part 2: Remove nsAutoPtr from dom/media. r=jya

This converts nsAutoPtr usage in dom/media to UniquePtr. Beyond just a
search and replace we also needed to update assignment and access of the
UniquePtrs. This falls into a few categories:

• Assignment from a newly constructed object switches to MakeUnique
• Assignment from a raw ptr switches to UniquePtr::reset
• Handing out a raw ptr now requires UniquePtr::get
• Uses UniquePtr::release rather than nsAutoPtr::forget
• A few spots are updated to return a UniquePtr rather than a raw ptr

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/078c0fe497b7
Part 1a: Move WrapRunnable's args when running. r=jya
https://hg.mozilla.org/integration/autoland/rev/14e115ba7f12
Part 1b: Update callbacks to support moved args and convert nsAutoPtr usage. r=jya

Comment 6

[Cristina Coroiu [:ccoroiu]]

Backed out 2 changesets (bug 1322095) for build bustage at dist/include/mtransport/runnable_utils.h

Backout: https://hg.mozilla.org/integration/autoland/rev/eecf011057c3cf9aa2528ac6dd47d5faed411b1a

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=14e115ba7f12359c0c6cb2e06bfdc0e70696a83c

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=286184404&repo=autoland&lineNumber=17696

Comment 7

[Eric Rahm [:erahm]]

Static analysis is unhappy about [this], are you okay with just going back to [&]?

Comment 8

[Jean-Yves Avenard [:jya]]

(In reply to Eric Rahm [:erahm] from comment #7)

Static analysis is unhappy about [this], are you okay with just going back to [&]?

IIRC it will fail in the same fashion.
You can't capture this by value or reference if the object is refcounted.

Comment 9

[Jean-Yves Avenard [:jya]]

You could use UnsafePtr for this, https://searchfox.org/mozilla-central/source/netwerk/ipc/ChannelEventQueue.h#99

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f9b82ed6f7a
Part 1a: Move WrapRunnable's args when running. r=gerald
https://hg.mozilla.org/integration/autoland/rev/0ea358101f67
Part 1b: Update callbacks to support moved args and convert nsAutoPtr usage. r=jya
https://hg.mozilla.org/integration/autoland/rev/bb1ce9b0e5f6
Part 2: Remove nsAutoPtr from dom/media. r=jya

Comment 11

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/3f9b82ed6f7a
https://hg.mozilla.org/mozilla-central/rev/0ea358101f67
https://hg.mozilla.org/mozilla-central/rev/bb1ce9b0e5f6