1983207 - Unnecessary fields in CallSetup

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

CallSetup class has some maybe-rooted fields for GC things, but they're not actually used anywhere outside of the constructor, or never read.
These fields should be removed.

https://searchfox.org/mozilla-central/rev/ce0d41b6033e2a104904327a43edf730245f5241/dom/bindings/CallbackObject.h#70,240,284-289

class CallbackObjectBase {
...
  class MOZ_STACK_CLASS CallSetup {
...
    Maybe<JS::Rooted<JSObject*>> mRootedCallable;
    // The global of mRootedCallable.
    Maybe<JS::Rooted<JSObject*>> mRootedCallableGlobal;

    // Members which are used to set the async stack.
    Maybe<JS::Rooted<JSObject*>> mAsyncStack;

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Thank you for opening this bug: I've been looking at these members as Chesterton's Fence but have been unable thus far to particularly make heads or tails as to why they were added -- though I've not done a historical deep dive to look into the state of CallSetup when they were added.

Comment 2

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

(mRootedCallableGlobal was added by bug 1477923 and it was also only being used in the constructor.)

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1983207 - Make parameter to AutoSetAsyncStackForNewCalls a bare object pointer r?arai

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1983207 - Remove un-needed members of CallSetup r?smaug

1. We don't need to root the stack -- fixed AutoSetAsyncStack in previous patch
2. We don't need to root the callable global -- it will be kept alive by the callable being rooted.

We -do- however still need to have the callable itself be rooted

(For example /test262/built-ins/Proxy/preventExtensions/trap-is-undefined-target-is-proxy.js crashes
if it's not rooted, as we end up trying to AutoRealm into a CCW.)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I think https://searchfox.org/firefox-main/rev/16f979184458f531fce47706d2fa7f2902e75ad3/dom/bindings/CallbackObject.h#223-227 was the reason, but trying to figure out if that is needed for CallSetup too.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/1c3ac3fe1e10
https://hg.mozilla.org/integration/autoland/rev/916d56392533
Make parameter to AutoSetAsyncStackForNewCalls a bare object pointer r=arai
https://github.com/mozilla-firefox/firefox/commit/b24ac9bf7210
https://hg.mozilla.org/integration/autoland/rev/53fe2827b95a
Remove un-needed members of CallSetup r=smaug

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/916d56392533
https://hg.mozilla.org/mozilla-central/rev/53fe2827b95a