Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

NEW
Unassigned

Status

()

Core
XPConnect
P3
normal
18 years ago
8 years ago

People

(Reporter: John Bandhauer, Unassigned)

Tracking

({perf})

Trunk
Future
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

18 years ago
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.
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.

(Reporter)

Comment 2

18 years ago
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.
(Reporter)

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 3

18 years ago
*** Bug 28120 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 4

17 years ago
Created attachment 12058 [details] [diff] [review]
proposed interim patch using wrappers around wrappers
(Reporter)

Comment 5

17 years ago
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.
(Reporter)

Comment 6

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

Comment 7

17 years ago
John: nsbeta3 goes in the keyword field, not status whiteboard. 
Keywords: nsbeta3
Whiteboard: nsbeta3 → [nsbeta3+]
(Reporter)

Comment 8

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

Clearing "nsbeta2" decorations.
Keywords: nsbeta3
Whiteboard: [nsbeta3+]
(Reporter)

Comment 9

16 years ago
mass reassign of xpconnect bugs to dbradley@netscape.com
Assignee: jband → dbradley
Status: ASSIGNED → NEW

Updated

16 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Future

Comment 10

16 years ago
Removing Rob as QA contact
QA Contact: rginda → pschwartau

Comment 11

11 years ago
This shouldn't be an in issue anymore...
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → WORKSFORME

Comment 12

11 years ago
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

11 years ago
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

11 years ago
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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

Updated

11 years ago
Keywords: perf

Comment 15

11 years ago
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

11 years ago
(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.
(Reporter)

Comment 17

11 years ago
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 :)
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.
Summary: xpconnect needs wrappers for JS -> JS communication → (maybe) optimize xpconnect wrapping for JS -> JS communication

Comment 19

11 years ago
> Do you mean via .wrappedJSObject?

No, I mean as described at the end of comment 1.
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

11 years ago
Ah, ok.  So what's the issue remaining, then?  I don't see a clear descrption of it in this bug....
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.
QA Contact: pschwartau → xpconnect
Assignee: dbradley → nobody
Status: REOPENED → NEW
We're using many more JS XPCOM components these days.  I wonder if it's worth it now.
You need to log in before you can comment on or make changes to this bug.