Closed Bug 877261 Opened 7 years ago Closed 7 years ago

Kill XPCLazyCallContext

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(11 files)

117.37 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
57.55 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
9.97 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
15.86 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
19.20 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
2.24 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
5.22 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
11.14 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
9.21 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
6.45 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
5.18 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
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.
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.
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
Nice, that looks green. I'll keep going.
Depends on: 877478
Blocks: 869740
Focusing the bug title on the end goal we have here for rooting.
Summary: Investigate removing most of the XPCCallContext instantiations from XPConnect → Kill XPCLazyCallContext
Starting with the above, this is the smallest unit change that will compile.
Attachment #757994 - Flags: review?(Ms2ger)
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.
Attachment #757996 - Flags: review?(Ms2ger)
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.
Attachment #758002 - Flags: review?(Ms2ger)
\o/
Attachment #758003 - Flags: review?(Ms2ger)
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.
Attachment #758004 - Flags: review?(Ms2ger)
The large block is a simple move.
Attachment #758005 - Flags: review?(Ms2ger)
Depends on: 879341
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
Attachment #757994 - Flags: review?(Ms2ger) → review+
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
Attachment #757995 - Flags: review?(Ms2ger) → review+
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
Attachment #757996 - Flags: review?(Ms2ger) → review+
Attachment #757998 - Flags: review?(Ms2ger) → review+
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
Attachment #757999 - Flags: review?(Ms2ger) → review+
Attachment #758000 - Flags: review?(Ms2ger) → review+
Attachment #758001 - Flags: review?(Ms2ger) → review+
Attachment #758002 - Flags: review?(Ms2ger) → review+
Attachment #758003 - Flags: review?(Ms2ger) → review+
Attachment #758004 - Flags: review?(Ms2ger) → review+
Attachment #758005 - Flags: review?(Ms2ger) → review+
Looks fine fine, I guess.
You need to log in before you can comment on or make changes to this bug.