Closed Bug 994964 Opened 10 years ago Closed 10 years ago

Consider removing [scriptable] from all the DOM XPIDL interfaces

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32

People

(Reporter: khuey, Assigned: ehsan.akhgari)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 files, 11 obsolete files)

Patch (v4)
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
Make GetConstant accept a MutableHandleValue
(deleted), patch
bholley
: review+
Details | Diff | Splinter Review 
Shrinking the size of our typelibs will save us precious memory on b2g.  We also don't have the addon constraints that desktop has.
Should we mark them as [scriptable_for_addons] or something like that?
(where [scriptable_for_addons] is an attribute added in this bug which makes things [scriptable] if desktop, and not otherwise)
If we're going to do that much work, we could just make them non-scriptable, period.  Addons are only likely to use these things for instanceof Components.interfaces.nsIDOMFoo, right?  We can make that work via some mapping of nsIFoo to WebIDL proto id or something.  We do something similar for the components shim already, though with the actual WebIDL ctor objects, which we won't have in non-window scopes in general...
(In reply to Boris Zbarsky [:bz] from comment #3)
> If we're going to do that much work, we could just make them non-scriptable,
> period.  Addons are only likely to use these things for instanceof
> Components.interfaces.nsIDOMFoo, right?  We can make that work via some
> mapping of nsIFoo to WebIDL proto id or something.  We do something similar
> for the components shim already, though with the actual WebIDL ctor objects,
> which we won't have in non-window scopes in general...

We could resolve them as-needed with a resolve hook that called GetConstructorObject.
Sounds fine to me!
Whoever works on this - have a look at the LookupComponentsShim stuff in nsDOMClassInfo.cpp. It's basically what you need, except that we probably want a second resolve hook on the phony Components.interface object, which will call GetConstructorObject and define it.
I'll take a stab at this.
Assignee: nobody → ehsan
So I started at this, and removed a whole bunch of scriptable interfaces, and measured the size difference between dom_base.xpt before and after, and found out that the size is the exact same.  Looking at <http://mxr.mozilla.org/mozilla-central/source/xpcom/typelib/xpt/tools/xpt.py#973> it indeed seems that the presence of [scriptable] only affects the value of a flag that we store in the xpt binary.

So, will this really change the size of our typelibs as comment 0 suggests?  What am I missing?
We should modify the XPIDL compiler to just not emit typelibs at all for non-scriptable and non-builtinclass interfaces. That might already give us some significant savings.

Does non-scriptable imply builtinclass? Absent any compelling counterexamples, I think it probably should. We should do make that change, and then make the above change, I think.
(In reply to Bobby Holley (:bholley) from comment #9)
> We should modify the XPIDL compiler to just not emit typelibs at all for
> non-scriptable and non-builtinclass interfaces. That might already give us
> some significant savings.

Indeed.  That's probably an artifact from the days when we used typelibs for XPCOM proxies.
OK, makes sense.
Attached patch WIP (obsolete) (deleted) — — Splinter Review 

    
    

    
  
Comment on attachment 8405794 [details] [diff] [review]
WIP

OK, here is a partial sketch of a patch.  The reason I'm asking for review is that this seems to define Ci.nsIDOMFoo names but doing the instanceof checks on these names does not work correctly.  Bobby, any idea what I'm doing wrong?

        Attachment #8405794 -
        Attachment description: Mark the XPIDL interfaces for WebIDL interfaces as non-scriptable and provide a shim to make them accessible through the WebIDL interface objects → WIP

        Attachment #8405794 -
        Flags: feedback?(bobbyholley)

    
    

    
  
>+            *objp = idobj;

That should be set to "obj", not to "idobj", no?

    
    

    
  
Comment on attachment 8405794 [details] [diff] [review]
WIP

(In reply to Boris Zbarsky [:bz] from comment #14)
> >+            *objp = idobj;
> 
> That should be set to "obj", not to "idobj", no?

D'oh, you're right!  Thanks.

        Attachment #8405794 -
        Attachment is obsolete: true

        Attachment #8405794 -
        Flags: feedback?(bobbyholley)

    
  
Depends on: 995753, 995754, 995757

    
    

    
  
OK, I tested my patch on try, and it seems like this technique doesn't work for passing the interface object to QueryInterface (although instanceof works as expected).  Passing one of these shim objects to QueryInterface causes a NS_ERROR_XPC_BAD_CONVERT_JS error.

Bobby, any idea how I should fix that?
Flags: needinfo?(bobbyholley)

    
    

    
  
Ugh.  We have two options.

Either we change mozilla::dom::QueryInterface over in BindingUtils.cpp to handle the case when args[0] is a WebIDL interface object, or we need to change these shim objects we hang off of Components.interfaces to be nsIJSID objects somehow.

I guess the former is as easy as checking IsDOMIfaceAndProtoClass on the js::Class and if so casting doing DOMIfaceAndProtoJSClass::FromJSClass.  Then we can do a typecheck similar to the one mozilla::dom::InterfaceHasInstance does.  In fact, maybe even factor that code out and share it.

    
    

    
  
(In reply to Boris Zbarsky [:bz] from comment #18)
> I guess the former is as easy as checking IsDOMIfaceAndProtoClass on the
> js::Class and if so casting doing DOMIfaceAndProtoJSClass::FromJSClass. 
> Then we can do a typecheck similar to the one
> mozilla::dom::InterfaceHasInstance does.  In fact, maybe even factor that
> code out and share it.

Yeah, this sounds good. But why the heck is anyone QIing to ClassInfo interfaces?
Flags: needinfo?(bobbyholley)

    
    

    
  
Presumably as a kind of dumb instanceof check...

    
    

    
  
OK, so Boris' suggestion fixes the failure in dom/xbl/test/test_bug821850.html, but still not the ones in the browser/components/preferences tests.  Specifically, this is the failing QI: <http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/tests/privacypane_tests_perwindow.js#11> and dom::QueryInterface fails on that because IsDOMIfaceAndProtoClass returns false.  Here's the JSClass there:

(lldb) p/x *clasp
(const js::Class) $23 = {
  name = 0x00000001076779b2 "Proxy"
  flags = 0x7c140420
  addProperty = 0x000000010617bea0 (XUL`JS_PropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) at jsapi.cpp:2218)
  delProperty = 0x000000010617f030 (XUL`JS_DeletePropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool*) at jsapi.cpp:2230)
  getProperty = 0x000000010617bea0 (XUL`JS_PropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) at jsapi.cpp:2218)
  setProperty = 0x000000010617bec0 (XUL`JS_StrictPropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>) at jsapi.cpp:2224)
  enumerate = 0x000000010617f060 (XUL`JS_EnumerateStub(JSContext*, JS::Handle<JSObject*>) at jsapi.cpp:2237)
  resolve = 0x000000010617f080 (XUL`JS_ResolveStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>) at jsapi.cpp:2243)
  convert = 0x000000010628e9c0 (XUL`js::proxy_Convert(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) at jsproxy.cpp:3012)
  finalize = 0x000000010628ea70 (XUL`js::proxy_Finalize(js::FreeOp*, JSObject*) at jsproxy.cpp:3019)
  call = 0x000000010628eb90 (XUL`js::proxy_Call(JSContext*, unsigned int, JS::Value*) at jsproxy.cpp:3036)
  hasInstance = 0x000000010628eb10 (XUL`js::proxy_HasInstance(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>, bool*) at jsproxy.cpp:3026)
  construct = 0x000000010628ecb0 (XUL`js::proxy_Construct(JSContext*, unsigned int, JS::Value*) at jsproxy.cpp:3045)
  trace = 0x000000010628e640 (XUL`js::proxy_Trace(JSTracer*, JSObject*) at jsproxy.cpp:2957)
  spec = {
    createConstructor = 0x0000000000000000
    createPrototype = 0x0000000000000000
    constructorFunctions = 0x0000000000000000
    prototypeFunctions = 0x0000000000000000
    finishInit = 0x0000000000000000
  }
  ext = {
    outerObject = 0x0000000000000000
    innerObject = 0x0000000000000000
    iteratorObject = 0x0000000000000000
    isWrappedNative = 0x00
    weakmapKeyDelegateOp = 0x000000010628e920 (XUL`js::proxy_WeakmapKeyDelegate(JSObject*) at jsproxy.cpp:3005)
  }
  ops = {
    lookupGeneric = 0x000000010628d5d0 (XUL`js::proxy_LookupGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JSObject*>, JS::MutableHandle<js::Shape*>) at jsproxy.cpp:2794)
    lookupProperty = 0x000000010628d6d0 (XUL`js::proxy_LookupProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JSObject*>, JS::MutableHandle<js::Shape*>) at jsproxy.cpp:2812)
    lookupElement = 0x000000010628d7c0 (XUL`js::proxy_LookupElement(JSContext*, JS::Handle<JSObject*>, unsigned int, JS::MutableHandle<JSObject*>, JS::MutableHandle<js::Shape*>) at jsproxy.cpp:2820)
    defineGeneric = 0x000000010628d8e0 (XUL`js::proxy_DefineGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>), bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>), unsigned int) at jsproxy.cpp:2830)
    defineProperty = 0x000000010628da50 (XUL`js::proxy_DefineProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<js::PropertyName*>, JS::Handle<JS::Value>, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>), bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>), unsigned int) at jsproxy.cpp:2843)
    defineElement = 0x000000010628db60 (XUL`js::proxy_DefineElement(JSContext*, JS::Handle<JSObject*>, unsigned int, JS::Handle<JS::Value>, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>), bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>), unsigned int) at jsproxy.cpp:2851)
    getGeneric = 0x000000010628dcb0 (XUL`js::proxy_GetGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) at jsproxy.cpp:2861)
    getProperty = 0x000000010628dd10 (XUL`js::proxy_GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) at jsproxy.cpp:2868)
    getElement = 0x000000010628de00 (XUL`js::proxy_GetElement(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, unsigned int, JS::MutableHandle<JS::Value>) at jsproxy.cpp:2876)
    setGeneric = 0x000000010628df20 (XUL`js::proxy_SetGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, bool) at jsproxy.cpp:2886)
    setProperty = 0x000000010628df90 (XUL`js::proxy_SetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>, bool) at jsproxy.cpp:2893)
    setElement = 0x000000010628e080 (XUL`js::proxy_SetElement(JSContext*, JS::Handle<JSObject*>, unsigned int, JS::MutableHandle<JS::Value>, bool) at jsproxy.cpp:2901)
    getGenericAttributes = 0x000000010628e1a0 (XUL`js::proxy_GetGenericAttributes(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, unsigned int*) at jsproxy.cpp:2910)
    setGenericAttributes = 0x000000010628e290 (XUL`js::proxy_SetGenericAttributes(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, unsigned int*) at jsproxy.cpp:2920)
    deleteProperty = 0x000000010628e3e0 (XUL`js::proxy_DeleteProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<js::PropertyName*>, bool*) at jsproxy.cpp:2941)
    deleteElement = 0x000000010628e540 (XUL`js::proxy_DeleteElement(JSContext*, JS::Handle<JSObject*>, unsigned int, bool*) at jsproxy.cpp:2948)
    watch = 0x000000010628edd0 (XUL`js::proxy_Watch(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JSObject*>) at jsproxy.cpp:3054)
    unwatch = 0x000000010628ee20 (XUL`js::proxy_Unwatch(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>) at jsproxy.cpp:3060)
    slice = 0x000000010628ee60 (XUL`js::proxy_Slice(JSContext*, JS::Handle<JSObject*>, unsigned int, unsigned int, JS::Handle<JSObject*>) at jsproxy.cpp:3067)
    enumerate = 0x0000000000000000
    thisObject = 0x0000000000000000
  }
}

I guess I should treat proxies differently here.  Any idea how?

    
    

    
  
Another question, don't we have to ultimately imitate an nsJSIID here anyways for backward compat?  For example, my patch makes Components.interfaces.nsIDOMAttr.number resolve to undefined now.  Not sure if there are any add-ons that depend on that though.

    
    

    
  
> Any idea how?

CheckedUnwrap before checking for IsDOMIfaceAndProtoClass?

> don't we have to ultimately imitate an nsJSIID here anyways for backward compat

Depending on how much compat we want, maybe...

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #22)
> OK, so Boris' suggestion fixes the failure in
> dom/xbl/test/test_bug821850.html

Oh hm, that's really not what that test was intending. Can you switch it to do a real QI (say, QIing an image to nsIImageLoadingContent), to make sure that we continue to test QI from XBL scopes?

> I guess I should treat proxies differently here.  Any idea how?

Try

obj = js::CheckedUnwrap(obj, /* stopAtOuter = */ false)

before detecting the constructor.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #23)
> Another question, don't we have to ultimately imitate an nsJSIID here
> anyways for backward compat?  For example, my patch makes
> Components.interfaces.nsIDOMAttr.number resolve to undefined now.  Not sure
> if there are any add-ons that depend on that though.

I'm hoping that we won't need that level of compat. This is never going to be fully compatible.

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #25)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #22)
> > OK, so Boris' suggestion fixes the failure in
> > dom/xbl/test/test_bug821850.html
> 
> Oh hm, that's really not what that test was intending. Can you switch it to
> do a real QI (say, QIing an image to nsIImageLoadingContent), to make sure
> that we continue to test QI from XBL scopes?

Sorry, I'm not sure how to parse this.  There is no nsIImageLoadingContent involved there, this is the QI call in question: <http://mxr.mozilla.org/mozilla-central/source/dom/xbl/test/file_bug821850.xhtml#59>, it's made on a document object.  Can you please tell me what you exactly want me to do with that test?  (Not sure if I understand all of the details related to bug 821850.)
Flags: needinfo?(bobbyholley)

    
    

    
  
(Also, FWIW I'm planning to deal with making non-scriptable interfaces not generate an entry in the xpt files in a follow-up bug yet to be filed.)

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #27)
> (Also, FWIW I'm planning to deal with making non-scriptable interfaces not
> generate an entry in the xpt files in a follow-up bug yet to be filed.)

I am willing to do that (doing the work) if you don't want to.

    
    

    
  
(In reply to comment #28)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #27)
> > (Also, FWIW I'm planning to deal with making non-scriptable interfaces not
> > generate an entry in the xpt files in a follow-up bug yet to be filed.)
> 
> I am willing to do that (doing the work) if you don't want to.

Offer accepted.  :-)  Let me file a bug and put what I know so far in it.

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #26)
> > Oh hm, that's really not what that test was intending. Can you switch it to
> > do a real QI (say, QIing an image to nsIImageLoadingContent), to make sure
> > that we continue to test QI from XBL scopes?
> 
> Sorry, I'm not sure how to parse this.  There is no nsIImageLoadingContent
> involved there, this is the QI call in question:
> <http://mxr.mozilla.org/mozilla-central/source/dom/xbl/test/file_bug821850.
> xhtml#59>, it's made on a document object.  Can you please tell me what you
> exactly want me to do with that test?  (Not sure if I understand all of the
> details related to bug 821850.)

Basically, the test is designed to check whether XBL can successfully QI content objects to arbitrary interfaces. I was kind of lazy when I wrote that test, and just QIed document to nsIDOMDocument, which would have effectively been a no-op, since document is already QI-ed to nsIDOMDocument via classinfo. And in this test we're making it even more of a no-op.

So I just meant that we should stick a dummy image somewhere in the target document, and make sure we can QI it to nsIIImageLoadingContent, so that we actually test QI from XBL.
Flags: needinfo?(bobbyholley)

    
  
Depends on: 996061

    
    

    
  
I discussed this with Boris on IRC and here's what we plan to do:

Add our own implementation of nsIJSIID, feed the IID from the interface using NS_GET_IID to it, create an object of that type in the resolve hook, wrap it using nsContentUtils::WrapNative, and then define the WebIDL constants on it like below:

const NativeProperties* props =
 FooBinding::sNativePropertyHooks->mNativeProperties.regular;
if (props && props->constants) {
 if (!DefineConstants(cx, obj, props->constants)) {
   // throw
 }
}

    
  
Blocks: 996061
No longer depends on: 996061

    
  
Depends on: 996158
Whiteboard: [MemShrink] → [MemShrink:P2]
Depends on: 996911

    
  
Depends on: 996955

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Implement bug 994964 comment 31 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #32)
> Created attachment 8407249 [details] [diff] [review]
> Implement bug 994964 comment 31

The DefineConstants call for nsIDOMKeyEvent fails with this because the JS_DefineProperty call ends up failing.  Not sure what I'm doing wrong...  Any ideas?

    
    

    
  
Well, my hypothesis was correct.  ;)

When we try to define the constants we land in XPC_WN_OnlyIWrite_AddPropertyStub which throws NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN.

This happens because our object's JSClass is XPC_WN_NoHelper_JSClass, which does not allow adding "expandos" to the object.

I think there are two pretty simple ways around this.

One is to insert a vanilla object into the proto chain of our XPCWrappedNative and define the constants on that.

The other is to give our new thing classinfo that QIs to nsIXPCScriptable.  Basically either an nsDOMClassInfo, or just inherit from nsIXPCScriptable directly and do the xpc_map_end.h thing.  The latter is probably more future-proof and simpler than trying to wedge nsDOMClassInfo in here.

I suspect the vanilla object route is simpler, fwiw.  :(

    
    

    
  
(In reply to Boris Zbarsky [:bz] from comment #34)
> I suspect the vanilla object route is simpler, fwiw.  :(

Yes please. nsIXPCScriptable needs to go away (mostly), so let's not add any new consumers.

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #35)
> (In reply to Boris Zbarsky [:bz] from comment #34)
> > I suspect the vanilla object route is simpler, fwiw.  :(
> 
> Yes please. nsIXPCScriptable needs to go away (mostly), so let's not add any
> new consumers.

Can you please give me some pointers on the JSAPIs involved for that (or even better, another place in the code where we do this kind of thing?)  Thanks!

    
    

    
  
(Note that I'm not very well versed in JSAPIs.)

    
    

    
  
  JS::Rooted<JSObject*> wrapped(cx, &wrappedv.toObject());
  const NativeProperties* props = shimEntry.nativeProps.regular;
  if (props && props->constants) {  JS::Rooted<JSObject*> proto(cx);
    JS::Rooted<JSObject*> proto(cx);
    if (!js::GetObjectProto(cx, wrapped, &proto)) {
      return false;
    }
    JS::Rooted<JSObject*> constantHolder(cx);
    constantHolder = JS_NewObject(cx, nullptr, proto, &constantHolder);
    if (!constantHolder) {
      return false;
    }
    if (!DefineConstants(cx, constantHolder, props->constants)) {
      return false;
    }
    if (!JS_SplicePrototype(cx, wrapped, constantHolder)) {
      return false;
    }
  }

    
    

    
  
Is it possible to create a shim without a corresponding XPCOM interface such as nsIDOMSVGLength?
Blocks: 984461

    
    

    
  
(In reply to Masatoshi Kimura [:emk] from comment #39)
> Is it possible to create a shim without a corresponding XPCOM interface such
> as nsIDOMSVGLength?

No, because we use the IID, but the shims being created here are different than the one that bug 984461 is about.  Those shims are used for Web content that depends on Components.interfaces.foo.  The shims being designed here are for chrome JS which depends on them.
No longer blocks: 984461

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #35)
> (In reply to Boris Zbarsky [:bz] from comment #34)
> > I suspect the vanilla object route is simpler, fwiw.  :(
> 
> Yes please. nsIXPCScriptable needs to go away (mostly), so let's not add any
> new consumers.

OK, so I fixed the previous problem by injecting an object into the prototype chain like Boris suggested, and now we get to a point where passing the shim Ci objects to instanceof fails because the XPCWrappedNative_NoHelper wrapper object's class does not have a hasInstance hook!  This can obviously be done through nsIXPCScriptable but given your comment above, is there another way of doing this?

    
  
Depends on: 998725

    
  
Depends on: 998727

    
  
Depends on: 998729

    
  
Depends on: 998731

    
  
Depends on: 998732

    
  
Depends on: 998733

    
  
Depends on: 998735

    
  
Depends on: 998738

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #41)
> (In reply to Bobby Holley (:bholley) from comment #35)
> > (In reply to Boris Zbarsky [:bz] from comment #34)
> > > I suspect the vanilla object route is simpler, fwiw.  :(
> > 
> > Yes please. nsIXPCScriptable needs to go away (mostly), so let's not add any
> > new consumers.
> 
> OK, so I fixed the previous problem by injecting an object into the
> prototype chain like Boris suggested, and now we get to a point where
> passing the shim Ci objects to instanceof fails because the
> XPCWrappedNative_NoHelper wrapper object's class does not have a hasInstance
> hook!  This can obviously be done through nsIXPCScriptable but given your
> comment above, is there another way of doing this?

Hm, Yuck.

Instead of trying to duplicate the nsJSIID machinery, maybe we should embrace the machinery more, and generate dummy implementations of nsIInterfaceInfo that have only constants, and register those somewhere along the pipeline (perhaps via XPTInterfaceInfoManager)?

    
    

    
  
(In reply to comment #42)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #41)
> > (In reply to Bobby Holley (:bholley) from comment #35)
> > > (In reply to Boris Zbarsky [:bz] from comment #34)
> > > > I suspect the vanilla object route is simpler, fwiw.  :(
> > > 
> > > Yes please. nsIXPCScriptable needs to go away (mostly), so let's not add any
> > > new consumers.
> > 
> > OK, so I fixed the previous problem by injecting an object into the
> > prototype chain like Boris suggested, and now we get to a point where
> > passing the shim Ci objects to instanceof fails because the
> > XPCWrappedNative_NoHelper wrapper object's class does not have a hasInstance
> > hook!  This can obviously be done through nsIXPCScriptable but given your
> > comment above, is there another way of doing this?
> 
> Hm, Yuck.
> 
> Instead of trying to duplicate the nsJSIID machinery, maybe we should embrace
> the machinery more, and generate dummy implementations of nsIInterfaceInfo that
> have only constants, and register those somewhere along the pipeline (perhaps
> via XPTInterfaceInfoManager)?

The constants problem is fixed now, like I said above.  It's the instanceof which is now an issue.

The issue is that the XPC_WN_NoHelper_JSClass class which nsContentUtils::WrapNative uses for the JS object has a null hasInstance <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNativeJSOps.cpp#680>, which causes the JS engine to generate an error when you use the object as an operand of instanceof.  Even if we find another way to put the constants on the JS object, we still need to solve this somehow.

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #43)
> The constants problem is fixed now, like I said above.  It's the instanceof
> which is now an issue.

Yes, I'm aware. My suggestion was a solution to both problems.
 
> The issue is that the XPC_WN_NoHelper_JSClass class which
> nsContentUtils::WrapNative uses for the JS object has a null hasInstance
> <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/
> XPCWrappedNativeJSOps.cpp#680>, which causes the JS engine to generate an
> error when you use the object as an operand of instanceof.  Even if we find
> another way to put the constants on the JS object, we still need to solve
> this somehow.

Right. nsJSID uses an nsIXPCScriptable hook for custom hasInstance behavior. So we have 3 options:
* Reuse nsJSID (my proposal above)
* Create something else that also implements nsIXPCScriptable. This isn't great, because it creates more work for us when we eventually move away from nsIXPCScriptable in its current form.
* Create an XPCNWN_NoHelper_HasInstance hook that just checks for this case. Not great either.

    
    

    
  
Ah I see, so if you're suggesting to re-use nsJSIID, then I'm not sure if I understand comment 42 correctly.  Can you please elaborate?

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #45)
> Ah I see, so if you're suggesting to re-use nsJSIID, then I'm not sure if I
> understand comment 42 correctly.  Can you please elaborate?

nsJSIID instances are constructed with an nsIInterfaceInfo, which tells them which interface they represent:
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSID.cpp#302

When resolving properties, Components.interfaces consults the XPTInterfaceInfoManager to get the nsIInterfaceInfo for a given interface name:
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCComponents.cpp#308

So I was suggesting that you try to hack the XPTInterfaceInfoManager to return something for the typelib-less shims. The alternative woul be to hack XPCComponents_Interface::NewResolve directly, but then you'd probably have to hack ::NewEnumerate as well.

    
    

    
  
Aha, I finally understand your proposal.  :-)  I think in order to make it work, we basically need to move the IID->WebIDL binding mapping that I have produced so far to XPTInterfaceInfoManager and then implement a new version of nsIInterfaceInfo which basically lies in response to everything expect for querying about its constants, and getting its name and IID.  That might however be more easier said than done because of two reasons:

1. We can't just do the above!  For example, we don't have information about interface inheritance captured anywhere.  Also, nsXPTConstant is not really compatible with the information that we have in ConstantSpec, which means that we probably need to change nsIInterfaceInfo itself.

2. I think before someone tries it, we won't know how far we need to extend the lies that this new nsIInterfaceInfo type thing tells to its consumers.  We might be breaking all kinds of assumptions in the existing consumers, and we're going to have to compensate for that by either changing the consumers or lying more.  This might of course just be me being cautious.

This seems like a fair amount of work, so I'd like to make sure that we really don't want to keep going down the nsXPCComponents_Interfaces::NewResolve route as I've been doing so far.  (FWIW I don't think we need to hack NewEnumerate as well, these interfaces will just not be there if you enumerate Ci -- I doubt that is going to break any add-ons.)

Looking at the other alternatives at the end of comment 44, I don't really like implementing nsIXPCScriptable that much myself either, but why is adding a XPCNWN_NoHelper_HasInstance hook so bad?  It seems like much less work, and I usually prefer small local hacks over the broader kinds of hacks such as the nsIInterfaceInfo trickery above.

Bobby, Boris, what do you guys think?

    
    

    
  
So I and Bobby discussed this a bit on IRC.  Bobby suggested instead of hooking our fake nsIInterfaceInfo object into XPTInterfaceInfoManager, break the couple of nsJSIID and XPTInterfaceInfoManager and just create those fake objects in XPCComponents_Interfaces::NewResolve so that this hackery would at least be limited to that place.  I didn't manage to convince him that what I've done so far is simpler, and neither of us has hard data on either of the two approaches.  So, here's what I'll do, I'll post a snapshot of my work so far here, and then will remove the stuff I've implemented so far and implement Bobby's suggestion and see how things turn out.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Snapshot of my work up to comment 38 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #8406137 -
        Attachment is obsolete: true

        Attachment #8407249 -
        Attachment is obsolete: true

    
    

    
  
OK, so one assumption here was that we can break the coupling between nsJSIID and XPTInterfaceInfoManager.  That requires passing in an nsIInterfaceInfo* argument to xpc::HasInstance so that it can use it here <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSID.cpp#527> to avoid going through XPTInterfaceInfoManager.  But that is not possible since this call site won't have an nsIInterfaceInfo* pointer to use.

What should I do, Bobby?
Flags: needinfo?(bobbyholley)

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #50)
> What should I do, Bobby?

I can fix that for you. Filing a dependent bug.
Flags: needinfo?(bobbyholley)
Depends on: 999297

    
    

    
  
OK, so despite bug 999297 comment 5, XPCNativeInterface::GetNewOrUsed(nsIID*) is indeed a problem.  I get to that function with this callstack:

  * frame #0: 0x00000001033a09cd XUL`XPCNativeInterface::GetNewOrUsed(iid=0x00000001068df8c0) + 205 at XPCWrappedNativeInfo.cpp:131
    frame #1: 0x000000010333bd7a XUL`XPCConvert::NativeInterface2JSObject(d=JS::MutableHandleValue at 0x00007fff5fbf7e28, dest=0x00007fff5fbf8188, aHelper=0x00007fff5fbf7e80, iid=0x00000001068df8c0, Interface=0x0000000000000000, allowNativeWrapper=true, pErr=0x00007fff5fbf7eb4) + 1546 at XPCConvert.cpp:867
    frame #2: 0x00000001033b8a9b XUL`NativeInterface2JSObject(aScope=JS::HandleObject at 0x00007fff5fbf7f68, aCOMObj=0x0000000119fae3d0, aCache=0x0000000000000000, aIID=0x00000001068df8c0, aAllowWrapping=true, aVal=JS::MutableHandleValue at 0x00007fff5fbf7f40, aHolder=0x00007fff5fbf8188) + 251 at nsXPConnect.cpp:502
    frame #3: 0x00000001033b896e XUL`nsXPConnect::WrapNative(this=0x0000000110a79c90, aJSContext=0x0000000110ca5170, aScopeArg=0x0000000118762ca0, aCOMObj=0x0000000119fae3d0, aIID=0x00000001068df8c0, aHolder=0x00007fff5fbf8188) + 622 at nsXPConnect.cpp:527
    frame #4: 0x00000001032d2960 XUL`nsXPCComponents_Interfaces::NewResolve(this=0x0000000122bb4d30, wrapper=0x0000000118665340, cx=0x0000000110ca5170, objArg=0x0000000118762ca0, idArg=jsid at 0x00007fff5fbf8280, objp=0x00007fff5fbf8620, _retval=0x00007fff5fbf8633) + 1024 at XPCComponents.cpp:1171

where IID2NativeInterfaceMap::Find() returns a XPCNativeInterface* from its hashtable (presumably because the interface will be in the xpt file too, even with bug 996061 for people's local builds), causing things to go south from that point on.

However it seems like Bobby was right in that the xpc::HasInstance() on trunk works fine for this.  I just had to modify nsJSIID::NewResolve and nsJSIID::Enumerate to pass mInfo directly to XPCNativeInterface::GetNewOrUsed.  With that, things seem to work fine locally.  I pushed what I have so far to try to see how things go: https://tbpl.mozilla.org/?tree=Try&rev=4effe40af364

    
    

    
  
This test failure seems to be interesting: <https://tbpl.mozilla.org/php/getParsedLog.php?id=38591052&tree=Try&full=1#error0>  I'm not sure what that is trying to test and why it fails with my patch.  Ideas welcome.

    
    

    
  
The other interesting test failure is the case where one of these interfaces which I'm making non-scriptable is passed to another method as an argument.  Examples:

<http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/nsISessionStore.idl#84>
<http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsIUpdateService.idl#292>
<http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsIUpdateService.idl#304>

Doing this with my patch results in an error like this: NS_ERROR_FAILURE: Failure arg 0 [nsIUpdateCheckListener.onError]

which seems to stem from here: <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#1048>.  This is basically JSObject2NativeInterface failing when trying to convert the method argument to an nsISupports*.  The call is coming from <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#761>.

Not sure how I should fix this.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP (based on comment 48) (obsolete) (deleted)
        — 
          — Splinter Review
      

    


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

        Attachment #8409806 -
        Attachment is obsolete: true

    
  
Depends on: 1002262

    
    

    
  
Another interesting case similar to comment 54 is <http://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsIGeolocationProvider.idl#28>, which is triggered by <http://mxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#257>.  There we have a JS-implemented nsIDOMGeoPosition object which we can't use to call nsIGeolocationUpdate.update()...

    
    

    
  
...and, my code seems to add a new rooting hazard: <https://tbpl.mozilla.org/php/getParsedLog.php?id=38595206&tree=Try&full=1> but that log doesn't contain any information on what the hazard is.  Steve, any ideas how I can find the rooting hazard that I'm adding so that I can fix it?
Flags: needinfo?(sphink)

    
    

    
  
(Note that the reason that comment 56 did not show up in the try run in comment 52 is that I added the nsIDOMGeoPosition shim after that try push.

    
    

    
  
Comment on attachment 8413437 [details] [diff] [review]
WIP (based on comment 48)

It seems like this is ready for review once we figure out the answers to the questions in the previous comments.  I guess now is a good time to get some general feedback on the patch so that I ca address any large issues while I'm waiting here.

Please feel free to disregard the DEFINE_SHIM list and also the .idl changes for now, although they would require careful review once this is ready.

        Attachment #8413437 -
        Flags: feedback?(bzbarsky)

        Attachment #8413437 -
        Flags: feedback?(bobbyholley)

    
    

    
  
BTW, the following WebIDL interfaces have [ChromeOnly] consts on them:

CanvasRenderingContext2D
* DRAWWINDOW_DRAW_CARET
* DRAWWINDOW_DO_NOT_FLUSH
* DRAWWINDOW_DRAW_VIEW
* DRAWWINDOW_USE_WIDGET_LAYERS
* DRAWWINDOW_ASYNC_DECODE_IMAGES
MozImageLoadingContent
* UNKNOWN_REQUEST
* CURRENT_REQUEST
* PENDING_REQUEST
MozObjectLoadingContent
* TYPE_LOADING
* TYPE_IMAGE
* TYPE_PLUGIN
* TYPE_DOCUMENT
* TYPE_NULL
* PLUGIN_UNSUPPORTED
* PLUGIN_ALTERNATE
* PLUGIN_DISABLED
* PLUGIN_BLOCKLISTED
* PLUGIN_OUTDATED
* PLUGIN_CRASHED
* PLUGIN_SUPPRESSED
* PLUGIN_USER_DISABLED
* PLUGIN_CLICK_TO_PLAY
* PLUGIN_VULNERABLE_UPDATABLE
* PLUGIN_VULNERABLE_NO_UPDATE
* PLUGIN_PLAY_PREVIEW
Selection
* ENDOFPRECEDINGLINE
* STARTOFNEXTLINE

I really can't see a good reason to not support them in this patch, but at the surface some of them seem to be used in a few add-ons: <https://mxr.mozilla.org/addons/search?string=DRAWWINDOW_&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons>, <https://mxr.mozilla.org/addons/search?string=CURRENT_REQUEST&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons>, etc.  But I think Boris suggested somewhere that he hopes to avoid having to do that...

    
    

    
  
> I'm not sure what that is trying to test and why it fails with my patch.

It fails because it calls this:

  gWindowUtils.dispatchDOMEventViaPresShell(obj, obj, false);

which is declared as:

   boolean dispatchDOMEventViaPresShell(in nsIDOMNode aTarget,
                                        in nsIDOMEvent aEvent,
                                        in boolean aTrusted);

but obj is just a vanilla JS object.  So this test is expecting nsIDOMNode to be scriptable and not builtinclass, basically.  We should figure out what this test is really trying to test and fix it.

It's trying to test that QueryInterface is in fact invoked in this situation for a scriptable interface that's implemented by a trusted object.  There's a corresponding test_bug503926.html which tries to make sure it's not invoked for untrusted objects, except that test no longer tests that anymore, since it's not even calling XPConnect APIs nowadays...

We should probably switch both versions of the test to testing some windowutils method (assuming that using SpecialPowers won't confuse the security check added in bug 503926) that we add for this purpose that takes some scriptable non-DOM interface pointer and does nothing with it.

> is passed to another method as an argument

I don't understand why this is being a problem.  You're only changing interfaces that are implemented by WebIDL objects, right?  So we should be landing in XPCConvert::JSObject2NativeInterface, calling GetISupportsFromJSObject, getting an nsISupports that way, and calling QI on it.  We should not be reaching line 1048 at all.

Are we somehow ending up with the wrong iid there or something?

> There we have a JS-implemented nsIDOMGeoPosition object

If we want to support that, then nsIDOMGeoPosition needs to be scriptable, no?

Are the failures mentioned in comment 54 due to people passing random JS objects to arguments that expect nsIDOMNode and nsIXMLHttpRequest???

> ...and, my code seems to add a new rooting hazard:

The hazard is in ShimInterfaceInfo::ResolveConstants.  The isEnabled indirect call could well GC for all the static analysis knows, but "obj" is a JSObject*, not a HandleObject.  

You should make that argument a HandleObject, and same up the callstack as needed.

I do wish the rooting job actually listed the failures somewhere.  :(

> But I think Boris suggested somewhere that he hopes to avoid having to do that

Which "that" are we talking about?

    
    

    
  
(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #61)
> > I'm not sure what that is trying to test and why it fails with my patch.
> 
> It fails because it calls this:
> 
>   gWindowUtils.dispatchDOMEventViaPresShell(obj, obj, false);
> 
> which is declared as:
> 
>    boolean dispatchDOMEventViaPresShell(in nsIDOMNode aTarget,
>                                         in nsIDOMEvent aEvent,
>                                         in boolean aTrusted);
> 
> but obj is just a vanilla JS object.  So this test is expecting nsIDOMNode
> to be scriptable and not builtinclass, basically.  We should figure out what
> this test is really trying to test and fix it.

Any suggestions?  I'm not really familiar with the wrappers magic that is going on here.

> It's trying to test that QueryInterface is in fact invoked in this situation
> for a scriptable interface that's implemented by a trusted object.  There's
> a corresponding test_bug503926.html which tries to make sure it's not
> invoked for untrusted objects, except that test no longer tests that
> anymore, since it's not even calling XPConnect APIs nowadays...
> 
> We should probably switch both versions of the test to testing some
> windowutils method (assuming that using SpecialPowers won't confuse the
> security check added in bug 503926) that we add for this purpose that takes
> some scriptable non-DOM interface pointer and does nothing with it.

Can you please clarify what you mean here?  Doesn't the convertion of the vanilla JS object to nsIDOMNode need to happen similarly when calling an nsIDOMWindowUtils method?

> > is passed to another method as an argument
> 
> I don't understand why this is being a problem.  You're only changing
> interfaces that are implemented by WebIDL objects, right?  So we should be
> landing in XPCConvert::JSObject2NativeInterface, calling
> GetISupportsFromJSObject, getting an nsISupports that way, and calling QI on
> it.  We should not be reaching line 1048 at all.

Ah, hmm.  So actually, looking at nsIDOMGeoPosition's implmentation in the tree <http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/nsGeoPosition.h#46>, it seems to be contained in the WebIDL "wrapper" implementation: <http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/nsGeoPosition.h#104>.  So, here for example, wouldn't we end up calling mozilla::dom::Position::QueryInterface with IID_nsIDOMGeoPosition?  That would obviously fail.  If that's the case then we obviously can't shim the geolocation interfaces...

But I guess that theory doesn't explain the nsIDOMNode case.  In that case the the QueryInterface method of the concrete C++ class should be called which already knows how to handle the nsIDOMNode interface...

> Are we somehow ending up with the wrong iid there or something?

No, IIRC I checked that.

> > There we have a JS-implemented nsIDOMGeoPosition object
> 
> If we want to support that, then nsIDOMGeoPosition needs to be scriptable,
> no?

Well, yeah, but do we want to do that?

> Are the failures mentioned in comment 54 due to people passing random JS
> objects to arguments that expect nsIDOMNode and nsIXMLHttpRequest???

Hmm, looks like it...  There is also the accessible/tests/mochitest/table/test_sels_listbox.xul failure but I have no idea what that test does and was hoping that fixing the other issues, those would go away as well.

> > ...and, my code seems to add a new rooting hazard:
> 
> The hazard is in ShimInterfaceInfo::ResolveConstants.  The isEnabled
> indirect call could well GC for all the static analysis knows, but "obj" is
> a JSObject*, not a HandleObject.  
> 
> You should make that argument a HandleObject, and same up the callstack as
> needed.

Ah interesting!  How did you find this?

> I do wish the rooting job actually listed the failures somewhere.  :(

Is there a bug on file for that?  Should be really easy to just cat the resulting file in a script or something.

> > But I think Boris suggested somewhere that he hopes to avoid having to do that
> 
> Which "that" are we talking about?

"having to handle [ChromeOnly] consts".  IIRC this was when we were talking abut comment 31 on IRC.

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #62)
> > > is passed to another method as an argument
> > 
> > I don't understand why this is being a problem.  You're only changing
> > interfaces that are implemented by WebIDL objects, right?  So we should be
> > landing in XPCConvert::JSObject2NativeInterface, calling
> > GetISupportsFromJSObject, getting an nsISupports that way, and calling QI on
> > it.  We should not be reaching line 1048 at all.
> 
> Ah, hmm.  So actually, looking at nsIDOMGeoPosition's implmentation in the
> tree
> <http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/
> nsGeoPosition.h#46>, it seems to be contained in the WebIDL "wrapper"
> implementation:
> <http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/
> nsGeoPosition.h#104>.  So, here for example, wouldn't we end up calling
> mozilla::dom::Position::QueryInterface with IID_nsIDOMGeoPosition?  That
> would obviously fail.  If that's the case then we obviously can't shim the
> geolocation interfaces...
> 
> But I guess that theory doesn't explain the nsIDOMNode case.  In that case
> the the QueryInterface method of the concrete C++ class should be called
> which already knows how to handle the nsIDOMNode interface...

Actually, forget about nsIDOMNode, there we're really passing an empty JS object and this should fail, we just need to remove the existing manual check to enforce this.  For the updater test failures for example in https://tbpl.mozilla.org/php/getParsedLog.php?id=38595785&tree=Try&full=1#error2, there are so many layers of indirection that my head hurts.  I can't confidently figure out what's going on there right now.

    
    

    
  
> We should figure out what this test is really trying to test and fix it.

Er, I meant to delete this sentence but forgot to, since I'd figured out what it was testing....

> Doesn't the convertion of the vanilla JS object to nsIDOMNode need to happen similarly
> when calling an nsIDOMWindowUtils method?

That conversion is only possible for scriptable interfaces.  We're trying to remove the ability to do that conversion.  ;)

But this test doesn't care about nsIDOMNode or the nsIDOMWindowUtils dispatchDOMEventViaPresShell method.  It just wants some XPConnect method that takes some scriptable interface argument.  And ideally it would be the same method in both test_bug503926.html and test_bug503926.xul, but just with different caller permissions: the pair of tests is trying to make sure the QueryInterface property of the passed-in object is called in the trusted-caller case but not the untrusted-caller case.

Hence my proposal to add a method explicitly for testing this to nsIDOMWindowUtils, since that's an XPConnect object (and ideally add a test that it really is an XPConnect object), and make that method take some scriptable interface.  nsIDOMWindowUtils would be a good candidate for the argument type.  ;)

> So actually, looking at nsIDOMGeoPosition's implmentation in the tree 

Like I said, nsIDOMGeoPosition has to remain scriptable, since we implement it with raw JSObjects in the NetworkGeolocationProvider.js code and hence have to be able to create an XPCWrappedJS for it.  There's no dom::Position involved at all; the caller is just passing in a vanilla JS object with no associated C++ object.

> But I guess that theory doesn't explain the nsIDOMNode case.

Indeed, unless some bozo is passing something that's not actually a node.

A JS stack for when you hit that failure case in JSObject2NativeInterface would be somewhat helpful...

> Well, yeah, but do we want to do that?

Yes, unless we rewrite NetworkGeolocationProvider.js somehow.  We're actually using its scriptability!

> Hmm, looks like it...

Well, we should find whoever is doing that, retroactively r-, and fix their broken junk... :(

> Ah interesting!  How did you find this?

I read your patch, looking for places where there are raw JSObject* (as opposed to Handle or Rooted) around, and then looked at the code that gets called while those are live.

I _could_ be wrong, note.  But I'm about 99% sure I'm right  ;)

> Is there a bug on file for that?

I don't know.

> "having to handle [ChromeOnly] consts". 

Oh, I see.  The point is that the code in comment 31 doesn't define them.  I think it should be fine to do a "is this chrome?" check and if so define the chromeonly ones, yes.  Or can we even just assume we're chrome at that point in the code and skip the check?

> there are so many layers of indirection that my head hurts.

Sounds like a lot of JS code I've seen.

If you can reproduce, a "call DumpJSStack()" (or Windows debugger equivalent) at the point when JSObject2NativeInterface fails would be a good start at figuring out what's going on, maybe.  Depending on how much async stuff is going on there.  :(

    
    

    
  
(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #64)
> > Doesn't the convertion of the vanilla JS object to nsIDOMNode need to happen similarly
> > when calling an nsIDOMWindowUtils method?
> 
> That conversion is only possible for scriptable interfaces.  We're trying to
> remove the ability to do that conversion.  ;)
> 
> But this test doesn't care about nsIDOMNode or the nsIDOMWindowUtils
> dispatchDOMEventViaPresShell method.  It just wants some XPConnect method
> that takes some scriptable interface argument.  And ideally it would be the
> same method in both test_bug503926.html and test_bug503926.xul, but just
> with different caller permissions: the pair of tests is trying to make sure
> the QueryInterface property of the passed-in object is called in the
> trusted-caller case but not the untrusted-caller case.
> 
> Hence my proposal to add a method explicitly for testing this to
> nsIDOMWindowUtils, since that's an XPConnect object (and ideally add a test
> that it really is an XPConnect object), and make that method take some
> scriptable interface.  nsIDOMWindowUtils would be a good candidate for the
> argument type.  ;)

OK, understood now!

> > So actually, looking at nsIDOMGeoPosition's implmentation in the tree 
> 
> Like I said, nsIDOMGeoPosition has to remain scriptable, since we implement
> it with raw JSObjects in the NetworkGeolocationProvider.js code and hence
> have to be able to create an XPCWrappedJS for it.  There's no dom::Position
> involved at all; the caller is just passing in a vanilla JS object with no
> associated C++ object.

Right, ok.

> > But I guess that theory doesn't explain the nsIDOMNode case.
> 
> Indeed, unless some bozo is passing something that's not actually a node.
> 
> A JS stack for when you hit that failure case in JSObject2NativeInterface
> would be somewhat helpful...

I'll fix the existing problems and report to try.  Will post a stack if this happens again.

> > Well, yeah, but do we want to do that?
> 
> Yes, unless we rewrite NetworkGeolocationProvider.js somehow.  We're
> actually using its scriptability!

That's true.

> > Ah interesting!  How did you find this?
> 
> I read your patch, looking for places where there are raw JSObject* (as
> opposed to Handle or Rooted) around, and then looked at the code that gets
> called while those are live.
> 
> I _could_ be wrong, note.  But I'm about 99% sure I'm right  ;)

I'll test on try.

> > Is there a bug on file for that?
> 
> I don't know.
> 
> > "having to handle [ChromeOnly] consts". 
> 
> Oh, I see.  The point is that the code in comment 31 doesn't define them.  I
> think it should be fine to do a "is this chrome?" check and if so define the
> chromeonly ones, yes.  Or can we even just assume we're chrome at that point
> in the code and skip the check?

Yes, for the non-chrome case we create a dummy Components object.  :-)

> > there are so many layers of indirection that my head hurts.
> 
> Sounds like a lot of JS code I've seen.
> 
> If you can reproduce, a "call DumpJSStack()" (or Windows debugger
> equivalent) at the point when JSObject2NativeInterface fails would be a good
> start at figuring out what's going on, maybe.  Depending on how much async
> stuff is going on there.  :(

OK, will try to do that at some point next week.

    
  
Depends on: 1002313

    
    

    
  
BTW, is this something that might break something in comm-central?  I can't think of why it might immediately...

    
    

    
  
> BTW, is this something that might break something in comm-central? 

Hrm.  Bobby, I seem to recall they had some sort of fake element thing in one of the profile importers or something.  Was that JS or C++?
Flags: needinfo?(bobbyholley)

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #57)
> ...and, my code seems to add a new rooting hazard:
> <https://tbpl.mozilla.org/php/getParsedLog.php?id=38595206&tree=Try&full=1>
> but that log doesn't contain any information on what the hazard is.  Steve,
> any ideas how I can find the rooting hazard that I'm adding so that I can
> fix it?

On <https://tbpl.mozilla.org/?rev=d6fa0df57441&tree=Try>, click the "Hf", and on the right side of the HUD you'll see "TinderboxPrint: upload results: complete" where "results" is a link to <https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-d6fa0df57441781457a2fe744acc03c8169cd005/try-linux64-br-haz>. hazards.txt.gz in that directory says

Function '_ZN17ShimInterfaceInfo16ResolveConstantsEP9JSContextP8JSObject|void ShimInterfaceInfo::ResolveConstants(JSContext*, JSObject*)' has unrooted 'obj' of type 'JSObject*' live across GC call '_ZNK7mozilla3dom8PrefableIKNS0_12ConstantSpecEE9isEnabledEP9JSContextP8JSObject|bool mozilla::dom::Prefable<T>::isEnabled(JSContext*, JSObject*) const [with T = const mozilla::dom::ConstantSpec]' at js/xpconnect/src/ShimInterfaceInfo.cpp:695



(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #68)
> > BTW, is this something that might break something in comm-central? 
> 
> Hrm.  Bobby, I seem to recall they had some sort of fake element thing in
> one of the profile importers or something.  Was that JS or C++?

That was C++. (I think it might have been removed already.)

    
  
Depends on: 1002467

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #65)
> > > But I guess that theory doesn't explain the nsIDOMNode case.
> > 
> > Indeed, unless some bozo is passing something that's not actually a node.
> > 
> > A JS stack for when you hit that failure case in JSObject2NativeInterface
> > would be somewhat helpful...
> 
> I'll fix the existing problems and report to try.  Will post a stack if this
> happens again.

OK, this is the stack from the updater test failures:

 1:33.01 System JS : WARNING file:///Users/ehsan/moz/src/obj-ff-dbg.noindex/dist/bin/components/nsUpdateService.js:3125 - reference to undefined property e.result
 1:51.52 0 UC_onLoad(event = [object Object]) ["file:///Users/ehsan/moz/src/obj-ff-dbg.noindex/dist/bin/components/nsUpdateService.js":3129]
 1:51.52     this = [object Object]
 1:51.52 1 anonymous(event = [object Object]) ["file:///Users/ehsan/moz/src/obj-ff-dbg.noindex/dist/bin/components/nsUpdateService.js":3012]
 1:51.52     this = [object Object]
 1:51.52 2 callHandleEvent() ["/Users/ehsan/moz/src/obj-ff-dbg.noindex/_tests/xpcshell/toolkit/mozapps/update/tests/unit_aus_update/urlConstruction.js":39]
 1:51.52     this = [object BackstagePass @ 0x111e580e0 (native @ 0x111e2e1b0)]
 1:51.52 3 anonymous() ["/Users/ehsan/moz/src/testing/xpcshell/head.js":554]
 1:51.52     this = [object Object]

This happens during the onload handler here: <http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#3629> where we end up getting to this point <http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#3727> and have onCheckComplete throw because it doesn't know how to deal with converting the event.target object.  Note that the XHR object created here is actually an XPCOM object: <http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#3603> so I think this is not very surprising.

Now one solution would be to use Cu.importGlobalProperties(["XMLHttpRequest"]) and the WebIDL object in the service, but the updater tests rely on being able to override the XHR service, which I think suggests that we need to un-shim nsIXMLHttpRequest.  What do you think?
Flags: needinfo?(bzbarsky)

    
    

    
  
(In reply to :Ms2ger from comment #69)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #57)
> > ...and, my code seems to add a new rooting hazard:
> > <https://tbpl.mozilla.org/php/getParsedLog.php?id=38595206&tree=Try&full=1>
> > but that log doesn't contain any information on what the hazard is.  Steve,
> > any ideas how I can find the rooting hazard that I'm adding so that I can
> > fix it?
> 
> On <https://tbpl.mozilla.org/?rev=d6fa0df57441&tree=Try>, click the "Hf",
> and on the right side of the HUD you'll see "TinderboxPrint: upload results:
> complete" where "results" is a link to
> <https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/
> eakhgari@mozilla.com-d6fa0df57441781457a2fe744acc03c8169cd005/try-linux64-br-
> haz>. 

Thank you Ms2ger. The "official" source for this info, admittedly difficult to find, is <https://wiki.mozilla.org/Javascript:SpiderMonkey:ExactStackRooting>. I thought I had a link to there in the tbpl help pane, but I'm not seeing it.
Flags: needinfo?(sphink)

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #70)
> Now one solution would be to use
> Cu.importGlobalProperties(["XMLHttpRequest"]) and the WebIDL object in the
> service, but the updater tests rely on being able to override the XHR
> service, which I think suggests that we need to un-shim nsIXMLHttpRequest. 
> What do you think?

I convinced myself that this is the right thing to do, and it does fix the updater tests.

    
    

    
  
The story of the a11y test failures turned out to be extremely interesting!  I ended up having to bisect the list of all of the interfaces I had made non-scriptable here to see which one is at fault, and narrowed it down to nsIDOMNodeList.  That was really puzzling, since nsIDOMNodeList is not an argument to any method, was not implemented by anything in accessible/ and was not even used in any of the test code involved.  So I desperately started to set breakpoints on the four locations where nsIDOMNodeList was used by C++, and I found that this call is failing: <http://mxr.mozilla.org/mozilla-central/source/accessible/src/xul/XULListboxAccessible.cpp#451>.  This method is implemented in XBL: <http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/listbox.xml#177> as returning a JS array.  That will behave like an nsIDOMNodeList but cannot be converted back to one.

Therefore, nsIDOMList must be unshimed as well, and with that, I think I have a patch ready for review.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch (v1) (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
I meant to push this to try except that I can't, because of bug 994028.  I'll do that later, but this should be green based on my local tests.

        Attachment #8413437 -
        Attachment is obsolete: true

        Attachment #8413437 -
        Flags: feedback?(bzbarsky)

        Attachment #8413437 -
        Flags: feedback?(bobbyholley)

        Attachment #8414195 -
        Flags: review?(bzbarsky)

        Attachment #8414195 -
        Flags: review?(bobbyholley)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bobbyholley)

    
    

    
  
> Note that the XHR object created here is actually an XPCOM object:

I'd think it's still a WebIDL object.  The nsXMLHttpRequest ctor calls SetIsDOMBinding(), so we should be creating a WebIDL reflection for it here...

Or is the point that this test actually overrides this contract to create something other than nsXMLHttpRequest?  If so, I guess we have no choice but to mark the XHR interfaces scriptable again.  :(

> That will behave like an nsIDOMNodeList but cannot be converted back to one.

Gag me with a spoon.  I wonder what makes .item() work.  Anyway, I agree we should just unshim nsIDOMNodeList for now, and cry.  And maybe take a few drinks.

    
    

    
  
(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #75)
> > Note that the XHR object created here is actually an XPCOM object:
> 
> I'd think it's still a WebIDL object.  The nsXMLHttpRequest ctor calls
> SetIsDOMBinding(), so we should be creating a WebIDL reflection for it
> here...
> 
> Or is the point that this test actually overrides this contract to create
> something other than nsXMLHttpRequest?  If so, I guess we have no choice but
> to mark the XHR interfaces scriptable again.  :(

It's the latter as far as I can tell.

> > That will behave like an nsIDOMNodeList but cannot be converted back to one.
> 
> Gag me with a spoon.  I wonder what makes .item() work.

I assume the fact that nobody calls it?  Didn't really want to bother and check, but let me know if you want me to.

> Anyway, I agree we
> should just unshim nsIDOMNodeList for now, and cry.  And maybe take a few
> drinks.

By the time I finish this bug, I'll need one.  ;-)

    
    

    
  
> I assume the fact that nobody calls it?

It's called in XULListboxAccessible::SelectedRowIndices as far as I can tell, assuming selectedItems is the array in question.  Is that code just not reached?

    
    

    
  
(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #78)
> > I assume the fact that nobody calls it?
> 
> It's called in XULListboxAccessible::SelectedRowIndices as far as I can
> tell, assuming selectedItems is the array in question.  Is that code just
> not reached?

It fails in this branch in nsXPCWrappedJSClass::CallMethod unsurprisingly: <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedJSClass.cpp#1274> raising the following exception: JavaScript component does not have a method named: "item".  XULListboxAccessible::SelectedRowIndices silently ignores this failure.  I was going to file a bug about that, but then I found bug 120684.  We've only known about this for 12 years.  ;-)

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #77)
> https://tbpl.mozilla.org/?tree=Try&rev=e3927711bc02

I forgot an #include, this is a try run for the rest of the platforms which didn't build before: <https://tbpl.mozilla.org/?tree=Try&rev=b16f346cd09f>.  Please ignore the test_bug503926.xul failures there, I forgot to push to try on top of my inbound clone which had the fix for bug 1002313.

    
    

    
  
So wait.  Does this mean that XULListboxAccessible::SelectedRowIndices always returns an array full of zeros?  But the test seems to check the returned values...  What the heck is going on there?

    
    

    
  
(In reply to Boris Zbarsky [:bz] from comment #81)
> So wait.  Does this mean that XULListboxAccessible::SelectedRowIndices
> always returns an array full of zeros?

Yes.

> But the test seems to check the
> returned values...  What the heck is going on there?

See bug 120684 comment 18.  The test is just buggy.  But we don't need to be concerned about that here anyways, I just made nsIDOMNodeList scriptable again.

    
    

    
  
Ms2ger answered the NI in comment 69.

    
    

    
  
Comment on attachment 8414195 [details] [diff] [review]
Patch (v1)

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

I didn't look at all the changes to nsIDOMFoo.idl, and don't have anything insightful to say about the SessionStore.jsm patches.

r- because of the reworking discussed below, but this is looking good in general. Thanks a ton for doing this!

::: js/xpconnect/src/ShimInterfaceInfo.cpp
@@ +336,5 @@
> +#include "mozilla/dom/XMLSerializerBinding.h"
> +#include "mozilla/dom/XPathEvaluatorBinding.h"
> +#include "mozilla/dom/XULCommandEventBinding.h"
> +#include "mozilla/dom/XULDocumentBinding.h"
> +#include "mozilla/dom/XULElementBinding.h"

This include list is huge. Can we put this file in xpcom/reflect/xptinfo/src so that I don't have to build it 30 times per day from now 'til infinity?

@@ +352,5 @@
> +     mozilla::dom::domName ## Binding::sNativePropertyHooks }
> +
> +const ComponentsInterfaceShimEntry kComponentsInterfaceShimMap[] =
> +{
> +  DEFINE_SHIM(nsIDOMAnimationEvent, AnimationEvent),

Do we have any instances where we can't construct the first parameter from the second other than nsISelection? Can we make a special DEFINE_SHIM_WITH_CUSTOM_INTERFACE_NAME or something?

Furthermore, I think it would make sense to automatically generate the shim definitions and the #includes from the same list. But I could be convinced that this is too onerous.

@@ +634,5 @@
> +    return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +void
> +ShimInterfaceInfo::ResolveConstant(const mozilla::dom::ConstantSpec* cs, ResolvedConstant* aConstant)

Per IRC discussion, I believe you can just add a jsval to the union in XPTConstValue, set the type descriptor appropriately with TD_JSVAL, and kill all of this stuff. \o/

::: js/xpconnect/src/ShimInterfaceInfo.h
@@ +30,5 @@
> +    NS_DECL_ISUPPORTS
> +    NS_DECL_NSIINTERFACEINFO
> +
> +    static already_AddRefed<ShimInterfaceInfo>
> +    Construct(const char* aName, JSContext* cx, JS::Handle<JSObject*> obj);

Let's call this MaybeConstruct or something else to indicate that this thing only constructs stuff we shim (and add a comment to that effect here as well).

        Attachment #8414195 -
        Flags: review?(bobbyholley) → review-

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #84)
> ::: js/xpconnect/src/ShimInterfaceInfo.cpp
> @@ +336,5 @@
> > +#include "mozilla/dom/XMLSerializerBinding.h"
> > +#include "mozilla/dom/XPathEvaluatorBinding.h"
> > +#include "mozilla/dom/XULCommandEventBinding.h"
> > +#include "mozilla/dom/XULDocumentBinding.h"
> > +#include "mozilla/dom/XULElementBinding.h"
> 
> This include list is huge. Can we put this file in xpcom/reflect/xptinfo/src
> so that I don't have to build it 30 times per day from now 'til infinity?

I wasn't joking on IRC, the build time for this is quite negligible.  I feel it's very odd for xpcom/reflect/xptinfo to depend on dom/...  But OK.

> @@ +352,5 @@
> > +     mozilla::dom::domName ## Binding::sNativePropertyHooks }
> > +
> > +const ComponentsInterfaceShimEntry kComponentsInterfaceShimMap[] =
> > +{
> > +  DEFINE_SHIM(nsIDOMAnimationEvent, AnimationEvent),
> 
> Do we have any instances where we can't construct the first parameter from
> the second other than nsISelection? Can we make a special
> DEFINE_SHIM_WITH_CUSTOM_INTERFACE_NAME or something?

OK.

> Furthermore, I think it would make sense to automatically generate the shim
> definitions and the #includes from the same list. But I could be convinced
> that this is too onerous.

You cannot generate preprocessor macros from macro expansions.  But yeah, I would love to do that if that were possible.  (FWIW all of these are hand-generated!)

> @@ +634,5 @@
> > +    return NS_ERROR_NOT_IMPLEMENTED;
> > +}
> > +
> > +void
> > +ShimInterfaceInfo::ResolveConstant(const mozilla::dom::ConstantSpec* cs, ResolvedConstant* aConstant)
> 
> Per IRC discussion, I believe you can just add a jsval to the union in
> XPTConstValue, set the type descriptor appropriately with TD_JSVAL, and kill
> all of this stuff. \o/

Wait, I didn't agree to kill all of this stuff, unless you only mean the arena allocation.  You still need to figure out a solution for isEnabled not being idempotent if you want to convince me to remove it all!

> ::: js/xpconnect/src/ShimInterfaceInfo.h
> @@ +30,5 @@
> > +    NS_DECL_ISUPPORTS
> > +    NS_DECL_NSIINTERFACEINFO
> > +
> > +    static already_AddRefed<ShimInterfaceInfo>
> > +    Construct(const char* aName, JSContext* cx, JS::Handle<JSObject*> obj);
> 
> Let's call this MaybeConstruct or something else to indicate that this thing
> only constructs stuff we shim (and add a comment to that effect here as
> well).

Sure, sounds good.

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #85)
> > @@ +634,5 @@
> > > +    return NS_ERROR_NOT_IMPLEMENTED;
> > > +}
> > > +
> > > +void
> > > +ShimInterfaceInfo::ResolveConstant(const mozilla::dom::ConstantSpec* cs, ResolvedConstant* aConstant)
> > 
> > Per IRC discussion, I believe you can just add a jsval to the union in
> > XPTConstValue, set the type descriptor appropriately with TD_JSVAL, and kill
> > all of this stuff. \o/
> 
> Wait, I didn't agree to kill all of this stuff, unless you only mean the
> arena allocation.  You still need to figure out a solution for isEnabled not
> being idempotent if you want to convince me to remove it all!

Actually, xpt_struct.h is a C header, and `jsval' is a type defined by C++, so I'm not sure how you want me to add a jsval to that union.
Flags: needinfo?(bobbyholley)

    
    

    
  
(I mean, I could do a hack based on #ifdef __cplusplus, but is that a good idea?  I have no strong opinion either way.)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch (v2) (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Other comments addressed.  I will ask again for Bobby's review once we settle comment 86.

        Attachment #8414195 -
        Attachment is obsolete: true

        Attachment #8414195 -
        Flags: review?(bzbarsky)

        Attachment #8415864 -
        Flags: review?(bzbarsky)

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #85)
> I wasn't joking on IRC, the build time for this is quite negligible.  I feel
> it's very odd for xpcom/reflect/xptinfo to depend on dom/...  But OK.

Oh, I missed that on IRC. But I think xptinfo is actually the right place for this. xpcom/base depends on dom/ at this point anyway.

> You cannot generate preprocessor macros from macro expansions.

Ah, right.

> > > +void
> > > +ShimInterfaceInfo::ResolveConstant(const mozilla::dom::ConstantSpec* cs, ResolvedConstant* aConstant)
> > 
> > Per IRC discussion, I believe you can just add a jsval to the union in
> > XPTConstValue, set the type descriptor appropriately with TD_JSVAL, and kill
> > all of this stuff. \o/
> 
> Wait, I didn't agree to kill all of this stuff, unless you only mean the
> arena allocation.

I meant that the body of ResolveConstant can go away (including the non-string stuff), because we can just pass the jsval directly. But I guess _that_ means that, with the current setup, we'd need a bunch of heap-allocated jsvals, which isn't great.

As discussed on IRC, I think we should just stop calling IsEnabled for these things and be done with it. Let's check with bz about that.
 
> Sure, sounds good.

Oh, and one more thing - you don't need to pass |obj| everywhere, and can just use JS::CurrentGlobalOrNull(cx) for the IsEnabled() call. As it stands, it's already a different object than the one the constants are being resolved on.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #86)
> Actually, xpt_struct.h is a C header, and `jsval' is a type defined by C++,
> so I'm not sure how you want me to add a jsval to that union.

We've already jumped the shark on this in xptcall.h. Just assume that it's being compiled with a C++ compiler.
Flags: needinfo?(bobbyholley)

    
    

    
  
(In reply to comment #89)
> > > > +void
> > > > +ShimInterfaceInfo::ResolveConstant(const mozilla::dom::ConstantSpec* cs, ResolvedConstant* aConstant)
> > > 
> > > Per IRC discussion, I believe you can just add a jsval to the union in
> > > XPTConstValue, set the type descriptor appropriately with TD_JSVAL, and kill
> > > all of this stuff. \o/
> > 
> > Wait, I didn't agree to kill all of this stuff, unless you only mean the
> > arena allocation.
> 
> I meant that the body of ResolveConstant can go away (including the non-string
> stuff), because we can just pass the jsval directly. But I guess _that_ means
> that, with the current setup, we'd need a bunch of heap-allocated jsvals, which
> isn't great.
> 
> As discussed on IRC, I think we should just stop calling IsEnabled for these
> things and be done with it. Let's check with bz about that.

We talked to bz about that on IRC, and decided to stop calling isEnabled here.

> > Sure, sounds good.
> 
> Oh, and one more thing - you don't need to pass |obj| everywhere, and can just
> use JS::CurrentGlobalOrNull(cx) for the IsEnabled() call. As it stands, it's
> already a different object than the one the constants are being resolved on.

This is moot given the above, right?

> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #86)
> > Actually, xpt_struct.h is a C header, and `jsval' is a type defined by C++,
> > so I'm not sure how you want me to add a jsval to that union.
> 
> We've already jumped the shark on this in xptcall.h. Just assume that it's
> being compiled with a C++ compiler.

We discussed this on IRC too, and I decided to use #ifdef __cplusplus for this.

    
    

    
  
Ah, I remembered one other reason why I did the array approach for ShimInterfaceInfo.  GetConstant() returns a const nsXPTConstant*, not an nsXPTConstant object itself, which means that I need to return the address of something which at least survives past the GetConstant() function itself.  Looking at the three existing consumers of this function: <http://mxr.mozilla.org/mozilla-central/ident?i=GetConstant&tree=mozilla-central&filter=&strict=1> none of them use the returned pointer immediately after they read some info from it, so for now we can have an nsXPTConstant member in ShimInterfaceInfo, and each time write our constant into it and return its address.  But of course this will horribly break if any of these implicit assumptions break at some point. :(

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch (v3) (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #8416523 -
        Flags: review?(bzbarsky)

        Attachment #8416523 -
        Flags: review?(bobbyholley)

    
  

        Attachment #8415864 -
        Attachment is obsolete: true

        Attachment #8415864 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 8416523 [details] [diff] [review]
Patch (v3)

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

Again, my review does not include the SessionStore.jsm stuff.

r=bholley with the below issues fixed. Thanks again for doing this!

::: xpcom/reflect/xptinfo/src/ShimInterfaceInfo.cpp
@@ +671,5 @@
> +                    if (index == aIndex) {
> +                        // This is a hack.  We're assuming that the callers do
> +                        // not expect these nsXPTConstant* pointers we return
> +                        // to be unique, and are not saving the address
> +                        // between calls to GetConstant.

I could maybe live with this, but it's kind of scary that we have to leave an unrooted jsval sitting on the heap. Can't we just alter the signature of nsIInterfaceInfo::GetConstant to have the caller pass in a single pointer, and the callee fill it? That seems like a much more sensible option.

::: xpcom/reflect/xptinfo/src/ShimInterfaceInfo.h
@@ +32,5 @@
> +
> +    // Construct a ShimInterfaceInfo object if we have a shim available for aName.
> +    // Otherwise, returns nullptr.
> +    static already_AddRefed<ShimInterfaceInfo>
> +    MaybeConstruct(const char* aName, JSContext* cx, JS::Handle<JSObject*> obj);

We don't need the |obj| param anymore, right?

        Attachment #8416523 -
        Flags: review?(bobbyholley) → review+

    
    

    
  
Some more wonderful news here.  I was trying to figure out why this crashes all over the place on Linux32 <>.  Turns out that on Linux32, sizeof(XPTConstDescriptor) would be 24 in C++ but 20 in C, which means that we allocate the array here <http://mxr.mozilla.org/mozilla-central/source/xpcom/typelib/xpt/src/xpt_struct.c#623> with the size 20 for each object and later on access it from <http://mxr.mozilla.org/mozilla-central/source/xpcom/reflect/xptinfo/src/xptiInterfaceInfo.cpp#256> with a size of 24, which means on the second iteration of the loop we read into the guts of the second element in the array.  Now, on to see if I can make this code compile in C++... :/

    
  
Depends on: 1005321

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #94)
> Some more wonderful news here.  I was trying to figure out why this crashes
> all over the place on Linux32 <>.

This was meant to link to https://tbpl.mozilla.org/?tree=Try&rev=c34b41d444a8

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #93)
> ::: xpcom/reflect/xptinfo/src/ShimInterfaceInfo.cpp
> @@ +671,5 @@
> > +                    if (index == aIndex) {
> > +                        // This is a hack.  We're assuming that the callers do
> > +                        // not expect these nsXPTConstant* pointers we return
> > +                        // to be unique, and are not saving the address
> > +                        // between calls to GetConstant.
> 
> I could maybe live with this, but it's kind of scary that we have to leave
> an unrooted jsval sitting on the heap. Can't we just alter the signature of
> nsIInterfaceInfo::GetConstant to have the caller pass in a single pointer,
> and the callee fill it? That seems like a much more sensible option.

How do I do this?  The following change makes the method take a const nsXPTConstant*, not an nsXPTConstant*, which is what I need:

diff --git a/xpcom/reflect/xptinfo/public/nsIInterfaceInfo.idl b/xpcom/reflect/xptinfo/public/nsIInterfaceInfo.idl
index fab99ff..4c565c3 100644
--- a/xpcom/reflect/xptinfo/public/nsIInterfaceInfo.idl
+++ b/xpcom/reflect/xptinfo/public/nsIInterfaceInfo.idl
@@ -11,7 +11,7 @@
 // forward declaration of non-XPCOM types

 [ptr] native nsXPTMethodInfoPtr(nsXPTMethodInfo);
-[ptr] native nsXPTConstantPtr(nsXPTConstant);
+native nsXPTConstant(nsXPTConstant);
 [ptr] native nsXPTParamInfoPtr(nsXPTParamInfo);
       native nsXPTType(nsXPTType);

@@ -56,7 +56,7 @@ interface nsIInterfaceInfo : nsISupports
                        [shared, retval] out nsXPTMethodInfoPtr info);

     void getConstant(in uint16_t index,
-                     [shared, retval] out nsXPTConstantPtr constant);
+                     [shared, retval] out nsXPTConstant constant);


     /**
Flags: needinfo?(bobbyholley)

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #96)
>      void getConstant(in uint16_t index,
> -                     [shared, retval] out nsXPTConstantPtr constant);
> +                     [shared, retval] out nsXPTConstant constant);

You need to remove [shared]. You might as well remove [retval] too, since this function is already inherently noscript on account of the [native] type.
Flags: needinfo?(bobbyholley)

    
    

    
  


  
This is so terrible that I would like you to review it again.  Note what I had to do with the nsXPTConstant ctor for example.

        Attachment #8416755 -
        Flags: review?(bobbyholley)

    
    

    
  
Comment on attachment 8416755 [details] [diff] [review]
Interdiff changing the argument type of nsIInterfaceInfo::GetConstant

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

This doesn't seem terrible at all! r=bholley

        Attachment #8416755 -
        Flags: review?(bobbyholley) → review+

    
    

    
  
So, here's another problem, attachment 8416755 [details] [diff] [review] introduces these rooting hazards:

Function '_ZN15XPCNativeMember7ResolveER14XPCCallContextP18XPCNativeInterfaceN2JS6HandleIP8JSObjectEEPNS4_5ValueE|uint8 XPCNativeMember::Resolve(XPCCallContext*, XPCNativeInterface*, class JS::Handle<JSObject*>, JS::Value*)' has unrooted 'constant' of type 'nsXPTConstant' live across GC call nsIInterfaceInfo.GetConstant at js/xpconnect/src/XPCWrappedNativeInfo.cpp:55
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:55: Call(6,7, __temp_3 := NS_FAILED_impl(__temp_4*))
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:55: Call(7,8, __temp_2 := __builtin_expect((__temp_3* != 0),0))
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:55: Assume(8,10, (__temp_2* != 0), false)
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:58: Call(10,11, mv := constant.GetValue())

Function '_ZN18XPCNativeInterface11NewInstanceEP16nsIInterfaceInfo|XPCNativeInterface* XPCNativeInterface::NewInstance(nsIInterfaceInfo*)' has unrooted 'constant' of type 'nsXPTConstant' live across GC call nsIInterfaceInfo.GetConstant at js/xpconnect/src/XPCWrappedNativeInfo.cpp:314
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:314: Call(4,5, __temp_50 := NS_FAILED_impl(__temp_51*))
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:314: Call(5,6, __temp_49 := __builtin_expect((__temp_50* != 0),0))
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:314: Assume(6,7, (__temp_49* != 0), false)
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:319: Call(7,8, __temp_53 := cx.operator 494())
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:319: Call(8,9, __temp_54 := constant.GetName())

Function '_ZL24DefineInterfaceConstantsP9JSContextN2JS6HandleIP8JSObjectEEPK4nsID|nsDOMClassInfo.cpp:uint32 DefineInterfaceConstants(JSContext*, class JS::Handle<JSObject*>, nsID*)' has unrooted 'c' of type 'nsXPTConstant' live across GC call nsIInterfaceInfo.GetConstant at dom/base/nsDOMClassInfo.cpp:2033
    dom/base/nsDOMClassInfo.cpp:2034: Call(5,6, __temp_25 := NS_FAILED_impl(rv*))
    dom/base/nsDOMClassInfo.cpp:2034: Call(6,7, __temp_24 := __builtin_expect((__temp_25* == 0),1))
    dom/base/nsDOMClassInfo.cpp:2034: Assume(7,8, (__temp_24* == 0), false)
    dom/base/nsDOMClassInfo.cpp:2036: Call(8,9, __temp_26 := c.GetType())

Now, I can't convert XPTConstValue::jsval to be a rooted type since that implies having a non-default constructor, and those types cannot be union members.  Not sure how to fix this.  Any ideas?

    
  
Depends on: 1005486

    
    

    
  
So, the rooting analysis assumes that all calls on interface types can GC, because it can't statically determine that they can't.

But there's a way to tell the rooting analysis not to worry about a given method. sfink, can we do that for nsIInterfaceInfo::GetConstant?

We should probably add [noscript, builtinclass] to nsIInterfaceInfo just for good measure (even though it's already implicitly so by virtue of the non-scriptable native argument types).
Flags: needinfo?(sphink)

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #101)
> So, the rooting analysis assumes that all calls on interface types can GC,
> because it can't statically determine that they can't.
> 
> But there's a way to tell the rooting analysis not to worry about a given
> method. sfink, can we do that for nsIInterfaceInfo::GetConstant?

That would be nice, yes!  FWIW, this patch is fully green except for this one last issue: <https://tbpl.mozilla.org/?tree=Try&rev=bd5c2252e2c8>

> We should probably add [noscript, builtinclass] to nsIInterfaceInfo just for
> good measure (even though it's already implicitly so by virtue of the
> non-scriptable native argument types).

Hmm, nsIInterfaceInfo is not [scriptable], what would that buy us?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch (v4) (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #8416523 -
        Attachment is obsolete: true

        Attachment #8416755 -
        Attachment is obsolete: true

        Attachment #8416523 -
        Flags: review?(bzbarsky)

        Attachment #8416921 -
        Flags: review?(bzbarsky)

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #102)
> Hmm, nsIInterfaceInfo is not [scriptable], what would that buy us?

Er, yeah. Don't mind me.

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #101)
> So, the rooting analysis assumes that all calls on interface types can GC,
> because it can't statically determine that they can't.
> 
> But there's a way to tell the rooting analysis not to worry about a given
> method. sfink, can we do that for nsIInterfaceInfo::GetConstant?

yes

> We should probably add [noscript, builtinclass] to nsIInterfaceInfo just for
> good measure (even though it's already implicitly so by virtue of the
> non-scriptable native argument types).

Yeah, can you explain to me why this is safe? I don't really understand all the possibilities.

Can I annotate this to be safe only when calling through nsIInterfaceInfo? It would then still complain if you called through an interface that inherits from nsIInterfaceInfo, assuming such a thing is possible in the xpcom world.

If I can do that, then this annotation is pretty safe -- if you later added an implementation of the interface that GC'd, then the analysis would still catch it and complain. The only way I know of to make this annotation mask a hazard would be to implement the interface in a binary extension. But for that reason, I like to be conservative with annotations.
Flags: needinfo?(sphink)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Annotate nsIInterfaceInfo.GetConstant (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Can you try pushing to try with this patch included, and let me know if it gets it?

        Attachment #8417491 -
        Flags: review?(terrence)
Assignee: ehsan → sphink
Status: NEW → ASSIGNED

    
    

    
  
(In reply to Steve Fink [:sfink] from comment #105)

> > We should probably add [noscript, builtinclass] to nsIInterfaceInfo just for
> > good measure (even though it's already implicitly so by virtue of the
> > non-scriptable native argument types).
> 
> Yeah, can you explain to me why this is safe? I don't really understand all
> the possibilities.

At present there is only one implementation of nsIInterfaceInfo. Ehsan is adding a second one. But neither of them will GC. And more importantly, the value here is effectively an out-param that gets set at the end of the function.

> Can I annotate this to be safe only when calling through nsIInterfaceInfo?
> It would then still complain if you called through an interface that
> inherits from nsIInterfaceInfo, assuming such a thing is possible in the
> xpcom world.

It's not possible at present per the above, but that sounds fine, yes.
 
> If I can do that, then this annotation is pretty safe -- if you later added
> an implementation of the interface that GC'd, then the analysis would still
> catch it and complain.

I don't understand this. Are you saying that if we added a third implementation that GCed, the analysis would complain? i.e. the analysis is smart enough to check that, if we claim nsIFoo::bar() cannot GC, then any concrete class that inherits nsIFoo should have a non-GCing Bar()? If so, that sounds great to me.

> The only way I know of to make this annotation mask a
> hazard would be to implement the interface in a binary extension. But for
> that reason, I like to be conservative with annotations.

It would only be a problem if that binary extension was _also_ using the jsval member of the union, at which point they'd have to know about rooting anyway.

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #107)
> (In reply to Steve Fink [:sfink] from comment #105)
> 
> > If I can do that, then this annotation is pretty safe -- if you later added
> > an implementation of the interface that GC'd, then the analysis would still
> > catch it and complain.
> 
> I don't understand this. Are you saying that if we added a third
> implementation that GCed, the analysis would complain? i.e. the analysis is
> smart enough to check that, if we claim nsIFoo::bar() cannot GC, then any
> concrete class that inherits nsIFoo should have a non-GCing Bar()? If so,
> that sounds great to me.

Yes. This specific type of annotation is pretty safe -- if you claim that nsIFoo::bar() cannot GC, but you compile an implementation that *does* GC, the analysis will report a hazard. The annotation only says "you may assume that nobody will link in a binary that implements this interface in a way that can GC". All of the code compiled during a regular build will be checked normally, including virtual function resolution.

XPCOM interfaces are by default treated with more suspicion than regular vtable entries. 

We have many other sorts of annotation that are much more dangerous, so don't trust the analysis completely. But it's pretty good.

> > The only way I know of to make this annotation mask a
> > hazard would be to implement the interface in a binary extension. But for
> > that reason, I like to be conservative with annotations.
> 
> It would only be a problem if that binary extension was _also_ using the
> jsval member of the union, at which point they'd have to know about rooting
> anyway.

Sure, they would have to know about it, but that doesn't prevent them from getting it wrong!

    
    

    
  
Comment on attachment 8417491 [details] [diff] [review]
Annotate nsIInterfaceInfo.GetConstant

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

r=me

        Attachment #8417491 -
        Flags: review?(terrence) → review+
Assignee: sphink → ehsan

    
    

    
  
Pushed to try with the rooting analysis fix: https://tbpl.mozilla.org/?tree=Try&rev=8e00a54ba0bd

    
    

    
  
The try run did not prove successful because I forgot to land another patch :(

Trying again: https://tbpl.mozilla.org/?tree=Try&rev=4ba4495cfe06

    
  
No longer depends on: 1005486

    
    

    
  
The analysis still fails in the exact same way:

Function '_ZN15XPCNativeMember7ResolveER14XPCCallContextP18XPCNativeInterfaceN2JS6HandleIP8JSObjectEEPNS4_5ValueE|uint8 XPCNativeMember::Resolve(XPCCallContext*, XPCNativeInterface*, class JS::Handle<JSObject*>, JS::Value*)' has unrooted 'constant' of type 'nsXPTConstant' live across GC call nsIInterfaceInfo.GetConstant at js/xpconnect/src/XPCWrappedNativeInfo.cpp:55
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:55: Call(6,7, __temp_3 := NS_FAILED_impl(__temp_4*))
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:55: Call(7,8, __temp_2 := __builtin_expect((__temp_3* != 0),0))
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:55: Assume(8,10, (__temp_2* != 0), false)
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:58: Call(10,11, mv := constant.GetValue())

Function '_ZN18XPCNativeInterface11NewInstanceEP16nsIInterfaceInfo|XPCNativeInterface* XPCNativeInterface::NewInstance(nsIInterfaceInfo*)' has unrooted 'constant' of type 'nsXPTConstant' live across GC call nsIInterfaceInfo.GetConstant at js/xpconnect/src/XPCWrappedNativeInfo.cpp:314
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:314: Call(4,5, __temp_50 := NS_FAILED_impl(__temp_51*))
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:314: Call(5,6, __temp_49 := __builtin_expect((__temp_50* != 0),0))
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:314: Assume(6,7, (__temp_49* != 0), false)
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:319: Call(7,8, __temp_53 := cx.operator 494())
    js/xpconnect/src/XPCWrappedNativeInfo.cpp:319: Call(8,9, __temp_54 := constant.GetName())

Function '_ZL24DefineInterfaceConstantsP9JSContextN2JS6HandleIP8JSObjectEEPK4nsID|nsDOMClassInfo.cpp:uint32 DefineInterfaceConstants(JSContext*, class JS::Handle<JSObject*>, nsID*)' has unrooted 'c' of type 'nsXPTConstant' live across GC call nsIInterfaceInfo.GetConstant at dom/base/nsDOMClassInfo.cpp:2026
    dom/base/nsDOMClassInfo.cpp:2027: Call(5,6, __temp_25 := NS_FAILED_impl(rv*))
    dom/base/nsDOMClassInfo.cpp:2027: Call(6,7, __temp_24 := __builtin_expect((__temp_25* == 0),1))
    dom/base/nsDOMClassInfo.cpp:2027: Assume(7,8, (__temp_24* == 0), false)
    dom/base/nsDOMClassInfo.cpp:2029: Call(8,9, __temp_26 := c.GetType())

Steve, any ideas why?
Depends on: 1005486
Flags: needinfo?(sphink)

    
  
No longer depends on: 1005486

    
    

    
  
Sorry, it took me a while to get this running on my slave. And slightly longer to discover that I didn't need to in the first place, since the answer is in the files that your build uploaded already.

The problem is that the analysis thinks that nsIInterfaceInfo.GetConstant can GC, so the annotation doesn't do any good. Here's why:

GC Function: nsIInterfaceInfo.GetConstant|nsIInterfaceInfo.GetConstant
    uint32 xptiInterfaceInfo::GetConstant(uint16, nsXPTConstant*)
    uint32 xptiInterfaceEntry::GetConstant(uint16, nsXPTConstant*)
    uint8 xptiInterfaceEntry::EnsureResolved()
    uint8 xptiInterfaceEntry::Resolve()
    mozilla::Mutex* mozilla::XPTInterfaceInfoManager::GetResolveLock()
    mozilla::XPTInterfaceInfoManager* mozilla::XPTInterfaceInfoManager::GetSingleton()
    void mozilla::XPTInterfaceInfoManager::InitMemoryReporter()
    RegisterWeakMemoryReporter
    FieldCall: nsIMemoryReporterManager.RegisterWeakReporter

It really should dump this out in the log now that I've added everything that it needs, but it doesn't, so continuing the manual dump:

GC Function: _ZN23nsMemoryReporterManager20RegisterWeakReporterEP17nsIMemoryReporter|uint32 nsMemoryReporterManager::RegisterWeakReporter(nsIMemoryReporter*)
    uint32 nsMemoryReporterManager::RegisterReporterHelper(nsIMemoryReporter*, uint8, uint8)
    bool nsTHashtable<EntryType>::Contains(nsTHashtable<EntryType>::KeyType) const [with EntryType = nsRefPtrHashKey<nsIMemoryReporter>; nsTHashtable<EntryType>::KeyType = nsIMemoryReporter*]
    EntryType* nsTHashtable<EntryType>::GetEntry(nsTHashtable<EntryType>::KeyType) const [with EntryType = nsRefPtrHashKey<nsIMemoryReporter>; nsTHashtable<EntryType>::KeyType = nsIMemoryReporter*]
    PLDHashEntryHdr* PL_DHashTableOperate(PLDHashTable*, void*, uint32)
    FieldCall: PLDHashTableOps.initEntry

These are potential stacks, by the way -- each line potentially calls the next line. And we end at my old friend PLDHashTableOps.initEntry, which really could GC with the right function pointers, so I don't want to just annotate it away. But RegisterWeakReporter really shouldn't GC, so there should be some safe place to annotate in these stacks. I'll look closer to figure out where.
Flags: needinfo?(sphink)

    
    

    
  
It looks to me like adding an AutoAssertNoGC to RegisterReporterHelper is the best bet. (initEntry cannot GC here because this is an nsTHashtable< nsRefPtrHashKey<nsIMemoryReporter> >. initEntry will only GC if its key type's constructor can GC, and nsRefPtrHashKey<nsIMemoryReporter> does not have a specific specialization so uses the basic pointer-based instantiation that just copies pointers around.)

But the analysis still doesn't believe me, because it's worried about some addon implementing nsIMemoryReporterManager.RegisterWeakReporter with something that can GC. (It's not totally crazy; you can implement memory reporters in JS. But that will only invoke JS when collecting a report. We're just registering the reporter itself here.) So I have to annotate that one too.

\o/ Whoo hoo! And that's the end of it. (These rabbit holes can go deep. Very deep.)

        Attachment #8417491 -
        Attachment is obsolete: true
Assignee: ehsan → sphink

    
    

    
  
Thanks a lot for your help, Steve!  I think we can make the analysis be better here, filed bug 1007092 with an idea!

    
    

    
  
Steve, the analysis is still not happy :(

https://tbpl.mozilla.org/?tree=Try&rev=13cc713be0e9
Flags: needinfo?(sphink)

    
    

    
  
Huh, you're right. I ran the analysis and it claimed that everything was good, but this was the first time I was experimenting with incremental analysis. I guess it doesn't completely work. :-(

Anyway, the next problem is:

GC Function: nsIInterfaceInfo.GetConstant|nsIInterfaceInfo.GetConstant
    uint32 xptiInterfaceInfo::GetConstant(uint16, nsXPTConstant*)
    uint32 xptiInterfaceEntry::GetConstant(uint16, nsXPTConstant*)
    uint8 xptiInterfaceEntry::EnsureResolved()
    uint8 xptiInterfaceEntry::Resolve()
    uint8 xptiInterfaceEntry::ResolveLocked()
    xptiInterfaceEntry* xptiTypelibGuts::GetEntryAt(uint16)
    UserDataType nsBaseHashtable<KeyClass, DataType, UserDataType>::Get(nsBaseHashtable<KeyClass, DataType, UserDataType>::KeyType) const [with KeyClass = nsIDHashKey; DataType = xptiInterfaceEntry*; UserDataType = xptiInterfaceEntry*; nsBaseHashtable<KeyClass, DataType, UserDataType>::KeyType = const nsID&]
    EntryType* nsTHashtable<EntryType>::GetEntry(nsTHashtable<EntryType>::KeyType) const [with EntryType = nsBaseHashtableET<nsIDHashKey, xptiInterfaceEntry*>; nsTHashtable<EntryType>::KeyType = const nsID&]
    PLDHashEntryHdr* PL_DHashTableOperate(PLDHashTable*, void*, uint32)
    FieldCall: PLDHashTableOps.initEntry

Ugh. I'll have to convince *myself* somehow that that one is ok.

        Attachment #8418537 -
        Flags: review?(terrence)

    
    

    
  
Yuckity yuck.

Dump question - Can we just change the signature of GetConstant to use a MutableHandleValue instead of this union thing? All of the consumers I'm finding just convert the result to a jsval anyway. That would solve our rooting analysis woes.

    
    

    
  
Whoa, that's unexpected.

I have a patch that implements a Rooted nsXPTConstant. Seems to work for at least one of the hazards. But it's just piling more craziness on top. If this could all just go away, including

        // XXX Big Hack!                                                                            
        nsXPTCVariant v;
        v.flags = 0;
        v.type = constant.GetType();
        memcpy(&v.val, &mv.val, sizeof(mv.val));

it would be very nice. I'll be polite and not mention

    // XXX this is ugly. But sometimes you gotta do what you gotta do.
    // A reinterpret_cast won't do the trick here. And this plain C cast
    // works correctly and is safe enough.
    // See http://bugzilla.mozilla.org/show_bug.cgi?id=49641
    const nsXPTCMiniVariant* GetValue() const
        {return (nsXPTCMiniVariant*) &value;}
Flags: needinfo?(sphink)

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #119)
> Yuckity yuck.
> 
> Dump question - Can we just change the signature of GetConstant to use a
> MutableHandleValue instead of this union thing? All of the consumers I'm
> finding just convert the result to a jsval anyway. That would solve our
> rooting analysis woes.

xptiInterfaceInfo::GetConstant relies on an array of XPTConstDescriptor's coming from xptiInterfaceEntry, which _seems_ to have to do some things at least with the binary format of xpt files.  To be completely honest I don't think that I will be much interested to keep on doing this surgery on the guts of xptiInterfaceEntry.  I don't know how much more work that is going to be, but this whole thing was supposed to be "easy" (remember comment 42? ;-)

I think the only "easy" option that I'm willing to try at this point is to basically move what happens after <http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#2026> into GetConstant() and make it accept a MutableHandleValue (is that even possible, nsIInterfaceInfo being an XPIDL interface??)

    
    

    
  
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #121)
> xptiInterfaceInfo::GetConstant relies on an array of XPTConstDescriptor's
> coming from xptiInterfaceEntry, which _seems_ to have to do some things at
> least with the binary format of xpt files.

I wasn't suggesting changing this.

> I think the only "easy" option that I'm willing to try at this point is to
> basically move what happens after
> <http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.
> cpp#2026> into GetConstant() and make it accept a MutableHandleValue

Yes, this was what I meant.

(is
> that even possible, nsIInterfaceInfo being an XPIDL interface??)

Yes. You just declare 'out jsval'.

    
    

    
  
OK, I'll try that.

    
    

    
  
Comment on attachment 8416921 [details] [diff] [review]
Patch (v4)

Why the changes in SessionStore.jsm?

>+ShimInterfaceInfo::GetName(char** aName)
>+    *aName = static_cast<char*> (nsMemory::Clone(mName.get(), mName.Length() + 1));

  *aName = ToNewCString(mName);

r=me

        Attachment #8416921 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
(In reply to Boris Zbarsky [:bz] from comment #124)
> Comment on attachment 8416921 [details] [diff] [review]
> Patch (v4)
> 
> Why the changes in SessionStore.jsm?

Comment 54.  SessionStore had to do poor man's type checking before.

    
  
Assignee: sphink → ehsan

    
    

    
  


  
This does fix the rooting hazard.

        Attachment #8418537 -
        Attachment is obsolete: true

        Attachment #8419367 -
        Flags: review?(bobbyholley)

    
    

    
  
Comment on attachment 8419367 [details] [diff] [review]
Make GetConstant accept a MutableHandleValue

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

::: xpcom/reflect/xptinfo/public/nsIInterfaceInfo.idl
@@ -26,5 @@
>  %}
>  
>  /* this is NOT intended to be scriptable */
>  [uuid(1affa260-8965-4612-869a-78af4ccfb287)]
>  interface nsIInterfaceInfo : nsISupports

Need to rev the uuid.

::: xpcom/reflect/xptinfo/src/ShimInterfaceInfo.cpp
@@ +668,5 @@
>                      // We have found one constant here.  We explicitly do not
>                      // bother calling isEnabled() here because it's OK to define
>                      // potentially extra constants on these shim interfaces.
>                      if (index == aIndex) {
>                          // This is a hack.  We're assuming that the callers do

This comment can go away.

        Attachment #8419367 -
        Flags: review?(bobbyholley) → review+

    
    

    
  
Yay!  I'm ready to land this when the tree reopens.

https://tbpl.mozilla.org/?tree=Try&rev=318e0c4deab5

    
    

    
  
Do we have any measurements on memory wins from this? Or do we still need bug 996061 to see improvements?
Flags: needinfo?(ehsan)

    
    

    
  
(In reply to Nicholas Nethercote [:njn] from comment #130)
> Do we have any measurements on memory wins from this? Or do we still need
> bug 996061 to see improvements?

I'm going to see if bug 996061 fixes anything today.
Flags: needinfo?(ehsan)
Depends on: 1008322
Depends on: 1216885
Depends on: 1416139
Component: DOM → DOM: Core & HTML

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: