Closed Bug 1019191 Opened 6 years ago Closed 5 years ago

Get rid of the quickstubs infrastructure

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(24 files)

3.18 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1008 bytes, patch
peterv
: review+
Details | Diff | Splinter Review
5.10 KB, patch
peterv
: review+
Details | Diff | Splinter Review
6.62 KB, patch
peterv
: review+
Details | Diff | Splinter Review
56.97 KB, patch
peterv
: review+
Details | Diff | Splinter Review
7.48 KB, patch
peterv
: review+
Details | Diff | Splinter Review
7.87 KB, patch
peterv
: review+
Details | Diff | Splinter Review
2.10 KB, patch
bholley
: review+
Details | Diff | Splinter Review
21.72 KB, patch
peterv
: review+
Details | Diff | Splinter Review
3.39 KB, patch
peterv
: review+
Details | Diff | Splinter Review
13.75 KB, patch
peterv
: review+
Details | Diff | Splinter Review
10.74 KB, patch
peterv
: review+
Details | Diff | Splinter Review
4.26 KB, patch
peterv
: review+
Details | Diff | Splinter Review
22.01 KB, patch
peterv
: review+
Details | Diff | Splinter Review
20.85 KB, patch
peterv
: review+
Details | Diff | Splinter Review
5.79 KB, patch
peterv
: review+
Details | Diff | Splinter Review
4.09 KB, patch
peterv
: review+
Details | Diff | Splinter Review
3.59 KB, patch
peterv
: review+
Details | Diff | Splinter Review
4.29 KB, patch
peterv
: review+
Details | Diff | Splinter Review
6.23 KB, patch
peterv
: review+
bholley
: review+
Details | Diff | Splinter Review
8.93 KB, patch
peterv
: review+
Details | Diff | Splinter Review
9.07 KB, patch
peterv
: review+
Details | Diff | Splinter Review
10.12 KB, patch
peterv
: review+
Details | Diff | Splinter Review
1.19 KB, patch
peterv
: review+
Details | Diff | Splinter Review
We're really close.
Depends on: 1019194
Depends on: 1020715
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.
Attachment #8508036 - Flags: review?(peterv)
Attachment #8508040 - Flags: review?(peterv)
Note that we know that we always call UnwrapArg with an object, so
xpc_qsUnwrapArgImpl knows v is always an object.
Attachment #8508041 - Flags: review?(peterv)
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(-)
Attachment #8508032 - Flags: review?(bobbyholley) → review+
Attachment #8508025 - Flags: review?(bobbyholley) → review+
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.
Attachment #8508044 - Flags: review?(bobbyholley) → review+
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.*.
Flags: needinfo?(peterv)
Oh, and do we actually still need that XPCCallContext bit in xpc_qsUnwrapArgImpl?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
I'm 99% sure nsXPCWrappedJS::GetNewOrUsed does not run script at all.
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.
(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.
Attachment #8508026 - Flags: review?(peterv) → review+
Attachment #8508027 - Flags: review?(peterv) → review+
Attachment #8508028 - Flags: review?(peterv) → review+
Attachment #8508029 - Flags: review?(peterv) → review+
Attachment #8508030 - Flags: review?(peterv) → review+
Attachment #8508031 - Flags: review?(peterv) → review+
Attachment #8508033 - Flags: review?(peterv) → review+
Attachment #8508034 - Flags: review?(peterv) → review+
Attachment #8508035 - Flags: review?(peterv) → review+
Attachment #8508036 - Flags: review?(peterv) → review+
Attachment #8508037 - Flags: review?(peterv) → review+
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.
Attachment #8508038 - Flags: review?(peterv) → review+
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.
Attachment #8508039 - Flags: review?(peterv) → review+
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.
Attachment #8508040 - Flags: review?(peterv) → review+
Attachment #8508041 - Flags: review?(peterv) → review+
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.
Attachment #8508042 - Flags: review?(peterv) → review+
Attachment #8508043 - Flags: review?(peterv) → review+
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.
Attachment #8508044 - Flags: review?(peterv) → review+
Attachment #8508045 - Flags: review?(peterv) → review+
Attachment #8508046 - Flags: review?(peterv) → review+
Attachment #8508047 - Flags: review?(peterv) → review+
Attachment #8508947 - Flags: review?(peterv) → review+
(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.
Flags: needinfo?(peterv)
> 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?
Blocks: 1087354
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
Target Milestone: --- → mozilla36
> Sounds good to me.

Bug 1087404 filed.
Blocks: 1087444
Depends on: 1088231
Depends on: 1088641
Blocks: 711638
Blocks: 707097
Blocks: 618049
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.