2044510 - For functions that take `already_AddRefed<T>&&`, change to `already_AddRefed<T>` (pass by value instead of by rvalue reference), to force an ownership-transfer into the function

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Daniel Holbert [:dholbert]]

We've got quite a few cases of functions that take an rvalue reference to an already_AddRefed type - this searchfox captures some (maybe all?) of them:
https://searchfox.org/firefox-main/search?q=%5C%28.*already_AddRefed.*%26%26&path=&case=false&regexp=true

Excluding the smart-pointer-internal ones (maybe?), I think none of the others should actually take an rvalue reference at all. The parameter type should just be already_AddRefed<T>, not already_AddRefed<T>&&.

By taking already_AddRefed<T>&&, we're giving too much leeway -- we're allowing the function to not-consume the passed-in value (and leave the caller's already_AddRefed untouched). And that's generally not the semantics that we're looking for. The fact that we're passing an already_AddRefed implies that we're intentionally trying to transfer ownership into the function, and passing-by-value forces that ownership-transfer to happen (by making the parameter take ownership of the reference when it's copy-constructed, and then forcing the function to take ownership from that parameter before the parameter gets destructed, by virtue of how ~already_AddRefed works).

In cases where we forget to consume the passed-in reference: if we use already_AddRefed<T> (passing by value instead of by rvalue reference), that should result in easier-to-understand error output -- the parameter will fatally assert when it goes out of scope, instead of the caller's value (a temporary or a passed-in local variable, some arbitrary distance up the stack) fatally asserting, as would happen right now.

Comment 1

[Daniel Holbert [:dholbert]]

I've got a patch that claude helped me write. Will post later on.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: WIP: Bug 2044510: Change function signatures to take already_AddRefed by-value instead of by-rvalue-reference.

Comment 3

[Daniel Holbert [:dholbert]]

Try: https://treeherder.mozilla.org/jobs?repo=try&landoInstance=lando-prod-2025&landoCommitID=53292

Comment 4

[Daniel Holbert [:dholbert]]

(Given the size of this bug's patch, maybe it's best for me to split into per-component parts, since it's reviewable/landable in a piece-wise fashion...)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)

(Given the size of this bug's patch, maybe it's best for me to split into per-component parts, since it's reviewable/landable in a piece-wise fashion...)

I started to do this in separate bugs (bug 2044569) but I realized that I should probably not land parts separately in case there are inheritance relationships between different functions. So I'm just going to split the original patch into multiple patches and post them all here as a stack to be landed atomically.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in xpcom/. r?#xpcom-reviewers

See https://bugzilla.mozilla.org/show_bug.cgi?id=2044510#c0 for more details.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in: layout/, netwerk/, parser/, security/, toolkit/, and widget/. r?#xpcom-reviewers

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in docshell/. r?#dom-core

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in dom/base/. r?#dom-core

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in dom/{bindings,filesystem,fs}. r?#dom-core

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in dom/html. r?#dom-core

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in dom/{ipc,mathml,media,notification,streams}. r?#dom-core

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in dom/svg. r?#firefox-svg-reviewers

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in dom/{workers,worklet,xml,xul}. r?#dom-core

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in image/. r?tnikkel

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in js/. r?#spidermonkey-reviewers

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in layout/. r?#layout-reviewers

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in netwerk/. r?#necko-reviewers

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in parser/. r?#dom-core

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in security/manager/ssl. r?#nss-reviewers

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in toolkit/components/antitracking/. r?timhuang

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in toolkit/components/telemetry/. r?TravisLong

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in widget/uikit/. r?#mac-reviewers

Comment 25

[Valentin Gosu [:valentin]]

Consider also writing a clang-plugin to prevent this pattern coming back in the future.

Comment 26

[Phabricator Automation]

Comment on attachment 9592909 [details]
Bug 2044510: Pass already_AddRefed types by-value instead of by rvalue reference, in image/. r?tnikkel

Revision D304366 was moved to bug 2045352. Setting attachment 9592909 [details] to obsolete.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c8723c0549ea
https://hg.mozilla.org/integration/autoland/rev/08bb32707816
Pass already_AddRefed types by-value instead of by rvalue reference, in xpcom/. r=xpcom-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/fbc22c18f3d0
https://hg.mozilla.org/integration/autoland/rev/5bb9e4d50fe1
Pass already_AddRefed types by-value instead of by rvalue reference, in docshell/. r=dom-core-reviewers,smaug
https://github.com/mozilla-firefox/firefox/commit/b9c26953f1e2
https://hg.mozilla.org/integration/autoland/rev/7b7fd5636306
Pass already_AddRefed types by-value instead of by rvalue reference, in dom/base/. r=dom-core-reviewers,smaug
https://github.com/mozilla-firefox/firefox/commit/654bbc1057ba
https://hg.mozilla.org/integration/autoland/rev/881cf0929ca2
Pass already_AddRefed types by-value instead of by rvalue reference, in dom/{bindings,filesystem,fs}. r=dom-core-reviewers,smaug
https://github.com/mozilla-firefox/firefox/commit/648f3a95c5a0
https://hg.mozilla.org/integration/autoland/rev/b7c18a980296
Pass already_AddRefed types by-value instead of by rvalue reference, in dom/html. r=dom-core-reviewers,smaug
https://github.com/mozilla-firefox/firefox/commit/3a33cc1c12ef
https://hg.mozilla.org/integration/autoland/rev/b3135760ba66
Pass already_AddRefed types by-value instead of by rvalue reference, in dom/{ipc,mathml,media,notification,streams}. r=dom-core-reviewers,media-playback-reviewers,webrtc-reviewers,smaug,pehrsons,aosmond
https://github.com/mozilla-firefox/firefox/commit/58343150e294
https://hg.mozilla.org/integration/autoland/rev/32438b1c051d
Pass already_AddRefed types by-value instead of by rvalue reference, in dom/svg. r=firefox-svg-reviewers,jwatt
https://github.com/mozilla-firefox/firefox/commit/806e3309d22e
https://hg.mozilla.org/integration/autoland/rev/73da4d62f357
Pass already_AddRefed types by-value instead of by rvalue reference, in dom/{workers,worklet,xml,xul}. r=dom-core-reviewers,smaug
https://github.com/mozilla-firefox/firefox/commit/7c3a9e9ccf60
https://hg.mozilla.org/integration/autoland/rev/e3072a946510
Pass already_AddRefed types by-value instead of by rvalue reference, in js/. r=spidermonkey-reviewers,nbp
https://github.com/mozilla-firefox/firefox/commit/3879f16d18d2
https://hg.mozilla.org/integration/autoland/rev/bc7da0b6b808
Pass already_AddRefed types by-value instead of by rvalue reference, in layout/. r=layout-reviewers,firefox-style-system-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/0136eea03d5c
https://hg.mozilla.org/integration/autoland/rev/5cb97c62a3e6
Pass already_AddRefed types by-value instead of by rvalue reference, in netwerk/. r=necko-reviewers,valentin
https://github.com/mozilla-firefox/firefox/commit/cb1c37d98ea1
https://hg.mozilla.org/integration/autoland/rev/85e8755ef3d8
Pass already_AddRefed types by-value instead of by rvalue reference, in parser/. r=dom-core-reviewers,smaug
https://github.com/mozilla-firefox/firefox/commit/a31407577a03
https://hg.mozilla.org/integration/autoland/rev/9c54ebfd2257
Pass already_AddRefed types by-value instead of by rvalue reference, in security/manager/ssl. r=nss-reviewers,keeler
https://github.com/mozilla-firefox/firefox/commit/c7937e1f141a
https://hg.mozilla.org/integration/autoland/rev/d9032c0a6075
Pass already_AddRefed types by-value instead of by rvalue reference, in toolkit/components/antitracking/.  r=timhuang
https://github.com/mozilla-firefox/firefox/commit/e0c53e05558d
https://hg.mozilla.org/integration/autoland/rev/b51ea2b34280
Pass already_AddRefed types by-value instead of by rvalue reference, in toolkit/components/telemetry/. r=TravisLong
https://github.com/mozilla-firefox/firefox/commit/2430ca31eec4
https://hg.mozilla.org/integration/autoland/rev/965658c44af0
Pass already_AddRefed types by-value instead of by rvalue reference, in widget/uikit/. r=mac-reviewers,bradwerth
https://github.com/mozilla-firefox/firefox/commit/69c63d49ae03
https://hg.mozilla.org/integration/autoland/rev/bfe5b60f478c
apply code formatting via Lando

Comment 28

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/08bb32707816
https://hg.mozilla.org/mozilla-central/rev/5bb9e4d50fe1
https://hg.mozilla.org/mozilla-central/rev/7b7fd5636306
https://hg.mozilla.org/mozilla-central/rev/881cf0929ca2
https://hg.mozilla.org/mozilla-central/rev/b7c18a980296
https://hg.mozilla.org/mozilla-central/rev/b3135760ba66
https://hg.mozilla.org/mozilla-central/rev/32438b1c051d
https://hg.mozilla.org/mozilla-central/rev/73da4d62f357
https://hg.mozilla.org/mozilla-central/rev/e3072a946510
https://hg.mozilla.org/mozilla-central/rev/bc7da0b6b808
https://hg.mozilla.org/mozilla-central/rev/5cb97c62a3e6
https://hg.mozilla.org/mozilla-central/rev/85e8755ef3d8
https://hg.mozilla.org/mozilla-central/rev/9c54ebfd2257
https://hg.mozilla.org/mozilla-central/rev/d9032c0a6075
https://hg.mozilla.org/mozilla-central/rev/b51ea2b34280
https://hg.mozilla.org/mozilla-central/rev/965658c44af0
https://hg.mozilla.org/mozilla-central/rev/bfe5b60f478c