25180 - (maybe) optimize xpconnect wrapping for JS -> JS communication

Status: RESOLVED INCOMPLETE

(Core :: XPConnect, defect, P3)

Description

[John Bandhauer]

Currently when JS code calls some interface that returns an xpcom object that 
happens to be implemented in JS then that object is received by the JS caller as 
a plain JS object; i.e. it is unwrapped for direct JS to JS calls.

This is efficient, but it undermines some of the cool support for xpcom objects 
built into xpconnect. And it is counter to the goal of transparent use of 
components regardless of implementation language because it makes 
unavailable some of the features available to JS code that uses a native 
interface. For instance... 

- automatic handling of QI
- "foo instanceof Components.interfaces.nsIFoo"
- foo.toString()
- the "foo.nsIFoo" for QI scheme proposed in bug 24694
- flattening

In order to really make JS components work right for JS callers I propose to 
implement a new kind of wrapper (wrappedJS4JS?) with its own JSOps. This should 
be pretty easy to implement. I can steal and convert a lot of the code I need 
from existing code.

Comment 1

[Mike Shaver (:shaver emeritus)]

Is that the case right now?

When I getService on a JS-implemented component, I get an ``[xpconnect wrapped
nsIWhatever]'' back.  But methods on that object that return JS objects as
implementations of interfaces do exhibit the unwrapping (never-wrapping?) stuff
that you're talking about.

Comment 2

[John Bandhauer]

You are seeing the "wrappers around wrappers" situation that I checked in a fix 
for last night. createInstance and getService were calling wrapNative 
regardless of the actual language of the component:
http://bugzilla.mozilla.org/show_bug.cgi?id=16130
In today's build you won't see that.

Comment 3

[Robert Ginda]

*** Bug 28120 has been marked as a duplicate of this bug. ***

Comment 4

[John Bandhauer]

Attachment: proposed interim patch using wrappers around wrappers

Comment 5

[John Bandhauer]

The patch above uses wrappers around wrappers to simulate the effect of the 
JS -> JS wrappers. This will give us the consistent behavior we want. It can be 
transparently replaced with the better wrappers later.

I added support for the wrapper user to ask for the underlying JSObject. This 
will only succeed if the underlying object has decided to allow it AND the 
security manager does not veto. See the comments.

Comment 6

[John Bandhauer]

nominating for nsbeta3 because JS components are happen'n and we don't want to 
break them later when we add this insulation layer.

Comment 7

[Warren Harris]

John: nsbeta3 goes in the keyword field, not status whiteboard. 

Comment 8

[John Bandhauer]

Interim patch checked in. The long term fix still needs to be done. 

Clearing "nsbeta2" decorations.

Comment 9

[John Bandhauer]

mass reassign of xpconnect bugs to [e-mail]

Comment 10

[David Bradley]

Removing Rob as QA contact

Comment 11

[Magnus Melin [:mkmelin]]

This shouldn't be an in issue anymore...

Comment 12

[David Bradley]

Can you provide a little more explanation.

From reading the bug, it was left open for a longer term fix that hasn't happened -> "Interim patch checked in. The long term fix still needs to be done."

If we're not going to bother with the long term fix, that's fine, but it should be stated in closing this bug.

Comment 13

[Magnus Melin [:mkmelin]]

Aah, missed the part about "long term fix"... Should I reopen? Or feel free to do it for me.

(I just found the bug searching something related, was first led to believe it wouldn't work, found out it works, thought it just had been forgotten/fixed elsewhere)

Comment 14

[David Bradley]

I'm tempted to just leave it closed. It's worked ok this long. One question would be how often this case comes up and would we get a decent perf boost by creating the special wrappers and eliminating the double wrapping. Going to reopen until that question is answered.

Comment 15

[Boris Zbarsky [:bzbarsky]]

There's also the side question of how much of our UI we'd break.  :(  I bet people are relying on getting the unwrapped JS objects... :(

Comment 16

[[e-mail]]

(In reply to comment #15)
>I bet people are relying on getting the unwrapped JS objects... :(
Do you mean via .wrappedJSObject? If so, that's fairly popular; Chatzilla uses it, for instance, and so apparently do parts of toolkit and browser, and I even used it in suite to work around an XPConnect exception-handling bug.
If I do something like tree.view = { selection: {} }; then tree.view.selection is still a wrapped JS object, not the original JS object.

Comment 17

[John Bandhauer]

I doubt anyone will ever feel the need to implement the new sort of wrapper that this bug proposes. It is a new level of complication. And, AFAIK, no one has shown that the performance of the current system is bad enough to justify the change. 

For the benefit of people who don't know the history... The original comment in this bug describes the situation *before* XPConnect did double wrapping. Double wrapping, along with the wrappedJSObject mechanism as a way to get at the underlying JSObject, has been in place for a long time. *That* is the status quo. If this new sort of wrapper were implemented it would be a completely transparent change to any code using such objects.

FWIW, I'm of the mind that bugs and feature requests that are likely to never be addressed ought to be resolved as LATER or WONTFIX rather than having them linger on bug lists forever :)

Comment 18

[Mike Shaver (:shaver emeritus)]

Fixing this wouldn't have to break .wrappedJSObject at all, though it might let code be poked in ways that it doesn't expect.

Some instrumentation here would, as dbradley says, be most fruitful, since the marshalling/demarshalling code in XPConnect is not especially cheap.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Do you mean via .wrappedJSObject?

No, I mean as described at the end of comment 1.

Comment 20

[Mike Shaver (:shaver emeritus)]

Comment #1 was fixed by the checkin referenced in comment #8, no?  We should always be wrapping return values now, regardless of implementation language.  If not, we have a major (other) bug.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  So what's the issue remaining, then?  I don't see a clear descrption of it in this bug....

Comment 22

[Mike Shaver (:shaver emeritus)]

The issue is that we currently create two wrappers for JS-calls-JS, and therefore go through the value marshalling and xptcall/stub code twice, at non-zero cost in time and malloc churn.

As the bug is now summarized (and was originally filed; see also comment 5), it's about optimizing those cases via a custom JS-to-JS wrapper to improve performance -- if we decide that it's worth it.

Comment 23

[Myk Melez [:myk] [@mykmelez]]

We're using many more JS XPCOM components these days.  I wonder if it's worth it now.

Comment 24

[Andrew McCreight [:mccr8]]

We can file new bugs if there's too much overhead from wrappers in chrome JS.