131048 - Scriptable methods should not have non-scriptable interface parameters

Status: VERIFIED WONTFIX

(Core :: XPCOM, defect, P3)

Description

[timeless]

i'm running with a patch from bug 130139.
i have an xpconnect windowwatcher class that's trying to proxy for the real thing.

it's setWindowCreator method isn't being called. This method is critical.

more later

Comment 1

[timeless]

Attachment: components/windowwatcher.js

windowwatcher proxy component

Comment 2

[timeless]

Attachment: make mozilla component ;2

apply this patch to allow the windowwatcher.js component to be the main window
watcher. theoretically, that class will pass each function through to the class
we're renaming to ;2

Comment 3

[timeless]

Attachment: nsWindowWatcher.cpp function call logging

tag each function with some debug output so that we can see what methods are
being called, when, and with what params

Comment 4

[timeless]

Attachment: xpcshell test of windowatcher.js

Comment 5

[timeless]

Attachment: normal startup log (based on applying attachment 74394)

Comment 6

[timeless]

Attachment: fatal startup log (applying attachment 74392, attachment 74394 and installing attachment 74391)

Comment 7

[David Bradley]

Ok, I'm going to change the summary to reflect what we found. Basically people
should be chastised appropriate for using non-scriptable interfaces as
parameters to scriptable methods.

I'd vote for an error, but that might be too heavy handed. I'll see how
pervasive a problem this is and report back.

Comment 8

[Doug Turner (:dougt)]

It is perfectly acceptable to have an interface with noscript methods.  A method
that has a "noscript" parameter should be treated as if the entire method was
not script.

Comment 9

[David Bradley]

Right, but what I meant is that if a method is not marked noscript or notxpcom
and it occurs on a scriptable interface and has a parameter that is of a
non-scriptable interface, then we should at least warn that this is the case.

If we make this implicit, then we'll end up with similar hard to track down
problems where people assumed an interface was scriptable.

Comment 10

[David Bradley]

Targetting, I don't think this is critical for 1.0.

Comment 11

[Dan M]

Dudes, I've just spend several minutes glancing over all the attachments and
logs and I still don't know what the issue is. But I do have the odd impression
I've been wasting my time. What's the deal with the big log of windowwatcher
methods that were called during some test run? They're all used; all its
methods. Really. I gather you're trying to write a new windowwatcher in
JavaScript and you're running afoul of some interface issues. How about a
simple, short description of which methods and which parameters?

Intrigued, I spent some time looking. Nothing jumps to mind. Every method in
nsIWindowWatcher takes normal, well-defined parameters, doesn't it? What am I
missing? Are you referring to nsPIWindowWatcher::OpenWindowJS? nsPIWindowWatcher
isn't scriptable at all. It's the dirty underbelly. The scriptable interface for
nsIWindowWatcher is intended for getting to the window watcher from script. The
component itself is not intended to be replaceable by a component written in
script. If you want to write a replacement component, I suggest you do it in C++.

I'm curious to see what your plans are for making windowwatcher's interface,
even the private, internal one, scriptable. I anticipate some fighting. Note
also that nsIWindowWatcher, the public interface, was mistakenly not frozen for
1.0. It will be. Soon.

Also, you might want to consider renaming your contract ID. You're colliding
with the Mozilla component's namespace. Version 2 is what we'll be using for the
standard component when we have to rev it. I suggest that instead of
@mozilla.org/embedcomp/window-watcher;1 you use, well, something utterly
different. @not-at-all-mozilla.org/my-own-window-watcher;1, maybe.

Comment 12

[John Bandhauer]

danm: I'm with you. This is a timeless-morpho bug. dbradley is attempting to
focus on the issue of detecting at xpidl compile time the case where a
scriptable method has a non-scriptable parameter. At runtime this case will
cause xpconnect to 'hide' the method - because it has resolved to not actually
being scriptable.

Because we have late type binding, I don't think it is reasonable to expect to
be able to always detect this case at compile time; i.e. a given param type
might only be forward declared. There is no requirement that the xpidl compiler
be able to actually find the param's full interface declaration in each place
the param's type is used. In fact, it is perfectly legal for the param's type to
have no full interface declaration at all (and even if it exists at compile
time, it is legal for it to not be findable at runtime without the whole world
imploding).

Arguably, we could emit debug only warnings at runtime (from xpconnect or the
typelib loader). But a list of those each time we startup would just be noise to
99.9% of the developers. It would also be fairly simple to build a tool - an
executable - that would load up the xpt files and detect these cases. This is
hardly critical. We are accepting code contributions.

[adding shaver to the cc list even though I'm claiming this is not worth wasting
time on :)]

Comment 13

[David Bradley]

I agree, it's not worth it, I wasn't accounting for the forward declarations, I
unfortunately only briefly looked at this when it came up. I vote for WONTFIX,
and marking as such.

Comment 14

[Phil Schwartau]

OK, marking Verified -

Comment 15

[timeless]

not that it matters,
> I gather you're trying to write a new windowwatcher in JavaScript and
> you're running afoul of some interface issues.
yes

> you might want to consider renaming your contract ID.
> You're colliding with the Mozilla component's namespace.
now this one's cute.  If I need to write a drop in replacement, how precisely 
do I replace a mozilla component without naming my contract the same as the one 
that other compiled modules request?