Closed Bug 996061 Opened 10 years ago Closed 10 years ago

Stop generating xpt's for non-[scriptable] interfaces

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32

People

(Reporter: ehsan.akhgari, Assigned: froydnj)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(5 files, 4 obsolete files)

part 0 - make tests use [scriptable] interfaces
(deleted), patch
ted
: review+
Details | Diff | Splinter Review
part 1 - add GetEntryNameAt method for typelibs
(deleted), patch
bholley
: review+
Details | Diff | Splinter Review
part 2 - split out a method for getting an interface's typelib index
(deleted), patch
bholley
: review+
Details | Diff | Splinter Review
part 3 - try to shim param information in xptiInterfaceEntry when possible
(deleted), patch
bholley
: review+
Details | Diff | Splinter Review
part 4 - throw away unscriptable interfaces that aren't otherwise needed
(deleted), patch
ted
: review+
Details | Diff | Splinter Review
Attached patch Naive patch (obsolete) (deleted) — — Splinter Review 
The attached patch does what we want here I think, but it breaks everything!  Perhaps that's not precisely what we want to do?  But at any rate, the errors look like this:

JavaScript error: chrome://browser/content/tabbrowser.xml, line 49: NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO: Cannot find interface information for parameter arg 0 [nsIDOMWindow.document]

presumably coming from <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#53>.  (Note that you probably want to apply my patch in bug 994964 which would help make things worse by making many DOM interfaces non-[scriptable]).

Nathan offered help driving this forward.  Much appreciated!
No longer blocks: 994964
Depends on: 994964
Depends on: 996718
This patch seems to do what we want; it implements mark-and-sweep garbage
collection of interfaces at xpt link time.  The changes, however, are not
stunning:

[froydnj@cerebro build-debug]$ f 5 before-xpidl-changes |addup
438664
[froydnj@cerebro build-debug]$ f 5 after-xpidl-changes |addup
426977

The *-xpidl-changes files are ls -l of config/makefiles/xpidl/xpt/*.xpt.

So about ~12K of savings.  If those files get linked together in new and
exciting ways in other places, it's possible we might save a little more.  I
guess we've done more for less space savings elsewhere, so maybe this is worth
it on its own.

Light local testing (i.e. starting the browser) indicates this doesn't break
anything.  Since khuey is leaving soon, I'm going to give this to Ted to
review.
Attachment #8406238 - Attachment is obsolete: true
Attachment #8407037 - Flags: review?(ted)
Hrm.

So it seems like there are two separate things we might want here:

1)  Throw away completely xpt info for non-scriptable interfaces that are not used as an argument or return value for any scriptable interface.  That's what the attached patch implements, kinda, right?  Or does it implement the weaker "not used as an argument or return value for any interface that we have xpt info for"?

2)  Throw away all the xpt data about the methods/properties of non-scriptable interfaces.  This is more likely to be effective for the "mark the DOM interfaces noscript" thing Ehsan is doing, since I expect some DOM interface is reachable from some scriptable non-DOM interface via return values or args, and then we pull in the whole DOM interface graph.
(In reply to Boris Zbarsky [:bz] from comment #2)
> 1)  Throw away completely xpt info for non-scriptable interfaces that are
> not used as an argument or return value for any scriptable interface. 
> That's what the attached patch implements, kinda, right?  Or does it
> implement the weaker "not used as an argument or return value for any
> interface that we have xpt info for"?

It implements the stronger version.

> 2)  Throw away all the xpt data about the methods/properties of
> non-scriptable interfaces.  This is more likely to be effective for the
> "mark the DOM interfaces noscript" thing Ehsan is doing, since I expect some
> DOM interface is reachable from some scriptable non-DOM interface via return
> values or args, and then we pull in the whole DOM interface graph.

This is intriguing.  I can try this too.
We wouldn't expect to see much of a change here until we start marking most of the DOM as non-scriptable, right?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (Away 4/19-5/7) from comment #4)
> We wouldn't expect to see much of a change here until we start marking most
> of the DOM as non-scriptable, right?

Yeah, which will probably limit gauging the effectiveness of bz's change #2.
You can apply my patch now for testing, it mostly works!
Also note that in bug 994964 I chose to not make nsIDOMNode, nsIDOMElement and nsIDOMEvent non-scriptable, since there are existing non-WebIDL inherited interfaces.
reftests break in very weird ways with this patch:

 0:02.24 ++DOMWINDOW == 8 (0x2af2ed0cfc00) [pid = 17062] [serial = 8] [outer = (nil)]
 0:02.31 ++DOMWINDOW == 9 (0x2af2ed2e6800) [pid = 17062] [serial = 9] [outer = 0x2af2ed0cfc00]
 0:02.36 ++DOMWINDOW == 10 (0x2af2ed391400) [pid = 17062] [serial = 10] [outer = 0x2af2ed0cfc00]
 0:02.40 REFTEST TEST-UNEXPECTED-FAIL | | EXCEPTION: [Exception... "Component returned failure code: 0x80570019 (NS_ERROR_XPC_CANT_CREATE_WN) [nsIJSCID.getService]"  nsresult: "0x80570019 (NS_ERROR_XPC_CANT_CREATE_WN)"  location: "JS frame :: chrome://reftest/content/reftest.js :: ReadManifest :: line 734"  data: no]
 0:02.40 REFTEST FINISHED: Slowest test took 0ms (undefined)
 0:02.40 REFTEST INFO | Result summary:
 0:02.40 REFTEST INFO | Successful: 0 (0 pass, 0 load only)
 0:02.40 REFTEST INFO | Unexpected: 1 (0 unexpected fail, 0 unexpected pass, 0 unexpected asserts, 0 unexpected fixed asserts, 0 failed load, 1 exception)
 0:02.40 REFTEST INFO | Known problems: 0 (0 known fail, 0 known asserts, 0 random, 0 skipped, 0 slow)
 0:02.41 REFTEST INFO | Total canvas count = 0

I guess that's related to some of the changes Ehsan still needs to make in bug 994964?
That could very well be related to the QueryInterface() stuff we're talking about on that bug.  Bug 994964 comment 31 will fix it.  Not sure I will have enough time to work on it before the weekend though :/
Whiteboard: [MemShrink:P2]
(In reply to comment #7)
> Also note that in bug 994964 I chose to not make nsIDOMNode, nsIDOMElement and
> nsIDOMEvent non-scriptable, since there are existing non-WebIDL inherited
> interfaces.

s/nsIDOMEvent/nsIDOMEventTarget/
One question.  Local builds of developers do not link their xpts.  Doing this GC during the link phase is going to mean that the contents of the xpt files are going to be different in local builds compared to packaged ones.  Are we fine with that?
That's already the case when developers forget to add xpts to packaging manifests, right?

It's a bit annoying in the sense that stuff works locally but fails when pushed, but that should not be a big deal in the cases here, since by assumption we're dropping precisely the xpts that we don't need, right?
(In reply to Boris Zbarsky [:bz] from comment #12)
> That's already the case when developers forget to add xpts to packaging
> manifests, right?

Yes.

> It's a bit annoying in the sense that stuff works locally but fails when
> pushed, but that should not be a big deal in the cases here, since by
> assumption we're dropping precisely the xpts that we don't need, right?

Yeah.  Just wanted to make sure that we're doing this consciously.  :-)
OK, here's a better patch with a worklist approach so we correctly pick up
chains of parents of required interfaces.

The |secretly_required_interfaces| bit is needed so we can grab
nsIScriptSecurityManager from script; otherwise, we don't have interface
information for its superclass and reftests (and probably other things) fall
over.  (I think that's what happens, anyway; I am not an xpconnect expert!)

It's not a reasonable long-term solution.  The right thing to do here is
probably to implement what bz suggests in comment 2 about simply discarding all
information about methods (and constants?) on non-scriptable interfaces.
Comments on that welcome.

Light testing shows that we really do need some DOM interfaces to be marked
[scriptable]; I'm sure full testing will probably point up others.  That patch
coming up.
Attachment #8407037 - Attachment is obsolete: true
Attachment #8407037 - Flags: review?(ted)
The mochitests I tried wanted nsIDOMMediaList to be scriptable.  Those
mochitests (and reftests, for that matter) wanted nsIDOMHTMLCanvasElement to be
scriptable so nsIDOMWindowUtils.compareCanvases would work.  (reftests also
want to access nsIDOMHTMLCanvasElement.toDataURL(), so I don't think we could
just delete nsIDOMHTMLCanvasElement's methods and whatnot.)

I would not be surprised if there were others.
> The mochitests I tried wanted nsIDOMMediaList to be scriptable.

Which ones?  Is this just because we return it from CSS rules?  We could change those to return nsISupports, I expect.

> nsIDOMHTMLCanvasElement to be scriptable so nsIDOMWindowUtils.compareCanvases would work.

Huh.  How is that working right now, with it not marked scriptable???

In any case, we should add comments to nsIDOMHTMLCanvasElement and nsIDOMWindowUtils pointing out the connection, so if/when we move windowutils to webidl we can drop the scriptability here.

> reftests also want to access nsIDOMHTMLCanvasElement.toDataURL()

In what case?  That should be happening via WebIDL bindings, I'd think.
FWIW, the xpt size situation improved once we had all those DOM interfaces marked non-scriptable.  Before this patch:

[froydnj@cerebro build-mc]$ ls -l config/makefiles/xpidl/xpt/*.xpt |f 5|addup
436985

After:

[froydnj@cerebro build-mc]$ ls -l config/makefiles/xpidl/xpt/*.xpt |f 5|addup
382399

70K is nothing to sneeze at!
(In reply to Boris Zbarsky [:bz] from comment #16)
> > The mochitests I tried wanted nsIDOMMediaList to be scriptable.
> 
> Which ones?  Is this just because we return it from CSS rules?  We could
> change those to return nsISupports, I expect.

This was the one I saw:

http://mxr.mozilla.org/mozilla-central/source/layout/style/test/test_condition_text.html#77

> > nsIDOMHTMLCanvasElement to be scriptable so nsIDOMWindowUtils.compareCanvases would work.
> 
> Huh.  How is that working right now, with it not marked scriptable???

I plead ignorance on that one.  Missing an IsScriptable() check somewhere?

> In any case, we should add comments to nsIDOMHTMLCanvasElement and
> nsIDOMWindowUtils pointing out the connection, so if/when we move
> windowutils to webidl we can drop the scriptability here.

Ah, yeah, that'd be good.

> > reftests also want to access nsIDOMHTMLCanvasElement.toDataURL()
> 
> In what case?  That should be happening via WebIDL bindings, I'd think.

I just assumed toDataURL wouldn't go through the WebIDL bindings.  That might have been a poor assumption.
> This was the one I saw:

Yeah, just making nsIDOMCSSImportRule and nsIDOMCSSMediaRule return nsISupports from .media should make that better.

> I plead ignorance on that one.

Yeah, it was a semi-rhetorical question.

> I just assumed toDataURL wouldn't go through the WebIDL bindings.

Everything on WebIDL objects goes through WebIDL bindings.  ;)  In any case, http://hg.mozilla.org/mozilla-central/file/420f4c65a67f/dom/webidl/HTMLCanvasElement.webidl#l27
(In reply to Boris Zbarsky [:bz] from comment #19)
> > This was the one I saw:
> 
> Yeah, just making nsIDOMCSSImportRule and nsIDOMCSSMediaRule return
> nsISupports from .media should make that better.

How does that work?  We're smart enough to just re-route things through the WebIDL bindings in that case?
(In reply to Nathan Froyd (:froydnj) from comment #14)
> Created attachment 8420161 [details] [diff] [review]
> throw away unscriptable interfaces that aren't otherwise needed
> 
> OK, here's a better patch with a worklist approach so we correctly pick up
> chains of parents of required interfaces.
> 
> The |secretly_required_interfaces| bit is needed so we can grab
> nsIScriptSecurityManager from script; otherwise, we don't have interface
> information for its superclass and reftests (and probably other things) fall
> over.  (I think that's what happens, anyway; I am not an xpconnect expert!)

Can we just get rid of nsIXPCSecurityManager and merge everything into nsIScriptSecurityManager? That's always bugged me.
> How does that work?

XPCConvert::NativeInterface2JSObject does a QI to nsWrapperCache, and if that succeeds and there is no cached object and cache->IsDOMBinding() calls WrapObject() on it.  That's a virtual method that goes and creates the WebIDL binding object.
(In reply to Bobby Holley (:bholley) from comment #21)
> (In reply to Nathan Froyd (:froydnj) from comment #14)
> > The |secretly_required_interfaces| bit is needed so we can grab
> > nsIScriptSecurityManager from script; otherwise, we don't have interface
> > information for its superclass and reftests (and probably other things) fall
> > over.  (I think that's what happens, anyway; I am not an xpconnect expert!)
> 
> Can we just get rid of nsIXPCSecurityManager and merge everything into
> nsIScriptSecurityManager? That's always bugged me.

That's an option, sure.

The real problem is a little more generic: we have interface I1 in x1.xpt that is scriptable and inherits from some non-scriptable interface I2.  I2 is defined in x2.xpt, but nothing in x2.xpt uses it, so we throw it away.  Then at runtime, I1 doesn't have a complete inheritance chain, and things go off the rails.

Why we separate everything up like this, I do not understand.  Seems like it'd be more efficient to merge all the xpts into one big blob.  Or do we do this merging when we package the app?  If so, any reason that we couldn't move that merging to build time all the time?
Modifying the patch to *additionally* scrub the methods for non-scriptable interfaces results in:

[froydnj@cerebro build-mc]$ ls -l config/makefiles/xpidl/xpt/*.xpt |f 5|addup
373359

Wins another ~9K or so.

...but the browser crashes shortly after startup, which is sad. :(
(In reply to Nathan Froyd (:froydnj) from comment #23)
> (In reply to Bobby Holley (:bholley) from comment #21)
> > (In reply to Nathan Froyd (:froydnj) from comment #14)
> > > The |secretly_required_interfaces| bit is needed so we can grab
> > > nsIScriptSecurityManager from script; otherwise, we don't have interface
> > > information for its superclass and reftests (and probably other things) fall
> > > over.  (I think that's what happens, anyway; I am not an xpconnect expert!)
> > 
> > Can we just get rid of nsIXPCSecurityManager and merge everything into
> > nsIScriptSecurityManager? That's always bugged me.
> 
> That's an option, sure.
> 
> The real problem is a little more generic: we have interface I1 in x1.xpt
> that is scriptable and inherits from some non-scriptable interface I2.  I2
> is defined in x2.xpt, but nothing in x2.xpt uses it, so we throw it away. 
> Then at runtime, I1 doesn't have a complete inheritance chain, and things go
> off the rails.

But nsIScriptSecurityManager is the only thing that actually does this, right? So let's just fix this and then forbid this practice in the XPIDL compiler.
(In reply to comment #25)
> (In reply to Nathan Froyd (:froydnj) from comment #23)
> > (In reply to Bobby Holley (:bholley) from comment #21)
> > > (In reply to Nathan Froyd (:froydnj) from comment #14)
> > > > The |secretly_required_interfaces| bit is needed so we can grab
> > > > nsIScriptSecurityManager from script; otherwise, we don't have interface
> > > > information for its superclass and reftests (and probably other things) fall
> > > > over.  (I think that's what happens, anyway; I am not an xpconnect expert!)
> > > 
> > > Can we just get rid of nsIXPCSecurityManager and merge everything into
> > > nsIScriptSecurityManager? That's always bugged me.
> > 
> > That's an option, sure.
> > 
> > The real problem is a little more generic: we have interface I1 in x1.xpt
> > that is scriptable and inherits from some non-scriptable interface I2.  I2
> > is defined in x2.xpt, but nothing in x2.xpt uses it, so we throw it away. 
> > Then at runtime, I1 doesn't have a complete inheritance chain, and things go
> > off the rails.
> 
> But nsIScriptSecurityManager is the only thing that actually does this, right?
> So let's just fix this and then forbid this practice in the XPIDL compiler.

Should be easy, since we already warn about that.
(In reply to Bobby Holley (:bholley) from comment #25)
> (In reply to Nathan Froyd (:froydnj) from comment #23)
> > The real problem is a little more generic: we have interface I1 in x1.xpt
> > that is scriptable and inherits from some non-scriptable interface I2.  I2
> > is defined in x2.xpt, but nothing in x2.xpt uses it, so we throw it away. 
> > Then at runtime, I1 doesn't have a complete inheritance chain, and things go
> > off the rails.
> 
> But nsIScriptSecurityManager is the only thing that actually does this,
> right? So let's just fix this and then forbid this practice in the XPIDL
> compiler.

Well, it's the only thing I had found.  A try run suggests there's similar things lurking:

https://tbpl.mozilla.org/?tree=Try&rev=33c13ef5319b

I think some of these might just be non-scriptable interfaces that have no uses, but XPConnect requires them to be present (?  nsIInterfaceInfo suggests itself).  Some of them might be nsIXPCSecurityManager-like.  I haven't tried very hard to track down all the problems.

The build bustages are just because we have tests that write out non-scriptable interfaces and then expect them to be there when we read them back in.  Wonderful.
(In reply to Nathan Froyd (:froydnj) from comment #27)
> Well, it's the only thing I had found.  A try run suggests there's similar
> things lurking:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=33c13ef5319b
> 
> I think some of these might just be non-scriptable interfaces that have no
> uses, but XPConnect requires them to be present (?  nsIInterfaceInfo
> suggests itself).  Some of them might be nsIXPCSecurityManager-like.  I
> haven't tried very hard to track down all the problems.

I started looking at these a little bit, just the obvious ones where we have NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO.  Some things look daunting, like:

uncaught exception - NS_ERROR_XPC_CANT_CREATE_WN: Component returned failure code: 0x80570019 (NS_ERROR_XPC_CANT_CREATE_WN) [nsIJSCID.createInstance]

Anyway, I found myself having to re-mark several DOM interfaces as scriptable.  This seems undesirable, as eventually we'll probably wind up dragging in a bunch of DOM interfaces that we didn't want anyway.  comment 16 suggests a way forward for other DOM interfaces, but I don't know what to do about these, because they're not obviously the same thing, being passed as parameters and returned as values.

	Modified   dom/interfaces/events/nsIDOMDragEvent.idl
	Modified   dom/interfaces/events/nsIDOMKeyEvent.idl
	Modified   dom/interfaces/offline/nsIDOMOfflineResourceList.idl
	Modified   dom/interfaces/range/nsIDOMRange.idl

bz, what's the general sort of fix for this problem?  Do we just make these things take/return nsISupports and throw if we don't get the right kind of thing?  Or something else
Flags: needinfo?(bzbarsky)
And if we have these problems, are addons likely to have similar problems with less recourse to solve them?
> uncaught exception - NS_ERROR_XPC_CANT_CREATE_WN: Component returned failure code:
> 0x80570019 (NS_ERROR_XPC_CANT_CREATE_WN) [nsIJSCID.createInstance]

Which interface and contract was this for, do you know?

> being passed as parameters and returned as values

Neither passing as a parameter nor returning as value should need xpt info about the methods... and for the case of passing as parameter I would argue it shouldn't really need to depend on scriptability, right?

Returning nsISupports should be fine for things in this list, since they all have classinfo.
Flags: needinfo?(bzbarsky)
Currently paramters and return values do require interfaceinfo in xpconnect. There is a very old timeless-filed bug about this, which perhaps we can fix.
> Currently paramters and return values do require interfaceinfo in xpconnect.

Right, but they shouldn't care if the interfaceinfo thinks the interface is empty.
Or is the point that we have no interfaceinfo for non-scriptable interfaces?
(In reply to Boris Zbarsky [:bz] from comment #33)
> Or is the point that we have no interfaceinfo for non-scriptable interfaces?

This.  Because in the dom-related xpt files, the nsIDOM* interfaces weren't used by scriptable interfaces, and weren't scriptable themselves, so they got deleted.  But in some other interfaces in other xpt files that merely had nsIDOM* for parameters or return values, placeholder interfaces were kept around.  Then, at runtime, resolving those placeholders failed, leading to runtime errors when the relevant methods of those interfaces were called.

i.e. this is the same thing as comment 23.  I suppose we could try linking one giant xpt to get around this, and see if that solves anything...
Shouldn't we just add those interfaces to ehsan's list of shims? That's exactly what they're for, no?
(In reply to Bobby Holley (:bholley) from comment #35)
> Shouldn't we just add those interfaces to ehsan's list of shims? That's
> exactly what they're for, no?

At least the small list of interfaces I modified in comment 28 are already present in the list of shims.
(In reply to Nathan Froyd (:froydnj) from comment #36)
> (In reply to Bobby Holley (:bholley) from comment #35)
> > Shouldn't we just add those interfaces to ehsan's list of shims? That's
> > exactly what they're for, no?
> 
> At least the small list of interfaces I modified in comment 28 are already
> present in the list of shims.

Then there should be no need to do anything at all with XPTs.
(In reply to Bobby Holley (:bholley) from comment #37)
> (In reply to Nathan Froyd (:froydnj) from comment #36)
> > (In reply to Bobby Holley (:bholley) from comment #35)
> > > Shouldn't we just add those interfaces to ehsan's list of shims? That's
> > > exactly what they're for, no?
> > 
> > At least the small list of interfaces I modified in comment 28 are already
> > present in the list of shims.
> 
> Then there should be no need to do anything at all with XPTs.

I am confused as to what you are implying should be done to fix all the errors in the testsuite, then.
(In reply to Nathan Froyd (:froydnj) from comment #38)
> I am confused as to what you are implying should be done to fix all the
> errors in the testsuite, then.

I am suggesting that comment 33 and comment 34 are incorrect, because we _do_ have interface info (via the shim). The answer is probably that we need invoke ShimInterfaceInfo::MaybeConstruct in a couple of other places in XPConnect.

Note that this is a perfect example of why the strategy I insisted on in bug 994964 is the right way to go. It scales to solve problems like this, whereas nsJSID prototype chain hackery does not. ;-)
(In reply to Bobby Holley (:bholley) from comment #39)
> (In reply to Nathan Froyd (:froydnj) from comment #38)
> > I am confused as to what you are implying should be done to fix all the
> > errors in the testsuite, then.
> 
> I am suggesting that comment 33 and comment 34 are incorrect, because we
> _do_ have interface info (via the shim). The answer is probably that we need
> invoke ShimInterfaceInfo::MaybeConstruct in a couple of other places in
> XPConnect.

I looked into this a little, trying to figure out where MaybeConstruct could be called.  All the places where NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO is thrown:

http://mxr.mozilla.org/mozilla-central/search?string=XPC_CANT_GET_PARAM_IFACE

are because we've just failed a call for GetIIDForParamNoAlloc.  And that function is deliberately unimplemented in ShimInterfaceInfo:

http://mxr.mozilla.org/mozilla-central/source/xpcom/reflect/xptinfo/src/ShimInterfaceInfo.cpp#769

I don't understand why we don't implement that function, because we have the IID for the shimmed interface.

At the point where the call is actually failing, we have an xptiInterfaceInfo object.  So in greater detail, what you're proposing is:

- Implement the missing method in ShimInterfaceInfo;
- Find wherever we are getting this not-complete xpitInterfaceInfo object from; and
- Make it return ShimInterfaceInfos instead.

Does that sound plausible?  Apologies for being dense.

I'm also looking into bz's question in comment 30.
Flags: needinfo?(bobbyholley)
(In reply to Nathan Froyd (:froydnj) from comment #40)
> are because we've just failed a call for GetIIDForParamNoAlloc.
> ...
> I don't understand why we don't implement that function, because we have the
> IID for the shimmed interface.

They key point is the "For Param" part. This is about the parameters of methods on a given interface, and the shim interfaces claim to have no method.

> At the point where the call is actually failing, we have an
> xptiInterfaceInfo object.  So in greater detail, what you're proposing is:
> 
> - Implement the missing method in ShimInterfaceInfo;
> - Find wherever we are getting this not-complete xpitInterfaceInfo object
> from; and
> - Make it return ShimInterfaceInfos instead.

Note quite, given comment 40. If these calls are failing, it means we're trying to call methods on these interfaces, which obviously isn't going to work. So we need to figure out why that's happening.
Flags: needinfo?(bobbyholley)
(In reply to Bobby Holley (:bholley) from comment #41)
> Note quite, given comment 40. If these calls are failing, it means we're
> trying to call methods on these interfaces, which obviously isn't going to
> work. So we need to figure out why that's happening.

We're actually not calling methods on interfaces we've discarded.  We're calling methods on interfaces that have parameters of types of interfaces that we've discarded.  And it looks like, from the debugger, that xptiInterfaceEntry::ResolvedLocked() has failed in some mysterious way, preventing forward progress.  Continuing to dig...
One XPC_CANT_CREATE_WN error, not necessarily the ones coming from the test suite:

(gdb) c
Continuing.
++DOCSHELL 0x7fffc06e6000 == 6 [pid = 18374] [id = 6]
++DOMWINDOW == 14 (0x7fffc06c6000) [pid = 18374] [serial = 14] [outer = (nil)]
[18374] WARNING: NS_ENSURE_TRUE(globalObject && globalObject->GetGlobalJSObject()) failed: file /home/froydnj/src/gecko-dev.git/content/html/document/src/nsHTMLContentSink.cpp, line 740
++DOMWINDOW == 15 (0x7fffc06c2800) [pid = 18374] [serial = 15] [outer = 0x7fffc06c6000]
[18374] WARNING: NS_ENSURE_TRUE(window) failed: file /home/froydnj/src/gecko-dev.git/dom/base/nsLocation.cpp, line 45

Breakpoint 2, nsJSCID::CreateInstance (this=<optimized out>, iidval=..., cx=
    0x7fffe1647390, optionalArgc=<optimized out>, retval=...)
    at /home/froydnj/src/gecko-dev.git/js/xpconnect/src/XPCJSID.cpp:663
663	        return NS_ERROR_XPC_CANT_CREATE_WN;
(gdb) call DumpJSStack()
0 G_Alarm(callback = [function], delayMS = 60000) ["file:///opt/build/froydnj/build-debug/dist/bin/components/nsUrlClassifierLib.js":1237]
    this = [object Object]
1 anonymous(status = 404) ["file:///opt/build/froydnj/build-debug/dist/bin/components/nsUrlClassifierListManager.js":451]
    this = [object Object]
2 anonymous(args = "404") ["file:///opt/build/froydnj/build-debug/dist/bin/components/nsUrlClassifierLib.js":52]
    this = function () {
    // Combine the static args and the new args into one big array
    var args = boundargs.concat(Array.slice(arguments));
    return fn.apply(self, args);
  }

(gdb) where 5
#0  nsJSCID::CreateInstance (this=<optimized out>, iidval=..., cx=0x7fffe1647390, 
    optionalArgc=<optimized out>, retval=...)
    at /home/froydnj/src/gecko-dev.git/js/xpconnect/src/XPCJSID.cpp:663
#1  0x00007ffff1e14df6 in NS_InvokeByIndex (that=<optimized out>, 
    methodIndex=<optimized out>, paramCount=<optimized out>, params=<optimized out>)
    at /home/froydnj/src/gecko-dev.git/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:164
#2  0x00007ffff2abb3a0 in Invoke (this=0x7fffffff7910)
    at /home/froydnj/src/gecko-dev.git/js/xpconnect/src/XPCWrappedNative.cpp:2397
#3  CallMethodHelper::Call (this=this@entry=0x7fffffff7910)
    at /home/froydnj/src/gecko-dev.git/js/xpconnect/src/XPCWrappedNative.cpp:1738
#4  0x00007ffff2aacc83 in XPCWrappedNative::CallMethod (ccx=..., mode=<optimized out>)
    at /home/froydnj/src/gecko-dev.git/js/xpconnect/src/XPCWrappedNative.cpp:1705
(More stack frames follow...)
(gdb) p iid
$1 = (const nsID *) 0x7fffe160ddd8
(gdb) p *iid
$2 = {m0 = 423609210, m1 = 35492, m2 = 19753, m3 = "\252W\032͇\302kf"}
(gdb) p/x *iid
$3 = {m0 = 0x193fc37a, m1 = 0x8aa4, m2 = 0x4d29, m3 = {0xaa, 0x57, 0x1a, 0xcd, 0x87, 
    0xc2, 0x6b, 0x66}}
(gdb) p rv
$4 = <optimized out>
(gdb) p inst
$5 = [(nsTimerImpl*) 0x7fffc8d71f80]

I would try to provide the actual instance from the m5 testsuite, but setting appropriate breakpoints and trying to continue from gdb just hit more and more of these errors:

(gdb) c
Continuing.
[New Thread 0x7fffbfdff700 (LWP 18443)]

Breakpoint 2, nsJSCID::CreateInstance (this=<optimized out>, iidval=..., cx=
    0x7fffe1647390, optionalArgc=<optimized out>, retval=...)
    at /home/froydnj/src/gecko-dev.git/js/xpconnect/src/XPCJSID.cpp:663
663	        return NS_ERROR_XPC_CANT_CREATE_WN;
(gdb) p inst
$6 = [(nsConverterInputStream*) 0x7fffc8dcdaa0]
(gdb) call DumpJSStack()
0 NetUtil_readInputStreamToString(aInputStream = [xpconnect wrapped (nsISupports, nsIInputStream, nsIAsyncInputStream, nsISeekableStream, nsISearchableInputStream) @ 0x7fffc8dcd320 (native @ 0x7fffc1803e98)], aCount = 63932, aOptions = [object Object]) ["resource://gre/modules/NetUtil.jsm":271]
    this = [object Object]
1 anonymous(aInputStream = [xpconnect wrapped (nsISupports, nsIInputStream, nsIAsyncInputStream, nsISeekableStream, nsISearchableInputStream) @ 0x7fffc8dcd380 (native @ 0x7fffc1803e98)], aResult = 0, aRequest = [xpconnect wrapped (nsISupports, nsIChannel, nsIRequest) @ 0x7fffc8dcd3e0 (native @ 0x7fffc59ea080)]) ["resource://gre/modules/DirectoryLinksProvider.jsm":144]
    this = [object Object]
2 anonymous(aRequest = [xpconnect wrapped (nsISupports, nsIChannel, nsIRequest) @ 0x7fffc96ede80 (native @ 0x7fffc59ea080)], aContext = null, aStatusCode = 0) ["resource://gre/modules/NetUtil.jsm":123]
    this = [object Object]

(gdb) c
Continuing.

Breakpoint 2, nsJSCID::CreateInstance (this=<optimized out>, iidval=..., cx=
    0x7fffe1647390, optionalArgc=<optimized out>, retval=...)
    at /home/froydnj/src/gecko-dev.git/js/xpconnect/src/XPCJSID.cpp:663
663	        return NS_ERROR_XPC_CANT_CREATE_WN;
(gdb) p inst
$7 = [(nsScriptableUnicodeConverter*) 0x7fffc95f5d00]
(gdb) call DumpJSStack()
0 anonymous() ["resource://gre/modules/NewTabUtils.jsm":40]
    this = [object BackstagePass @ 0x7fffd91b3760 (native @ 0x7fffd9190390)]
1 anonymous() ["resource://gre/modules/XPCOMUtils.jsm":177]
    this = [object BackstagePass @ 0x7fffd91b3760 (native @ 0x7fffd9190390)]
2 toHash(aValue = "https://www.facebook.com/") ["resource://gre/modules/NewTabUtils.jsm":73]
3 BlockedLinks_isBlocked(aLink = [object Object]) ["resource://gre/modules/NewTabUtils.jsm":512]
    this = [object Object]
4 anonymous(link = [object Object], 0, [object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object]) ["resource://gre/modules/NewTabUtils.jsm":803]
5 Links_getLinks() ["resource://gre/modules/NewTabUtils.jsm":802]
    this = [object Object]
6 anonymous() ["resource://gre/modules/NewTabUtils.jsm":1117]
7 executeCallbacks() ["resource://gre/modules/NewTabUtils.jsm":774]
8 anonymous() ["resource://gre/modules/NewTabUtils.jsm":786]
    this = [object Object]
9 anonymous(links = [object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object]) ["resource://gre/modules/NewTabUtils.jsm":865]
    this = [object Object]
10 anonymous(rawLinks = [object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object]) ["resource://gre/modules/DirectoryLinksProvider.jsm":207]
    this = [object Object]
11 anonymous(aInputStream = [xpconnect wrapped (nsISupports, nsIInputStream, nsIAsyncInputStream, nsISeekableStream, nsISearchableInputStream) @ 0x7fffc8dcd380 (native @ 0x7fffc1803e98)], aResult = 0, aRequest = [xpconnect wrapped (nsISupports, nsIChannel, nsIRequest) @ 0x7fffc8dcd3e0 (native @ 0x7fffc59ea080)]) ["resource://gre/modules/DirectoryLinksProvider.jsm":155]
    this = [object Object]
12 anonymous(aRequest = [xpconnect wrapped (nsISupports, n

bz, is this anything like what you expected the answer to comment 30 to be?
Flags: needinfo?(bzbarsky)
Not at all, actually!  That first one is for nsITimer, which is presumably a scriptable interface and all, and not in any way affected by the DOM bits...
Flags: needinfo?(bzbarsky)
Depends on: 1012748
Figured out what the XPC_CANT_CREATE_WN problem was:

- inIDeepTreeWalker, for layout/inspector/ inherited from nsIDOMTreeWalker
- nsIDOMTreeWalker was marked unscriptable
- presumably XPConnect was unable to construct the prototype chain for instantiating inIDeepTreeWalker instances or somesuch, because it didn't have any information for nsIDOMTreeWalker?

I tried to return shim interface information from xptiInterfaceInfo::GetParent, but that didn't have the desired effect.

Since inIDeepTreeWalker is one of the two remaining things that inherit from non-scriptable interfaces, WDYT about just declaring it to have all of nsIDOMTreeWalker's methods/properties, and making it inherit from nsISupports instead?
Flags: needinfo?(bzbarsky)
That sounds just fine to me.
Flags: needinfo?(bzbarsky)
Depends on: 1013475
Smart xpt linking will keep around [scriptable] interfaces and anything
those interfaces depend on.  Modify the tests that deal with xpt linking
so they use [scriptable] interfaces, ensuring that the tests continue to
work in the face of smarter linkers.
Attachment #8425656 - Flags: review?(ted)
xpt linking works with two different currencies: UUIDs of interfaces, and names
of interfaces.  Names are how we support linking in unknown-at-compile-time
interfaces at runtime.  Since we need to know names to decide whether we have
shims for interfaces or not, we need a method for exposing the names.

I think bholley is the closest person to this stuff, so he gets the pleasure of
reviewing it.
Attachment #8425660 - Flags: review?(bobbyholley)
The code that looks up |xptiInterfaceEntry|s starts by figuring out where in
the typelib the desired entry lives.  We want to do the same thing for the
purpose of getting shims, so we can ask the typelib for the name of the thing
at that index.  Split out a method to do this for us.
Attachment #8425664 - Flags: review?(bobbyholley)
This patch does all the interesting work: we check for shims whenever we get
asked about parameter information for methods.  If there's a shim for what we
want, we ask the shim to provide the information.  If not, we fail as usual.

Happily, doing this means that we don't need to mark any DOM interfaces as
[scriptable]; the shims take care of providing the necessary information for
parameters.
Attachment #8420162 - Attachment is obsolete: true
Attachment #8425667 - Flags: review?(bobbyholley)
Final version of the xpt linker modifications.

I suspect that this has potential to cause c-c heartburn and/or cause problems
for addons on m-c or c-c.  (I think, for instance, that some of the interfaces
required for Thunderbird's secure mail stuff get discarded, though I am unsure
if those interfaces are accessed from Javascript or not.)  If we need to
reconsider our approach, backing this out or disabling it is straightforward.
Attachment #8420161 - Attachment is obsolete: true
Attachment #8425669 - Flags: review?(ted)
Thanks a lot, Nathan, for driving this through!

What's the final measurement on how much this saves us?
Attachment #8425660 - Flags: review?(bobbyholley) → review+
Attachment #8425664 - Flags: review?(bobbyholley) → review+
Comment on attachment 8425667 [details] [diff] [review]
part 3 - try to shim param information in xptiInterfaceEntry when possible

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

Sorry for the delay here. I may be the de-facto owner here, but it's not like I've ever looked at this code before. :P
Attachment #8425667 - Flags: review?(bobbyholley) → review+
Comment on attachment 8425656 [details] [diff] [review]
part 0 - make tests use [scriptable] interfaces

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

::: xpcom/typelib/xpt/tools/runtests.py
@@ +387,5 @@
>          
>          """
>          t1 = xpt.Typelib()
>          # add an unresolved interface
> +        t1.interfaces.append(xpt.Interface("IFoo", scriptable=True))

I wonder if it'd be worth making a TestInterface helper method that returned an xpt.Interface with scriptable=True to make this simpler?
Attachment #8425656 - Flags: review?(ted) → review+
Comment on attachment 8425669 [details] [diff] [review]
part 4 - throw away unscriptable interfaces that aren't otherwise needed

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

::: xpcom/typelib/xpt/tools/xpt.py
@@ +1368,5 @@
>              checkType(m.result.type)
>              for p in m.params:
>                  checkType(p.type)
> +
> +    # There's no need to have non-scriptable interfaces in a typelib, and

I guess we have to do this after we do the merging so we can transitively find all the things we need to keep, right?

@@ +1371,5 @@
> +
> +    # There's no need to have non-scriptable interfaces in a typelib, and
> +    # removing them saves memory when typelibs are loaded.  But we can't
> +    # just blindly remove all non-scriptable interfaces, since we still
> +    # need to know about interfaces for parameter information and the like.

I would probably phrase this as "we still need to know about non-scriptable interfaces that are referenced from scriptable interfaces."

Also, you should move this comment up to the docstring for this function, since it's non-obvious that this would be the behavior.

@@ +1372,5 @@
> +    # There's no need to have non-scriptable interfaces in a typelib, and
> +    # removing them saves memory when typelibs are loaded.  But we can't
> +    # just blindly remove all non-scriptable interfaces, since we still
> +    # need to know about interfaces for parameter information and the like.
> +    worklist = set(filter(lambda i: i.scriptable, interfaces))

I would probably write this as a generator expression instead, but it's basically the same:
set(i for i in interfaces if i.scriptable)

@@ +1379,5 @@
> +        if iface in required_interfaces or iface in worklist:
> +            return
> +        worklist.add(iface)
> +
> +    while len(worklist) != 0:

while worklist:

@@ +1391,5 @@
> +            for p in m.params:
> +                if isinstance(p.type, InterfaceType):
> +                    maybe_add_to_worklist(p.type.iface)
> +                elif isinstance(p.type, ArrayType) and isinstance(p.type.element_type, InterfaceType):
> +                    maybe_add_to_worklist(p.type.element_type.iface)

Almost seems like you want to write a little helper that does the isinstance/maybe_add_to_worklist and just call it, but it's not critical.
Attachment #8425669 - Flags: review?(ted) → review+
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #52)
> What's the final measurement on how much this saves us?

~100K on x86 32-bit (measured just now), ~140K on 64-bit (IIRC).
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3253a7cb5d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9e56658ef05
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c2eaff26d0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c7acfdd623b
https://hg.mozilla.org/integration/mozilla-inbound/rev/bee33343ee18

I left the interfaces in part 0 alone.  I applied the review feedback for part 4, writing a brief blurb in the doc comment about removing unscriptable interfaces, but keeping the explanatory comment in the code; I felt that it was valuable to have both, and that the details in the code comment didn't belong in the doc comment.

I'll go make a post in dev-platform about this change.
Flags: in-testsuite-
> ~100K on x86 32-bit (measured just now), ~140K on 64-bit (IIRC).

Excellent. Is the reduction in libxul.so code size?
(In reply to Nicholas Nethercote [:njn] from comment #58)
> > ~100K on x86 32-bit (measured just now), ~140K on 64-bit (IIRC).
> 
> Excellent. Is the reduction in libxul.so code size?

No.  That reduction is runtime memory, as measured by about:memory.  There's a ~70K reduction in disk space required by the .xpts, as comment 17 suggests.
> That reduction is runtime memory, as measured by about:memory.

Aha. Is it "xpti-working-set" that shrank?
(In reply to Nicholas Nethercote [:njn] from comment #60)
> > That reduction is runtime memory, as measured by about:memory.
> 
> Aha. Is it "xpti-working-set" that shrank?

Yes.
Depends on: 1030399
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: