877261 - Kill XPCLazyCallContext

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

In a single-threaded world, the only thing we actually _need_ these things for should be for method calls on reflectors (XPCWrappedNative::CallMethod, and XPCWrappedJSClass::CallMethod). The rest of it is an artifact that all the function signatures expect one of these things, and so we have to pass it everywhere.

Because of the performance overhead, we create these XPCLazyCallContexts, which do placement-new to instantiate an XPCCallContext when they need one. But this non-LIFOness is a total nightmare for rooting.

It may be a Sisyphusian task, but I'm going to take a crack at seeing how hard it might be to just remove most of this.

Comment 1

[Gabor Krizsanits (INACTIVE)]

I'm all for it. In most places it's really only used for stupid things, like getting the run-time from it. But there are a few bits that needs to get right. I'd suggest you to start in xpconvert... the XPCWrappedNative::GetNewOrUsed should be the hardest part I guess, but I didn't have the time back then to figure it out when I wanted to remove all these nonsense XPCCallContext arguments.

Comment 2

[Bobby Holley (:bholley)]

This turned out to be more tractable than I expected. My primary strategy was just to ditch the param entirely (rather than converting ccx -> cx) and use AutoJSContexts whenever we need them.

I started in XPCWrappedNative and worked outwards. I managed to get things compiling after removing about half of the ccx instances in XPConnect, so I'm going to push this to try now to nip any regressions in the bud:

https://tbpl.mozilla.org/?tree=Try&rev=7625c5ad5472

Comment 3

[Bobby Holley (:bholley)]

Nice, that looks green. I'll keep going.

Comment 4

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=221ae25574e6

Comment 5

[Bobby Holley (:bholley)]

Focusing the bug title on the end goal we have here for rooting.

Comment 6

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=029d2fb1d251

Comment 7

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=30ab7672305a

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 1 - Stop using XPCCallContext for most stuff in XPCWrappedNative.cpp. v1

Starting with the above, this is the smallest unit change that will compile.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 2 - Stop using XPCCallContext for XPCConvert. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 3 - Stop taking a cx in XPCWrappedJS::GetNewOrUsed. v1

Some of these callers seem to be passing a ccx when they don't need to, but
let's just remove the param all together for consistency.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove XPCCallContext from XPCCComponents. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 5 - Remove a bunch of now-unnecessary ccx declarations from nsXPConnect. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 6 - Remove the last two non-quickstub XPCLazyCallContexts. v1

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 7 - Stop Passing lccx in a few Quickstubs places where we just want a JSContext. v1

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 8 - Remove XPCLazyCallContext from Quickstubs. v1

There are a number of places where quickstubs does a scary-looking call to
lccx->SetWrapper. However, the lccx never gets morphed into a ccx, nor does
it escape in any other way. And unlike ccxes, declaring an lccx on the stack
doesn't have any observable side-effects. So this should actually be safe.

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 9 - Kill XPCLazyCallContext. v1

\o/

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 10 - Remove WrapperInitOptions. v1

Now that we don't have the separate path for initialization from an
XPCLazyCallContext, this stuff can be simplified. We get rid of Init entirely
in the next patch.

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 11 - Fold XPCCallContext::Init into XPCCallContext::XPCCallContext. v1

The large block is a simple move.

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 757994 [details] [diff] [review]
Part 1 - Stop using XPCCallContext for most stuff in XPCWrappedNative.cpp. v1

Review of attachment 757994 [details] [diff] [review]:
-----------------------------------------------------------------

It looks good.

https://twitter.com/girayozil/status/306836785739210752

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +516,5 @@
>  
>          NS_ASSERTION(!xpc::WrapperFactory::IsXrayWrapper(parent),
>                       "Xray wrapper being used to parent XPCWrappedNative?");
>  
> +        ac.construct((JSContext*)cx, parent);

I'd prefer static_cast or adding a  .get().

@@ +1988,3 @@
>                  clazz) {
> +                RootedObject answer(cx,
> +                    clazz->CallQueryInterfaceOnJSObject(cx,jso, *iid));

Missing space

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 757995 [details] [diff] [review]
Part 2 - Stop using XPCCallContext for XPCConvert. v1

Review of attachment 757995 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCConvert.cpp
@@ +1391,5 @@
>  
>  #define POPULATE(_t)                                                                    \
>      PR_BEGIN_MACRO                                                                      \
>          for (i = 0; i < count; i++) {                                                   \
> +            if (!NativeData2JS(current.address(), ((_t*)*s)+i, type, iid, pErr) || \

Keep the \s aligned

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 757996 [details] [diff] [review]
Part 3 - Stop taking a cx in XPCWrappedJS::GetNewOrUsed. v1

Review of attachment 757996 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +2389,5 @@
>                  "${target} = ${source};"
>                  ).substitute(self.substitution)
>  
>          return checkObjectType + string.Template(
>              """nsresult rv;

Move the declaration down

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 757999 [details] [diff] [review]
Part 5 - Remove a bunch of now-unnecessary ccx declarations from nsXPConnect. v1

Review of attachment 757999 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/nsXPConnect.cpp
@@ +1195,5 @@
>                              nsIVariant ** aResult)
>  {
>      NS_PRECONDITION(aJSVal, "bad param");
>      NS_PRECONDITION(aResult, "bad param");
>      *aResult = nullptr;

Can drop this

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Looks fine fine, I guess.

Comment 24

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=6638e21a7443

Comment 25

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=9688ed435bf8

Comment 26

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=902172f7b451

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5a678ff4ce22
https://hg.mozilla.org/mozilla-central/rev/9ab08f6d2258
https://hg.mozilla.org/mozilla-central/rev/29351d714861
https://hg.mozilla.org/mozilla-central/rev/6e56f8dae182
https://hg.mozilla.org/mozilla-central/rev/4cb6b2983e9d
https://hg.mozilla.org/mozilla-central/rev/c7f87d78d5b1
https://hg.mozilla.org/mozilla-central/rev/16f8376c7b34
https://hg.mozilla.org/mozilla-central/rev/4bf9a018b8f2
https://hg.mozilla.org/mozilla-central/rev/b5dc59eec4c6
https://hg.mozilla.org/mozilla-central/rev/576842d446e3
https://hg.mozilla.org/mozilla-central/rev/902172f7b451