1019191 - Get rid of the quickstubs infrastructure

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We're really close.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove the dead finishInitForWrappedGlobal method

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove WebIDL quickstubs for EventTarget, since the only remaining xpconnect EventTargets with classinfo are globals, and we never call PostCreatePrototype on globals

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove calls to the empty DOM_MarkInterfaces/DOM_ClearInterfaces/DOM_InitInterfaces functions and the functions themselves

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Remove the now-dead xpc_qsThrowGetterSetterFailed and xpc_qsThrowBadSetterValue functions

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Remove the one remaining quickstub we have and all quickstub codegen

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Remove dead code for setting up quickstubs on a prototype from XPCQuickStubs.h/cpp

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Remove various dead-code throwing functions

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Stop using xpc_qsACString in XPConnect

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Remove now-dead JS-to-C++ quickstub conversion functions

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Stop using xpc_qsUnwrapArg directly in BindingUtils

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Get rid of the now-unused xpc_qsUnwrapArg

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 12. Eliminate some unused UnwrapArg specializations

The following command:

  grep -Ir "UnwrapArg<" $srcdir/* $objdir/dom/bindings/ | sed 's/^[^<]*<//' | sed 's/>.*//' | sort -u

(with $srcdir and $objdir replaced by the relevant paths) produces this list:

${type}
_clazz
_interface
imgINotificationObserver
imgIRequest
mozilla::dom::EventTarget
mozilla::dom::IndirectlyImplementedInterface
mozilla::dom::TestExternalInterface
nsGenericHTMLElement
nsIBrowserDOMWindow
nsIChannel
nsIDOMCSSRule
nsIDOMDataChannel
nsIDOMMozMmsMessage
nsIDOMMozSmsMessage
nsIDOMMozWakeLockListener
nsIDOMWindow
nsIDOMXPathNSResolver
nsIFile
nsIFrameRequestCallback
nsIInputStream
nsIInputStreamCallback
nsIJSID
nsIMenuBuilder
nsIObserver
nsIOutputStream
nsIPrincipal
nsISelectionListener
nsISupports
nsITreeView
nsIURI

The ${type} bit is part of codegen.  The _clazz and _interface bits are the
macros in js/xpconnect/src/nsDOMQS.h that are defining UnwrapArg methods.  The
nsGenericHTMLElement is only used in xpc_qsUnwrapArg_HTMLElement.

Inspection of the above list indicates that none of the classes used with
DEFINE_UNWRAP_CAST_HTML are in it, so all those specializations of UnwrapArg,
and the xpc_qsUnwrapArg_HTMLElement method they call, are dead code.

Moreover, almost all the specializations set up by DEFINE_UNWRAP_CAST are dead
code as well; the only exception is the one for mozilla::dom::EventTarget.  But
since we no longer use Web IDL quickstubs for EventTarget, that one is only
used in a few places for method arguments, and none of those should have our
one remaining webidl event target passed to them.  So it's safe to remove that
specialization as well.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 13. Remove the now-unused castNativeArgFromWrapper and castNativeFromWrapper functions

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 14. Remove the now-unused HasBitInInterfacesBitmap and all the interfaces bitmap machinery that ends up unused as a result

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 15. Remove now-dead DOMCI_DATA bits

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 16. Remove nsDOMQS.h

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 17. Make some of the remaining quickstub unwrap methods static

Note that we know that we always call UnwrapArg with an object, so
xpc_qsUnwrapArgImpl knows v is always an object.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 18. Remove the unused tearoff bits in quickstub unwrapping

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 19. Change GlobalObject::GetAsSupports to not rely on UnwrapArg producing an nsISupports for non-reflectors

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 20. Replace the getWrapper/castNative stuff in XPCQuickStubs with what I believe is a single function call that's equivalent to it, since the tearoff bit was dead code anyway due to never resetting clasp

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 21. Change xpc_qsUnwrapArgImpl and UnwrapArg to take a HandleObject, not a HandleValue

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 22. Eliminate the effectively unused vp argument of xpc_qsUnwrapArgImpl

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: part 23. Eliminate the ppArgRef argument of xpc_qsUnwrapArgImpl and UnwrapArg, since we're now always handing out a ref in ppArg

Comment 24

[Boris Zbarsky [:bzbarsky]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=b263f8e01802 and then I fixed the X and bc3 and Mnw issue and https://tbpl.mozilla.org/?tree=Try&rev=d8dbe3dfa2a1

diffstat says: 39 files changed, 96 insertions(+), 3334 deletions(-)

Comment 25

[Bobby Holley (:bholley)]

Comment on attachment 8508044 [details] [diff] [review]
part 20.  Replace the getWrapper/castNative stuff in XPCQuickStubs with what I believe is a single function call that's equivalent to it, since the tearoff bit was dead code anyway due to never resetting clasp

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

Very happy to see this code go.

Comment 26

[Boris Zbarsky [:bzbarsky]]

One other thing.  After these patches, the only thing left in XPCQuickStubs.h is the decl of xpc_qsUnwrapArgImpl and the only things in XPCQuickStubs.cpp are its impl and the impl of NonVoidStringToJsval.

We could basically move xpc_qsUnwrapArgImpl to BindingUtils.cpp (with a better name), move NonVoidStringToJsval somewhere more useful, and kill off XPCQuickStubs.*.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Oh, and do we actually still need that XPCCallContext bit in xpc_qsUnwrapArgImpl?

Comment 28

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #27)
> Oh, and do we actually still need that XPCCallContext bit in
> xpc_qsUnwrapArgImpl?

I can't really say - it depends on whether we run any script in nsXPCWrappedJS::GetNewOrUsed that expects an XPCCallContext to be on the stack. But I would guess it's fine with 90% certainty. And anyway, if something does come up, I'd rather just fix up that consumer to not expect and XPCCallContext, since that's no longer a reasonable expectation.

Comment 29

[Boris Zbarsky [:bzbarsky]]

I'm 99% sure nsXPCWrappedJS::GetNewOrUsed does not run script at all.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Ah, I guess the QI call might run script, though.  But that puts its own XPCCallContext on the stack before doing so, in nsXPCWrappedJSClass::DelegatedQueryInterface.

Comment 31

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #30)
> Ah, I guess the QI call might run script, though.

Yes, exactly.

> But that puts its own
> XPCCallContext on the stack before doing so, in
> nsXPCWrappedJSClass::DelegatedQueryInterface.

Ok then. Again, I don't expect it to be a problem regardless, given that we're more and more likely to be entering JS without an XPCCallContext these days.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: part 24. Remove the XPCCallContext stuff from xpc_qsUnwrapArgImpl

Comment 33

[Peter Van der Beken [:peterv]]

Comment on attachment 8508038 [details] [diff] [review]
part 14.  Remove the now-unused HasBitInInterfacesBitmap and all the interfaces bitmap machinery that ends up unused as a result

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

::: js/xpconnect/src/xpcprivate.h
@@ +1648,5 @@
>  // We maintain the invariant that every JSClass for which ext.isWrappedNative
>  // is true is a contained in an instance of this struct, and can thus be cast
>  // to it.
> +//
> +// XXXbz Do we still need this struct at all?

File a bug, we probably don't.

Comment 34

[Peter Van der Beken [:peterv]]

Comment on attachment 8508039 [details] [diff] [review]
part 15.  Remove now-dead DOMCI_DATA bits

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

::: dom/xslt/xpath/nsXPathNSResolver.cpp
@@ +8,1 @@
>  #include "nsDOMClassInfoID.h"

Undo this.

Comment 35

[Peter Van der Beken [:peterv]]

Comment on attachment 8508040 [details] [diff] [review]
part 16.  Remove nsDOMQS.h

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

::: js/xpconnect/src/nsDOMQS.h
@@ -37,5 @@
> -    return static_cast<nsINodeList*>(p);
> -}
> -
> -inline nsISupports*
> -ToCanonicalSupports(nsContentList *p)

Do you know if we still use this? Note that we usually fall back to QIing if this isn't provided, so just because it compiles doesn't mean we don't use it.

Comment 36

[Peter Van der Beken [:peterv]]

Comment on attachment 8508042 [details] [diff] [review]
part 18.  Remove the unused tearoff bits in quickstub unwrapping

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

::: js/xpconnect/src/XPCQuickStubs.cpp
@@ +94,3 @@
>          obj = js::GetObjectParent(obj);
> +        // XXXbz don't we need to reget clasp here?  Otherwise, we
> +        // won't set *wrapper right, will we?

Yeah. We probably almost never hit this.

Comment 37

[Peter Van der Beken [:peterv]]

Comment on attachment 8508044 [details] [diff] [review]
part 20.  Replace the getWrapper/castNative stuff in XPCQuickStubs with what I believe is a single function call that's equivalent to it, since the tearoff bit was dead code anyway due to never resetting clasp

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

::: js/xpconnect/src/XPCQuickStubs.cpp
@@ +31,5 @@
>      RootedObject src(cx, &v.toObject());
>  
> +    nsISupports *iface = xpc::UnwrapReflectorToISupports(src);
> +    if (iface) {
> +        if (NS_FAILED(iface->QueryInterface(iid, ppArg))) {

If we mostly get here with iid == nsISupports you might want to check that and avoid the QI.

Comment 38

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] from comment #26)
> One other thing.  After these patches, the only thing left in
> XPCQuickStubs.h is the decl of xpc_qsUnwrapArgImpl and the only things in
> XPCQuickStubs.cpp are its impl and the impl of NonVoidStringToJsval.
> 
> We could basically move xpc_qsUnwrapArgImpl to BindingUtils.cpp (with a
> better name), move NonVoidStringToJsval somewhere more useful, and kill off
> XPCQuickStubs.*.

Sounds good to me.

Comment 39

[Boris Zbarsky [:bzbarsky]]

> File a bug, we probably don't.

Bug 1087354 filed.

> Undo this.

Oops, yes.

> Do you know if we still use this? Note that we usually fall back to QIing

Ugh, I forgot about that.  So doing this the the hard way....

ToCanonicalSupports is called only from the qsObjectHelper constructor.  After this patch series this is only used in HandleNewBindingWrappingFailure, dom::WrapObject, WrapNativeISupportsParent.  And of these WrapNativeISupportsParent calls ToSupports() manually, so doesn't rely on ToCanonicalSupports at all.  HandleNewBindingWrappingFailure should never get used with nsContentList.  WrapObject could conceivably be used, though I expect it's not for nsContentList, and if they do they'll get slow xpconnect wrapping anyway.  Per irc discussion, let's drop the ToCanonicalSupports.

ToSupports is used all over; but peterv just pointed out on IRC that the template one in nsCycleCollectionNoteChild.h works for nsContentList.

> If we mostly get here with iid == nsISupports you might want to check that and avoid the
> QI.

We get here with iid == nsISupports for QI basically.  Or anything else that has an nsISupports arg in the IDL; in practice that's BoxObject, PeerConnectionImpl, and SpeechRecognitionEvent.

In terms of number of callsites, the most common thing here is nsIDOMWindow, but I just filed bug 1087378 on reducing that.  The next most common is imgINotificationObserver and then mozilla::dom::EventTarget.  This last will go away once we drop hasXPConnectImpls.

In terms of number of runtime calls, I expect that these are all pretty rare, right?

Comment 40

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/871381d9ec4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fca92cc7229
https://hg.mozilla.org/integration/mozilla-inbound/rev/915c98bbdbaa
https://hg.mozilla.org/integration/mozilla-inbound/rev/634e4ac7bc3b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c74d9fc161ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc58260747f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/307ce2aad881
https://hg.mozilla.org/integration/mozilla-inbound/rev/13192ac3910b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d518efb791ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4df8d92d4f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/649e33dfabd6
https://hg.mozilla.org/integration/mozilla-inbound/rev/28abe750b8ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/63d3d488d188
https://hg.mozilla.org/integration/mozilla-inbound/rev/593ede2c0038
https://hg.mozilla.org/integration/mozilla-inbound/rev/933bbcafc8e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/99b3f0919b5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b146182cda9
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1c5a9cfd0c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/7535aa66fec1
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb6ab1525b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/660bf8b6a9ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc4056729207
https://hg.mozilla.org/integration/mozilla-inbound/rev/02e63032ce53
https://hg.mozilla.org/integration/mozilla-inbound/rev/59be1e005220

Comment 41

[Boris Zbarsky [:bzbarsky]]

> Sounds good to me.

Bug 1087404 filed.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/871381d9ec4d
https://hg.mozilla.org/mozilla-central/rev/3fca92cc7229
https://hg.mozilla.org/mozilla-central/rev/915c98bbdbaa
https://hg.mozilla.org/mozilla-central/rev/634e4ac7bc3b
https://hg.mozilla.org/mozilla-central/rev/c74d9fc161ca
https://hg.mozilla.org/mozilla-central/rev/dc58260747f9
https://hg.mozilla.org/mozilla-central/rev/307ce2aad881
https://hg.mozilla.org/mozilla-central/rev/13192ac3910b
https://hg.mozilla.org/mozilla-central/rev/d518efb791ac
https://hg.mozilla.org/mozilla-central/rev/e4df8d92d4f3
https://hg.mozilla.org/mozilla-central/rev/649e33dfabd6
https://hg.mozilla.org/mozilla-central/rev/28abe750b8ef
https://hg.mozilla.org/mozilla-central/rev/63d3d488d188
https://hg.mozilla.org/mozilla-central/rev/593ede2c0038
https://hg.mozilla.org/mozilla-central/rev/933bbcafc8e6
https://hg.mozilla.org/mozilla-central/rev/99b3f0919b5d
https://hg.mozilla.org/mozilla-central/rev/5b146182cda9
https://hg.mozilla.org/mozilla-central/rev/e1c5a9cfd0c7
https://hg.mozilla.org/mozilla-central/rev/7535aa66fec1
https://hg.mozilla.org/mozilla-central/rev/7cb6ab1525b2
https://hg.mozilla.org/mozilla-central/rev/660bf8b6a9ad
https://hg.mozilla.org/mozilla-central/rev/cc4056729207
https://hg.mozilla.org/mozilla-central/rev/02e63032ce53
https://hg.mozilla.org/mozilla-central/rev/59be1e005220