Closed Bug 999297 Opened 10 years ago Closed 10 years ago

Stop doing XPCWrappedNative gymnastics in xpc::HasInstance

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files)

Right now we do a bunch of unnecessary beating around the bush. I can make this simpler, which will help out ehsan in bug 994964.
Thanks!
I kind of thought we already had something like this, but I couldn't find
anything.
Attachment #8410338 - Flags: review?(bzbarsky)
Attachment #8410339 - Flags: review?(bzbarsky)
Doesn't this just move the problem out of xpc::HasInstance() to XPCWrappedNative::FindTearOff()?  I thought the goal is to avoid hitting <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNativeInfo.cpp#135>.
Flags: needinfo?(bobbyholley)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #4)
> Doesn't this just move the problem out of xpc::HasInstance() to
> XPCWrappedNative::FindTearOff()?  I thought the goal is to avoid hitting
> <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/
> XPCWrappedNativeInfo.cpp#135>.

Glad you're paying attention! :-)

It's actually fine - let me explain.

First of all, in truth, the code on trunk isn't a problem for you, because XPCNativeInterface::GetNewOrUsed(nsIID*) only calls into XPTInterfaceInfoManager (the "new" branch) if it doesn't find an existing entry in the IID2NativeInterfaceMap (the "used" branch). And given that the nsJSIID would be holding onto its entry (via mInfo), we're guaranteed to take the "used" branch when nsJSIID calls into xpc::HasInstance. I was tempted to just explain this over in bug 994964, but it felt a bit brittle, and xpc::HasInstance could use some simplification anyway, which brings me to the next point.

Second, the patches in this bug alter xpc::HasInstance so that the instanceof check itself just depends on a simple QI of the native, and not the XPCWrappedNative goop. The only reason I had to add some of the goop back was for this best-effort auto-QI hack for buggy chrome scripts, which expect the properties to be automatically resolved after a successful instanceof check. This is a non-issue for DOM objects, since those have classinfo and automatically have all of the relevant properties on the prototype. And if we're pulling this stunt on a non-DOM object, then there's really nothing we can do here, because we're making these interfaces empty from the perspective of script, so there would be no properties to resolve.

I would have explained all of this before, but you indicated yesterday that you're not really interested in learning about the guts of this stuff. ;-)
Flags: needinfo?(bobbyholley)
(In reply to comment #5)
> It's actually fine - let me explain.
> 
> First of all, in truth, the code on trunk isn't a problem for you, because
> XPCNativeInterface::GetNewOrUsed(nsIID*) only calls into
> XPTInterfaceInfoManager (the "new" branch) if it doesn't find an existing entry
> in the IID2NativeInterfaceMap (the "used" branch). And given that the nsJSIID
> would be holding onto its entry (via mInfo), we're guaranteed to take the
> "used" branch when nsJSIID calls into xpc::HasInstance. I was tempted to just
> explain this over in bug 994964, but it felt a bit brittle, and
> xpc::HasInstance could use some simplification anyway, which brings me to the
> next point.
> 
> Second, the patches in this bug alter xpc::HasInstance so that the instanceof
> check itself just depends on a simple QI of the native, and not the
> XPCWrappedNative goop. The only reason I had to add some of the goop back was
> for this best-effort auto-QI hack for buggy chrome scripts, which expect the
> properties to be automatically resolved after a successful instanceof check.
> This is a non-issue for DOM objects, since those have classinfo and
> automatically have all of the relevant properties on the prototype. And if
> we're pulling this stunt on a non-DOM object, then there's really nothing we
> can do here, because we're making these interfaces empty from the perspective
> of script, so there would be no properties to resolve.

Aha!  Now this all makes sense.  Does this mean that other places in nsJSIID code where we pass the iid and not the nsIInterfaceInfo* to GetNewOrUsed are fine based on the same reasoning?

> I would have explained all of this before, but you indicated yesterday that
> you're not really interested in learning about the guts of this stuff. ;-)

Haha, I'm kind of not, but I guess it's a bit too late now, and bits and pieces of this puzzle are slowly fitting into place anyways.

(BTW sorry if I was cranky yesterday when we were talking, I didn't mean to.  :-)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #6)
> Aha!  Now this all makes sense.  Does this mean that other places in nsJSIID
> code where we pass the iid and not the nsIInterfaceInfo* to GetNewOrUsed are
> fine based on the same reasoning?

I would assume so, because we have an invariant that we don't create duplicate XPCNativeInterface instances for a given runtime. But the only way to tell is by looking at the code. In particular, the mapping for const char *interfaceName to nsIID* (via nsIInterfaceInfo) happens with the help of the XPTInterfaceInfoManager, so if there's any code using the name for some reason, that would be a problem.

> Haha, I'm kind of not, but I guess it's a bit too late now, and bits and
> pieces of this puzzle are slowly fitting into place anyways.
> 
> (BTW sorry if I was cranky yesterday when we were talking, I didn't mean to.
> :-)

No worries at all! :-)
(In reply to comment #7)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #6)
> > Aha!  Now this all makes sense.  Does this mean that other places in nsJSIID
> > code where we pass the iid and not the nsIInterfaceInfo* to GetNewOrUsed are
> > fine based on the same reasoning?
> 
> I would assume so, because we have an invariant that we don't create duplicate
> XPCNativeInterface instances for a given runtime. But the only way to tell is
> by looking at the code. In particular, the mapping for const char
> *interfaceName to nsIID* (via nsIInterfaceInfo) happens with the help of the
> XPTInterfaceInfoManager, so if there's any code using the name for some reason,
> that would be a problem.

I don't think that there is.  Thanks!
> This is a non-issue for DOM objects, since those have classinfo and automatically have
> all of the relevant properties on the prototype.

Unless they implement interfaces they don't list in classinfo.  We certainly had DOM objects doing that!

WebIDL objects don't have this problem, of course.
(In reply to comment #9)
> > This is a non-issue for DOM objects, since those have classinfo and automatically have
> > all of the relevant properties on the prototype.
> 
> Unless they implement interfaces they don't list in classinfo.  We certainly
> had DOM objects doing that!

Like nsIImageLoadingContent?
Yes, as a prominent example.
Well, that's commonly used by add-ons, isn't it?  Is that OK?
Yes, because "WebIDL objects don't have this problem".  All the nsIImageLoadingContent bits are already flattened onto <img> on trunk, but [ChromeOnly].
(In reply to comment #13)
> Yes, because "WebIDL objects don't have this problem".  All the
> nsIImageLoadingContent bits are already flattened onto <img> on trunk, but
> [ChromeOnly].

You're right.  And guess who did that work?  ;-)

I guess I'm getting old...
Comment on attachment 8410338 [details] [diff] [review]
Part 1 - Implement xpc::UnwrapReflectorToISupports. v1

>+    // Unwrap security wrappers, if allowed.
>+    reflector = js::UncheckedUnwrap(reflector, /* stopAtOuter = */ false);

The "if allowed" part of the comment makes no sense to me given the UncheckedUnwrap.  I assume the comment is what's wrong.

>+        if (!wn)

Can it really be null?  I mean, I know the code used to have that null-check, but is it relevant?

The differences between this function and XPCConvert::GetISupportsFromJSObject are ... subtle.  :(  We might want to add some comments for when which one should be used.

r=me with those addressed
Attachment #8410338 - Flags: review?(bzbarsky) → review+
Comment on attachment 8410339 [details] [diff] [review]
Part 2 - Simplify xpc::HasInstance. v1

I just discovered FindObjectForHasInstance.  Now I feel sick.  Do we really have chrome code depending on #2?  We should consider a followup to rip that out.  :(

I looked up the blame on the HasInterfaceNoQI, and it's part of the xpcdom landing.  I guess it was trying to deal with things that used classinfo and had no tearoffs for those interfaces or something?

Also, now I know why instanceof happened to flatten things onto the object.  <sigh>.

r=me.
Attachment #8410339 - Flags: review?(bzbarsky) → review+
This had a try push that I forgot to include here:

https://tbpl.mozilla.org/?tree=Try&rev=cd455980bc27
(In reply to Boris Zbarsky [:bz] from comment #15)
> >+    // Unwrap security wrappers, if allowed.
> >+    reflector = js::UncheckedUnwrap(reflector, /* stopAtOuter = */ false);
> 
> The "if allowed" part of the comment makes no sense to me given the
> UncheckedUnwrap.  I assume the comment is what's wrong.

No, it should have been CheckedUnwrap. Fixed.

> 
> >+        if (!wn)
> 
> Can it really be null?  I mean, I know the code used to have that
> null-check, but is it relevant?

I'm going to file a followup bug to try removing all of these null checks.
 
> The differences between this function and
> XPCConvert::GetISupportsFromJSObject are ... subtle.  :(  We might want to
> add some comments for when which one should be used.

Done.
(In reply to Bobby Holley (:bholley) from comment #18)
> > Can it really be null?  I mean, I know the code used to have that
> > null-check, but is it relevant?
> 
> I'm going to file a followup bug to try removing all of these null checks.

Actually, I think we should probably wait until we get rid of XPCWN reparenting, which can happen when we eliminate PreCreate in its current form.
https://hg.mozilla.org/mozilla-central/rev/660ddfb69dc5
https://hg.mozilla.org/mozilla-central/rev/d92c11752f44
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.