296967 - [FIX]Could XPCNativeWrapper call the classinfo helpers?

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This came up in a conversation yesterday.  Would it be possible for
XPCNativeWrapper to call the Get/Set/Resolve classinfo hooks as needed and
remain secure?  That is, do whatever the current native wrapper class hooks do
w.r.t calling into the classinfo.

That would remove pretty much all the items at
<http://developer-test.mozilla.org/en/docs/XPCNativeWrapper#Limitations_of_XPCNativeWrapper>,
I think, as well as whatever other things might pop up in the future, so I think
it would be worth it if it wouldn't take too much code, wouldn't be too slow,
and most of all could be done securely.

Comment 1

[Brendan Eich [:brendan]]

Attachment: patch to restore JSClass.addProperty in/out *vp API compatibility

This is to help bz with auto-wrapping DOM objects in addProperty hooks.

/be

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 189732 [details] [diff] [review]
patch to restore JSClass.addProperty in/out *vp API compatibility

Yeah, this is great. r=shaver.

Comment 3

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 4

[Brendan Eich [:brendan]]

Oops.  Not my bug, I was just doing a helper patch!  How about I reopen and
assign to you, bz?

/be

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

This is the testcase I was using to test various stuff with XPCNativeWrapper. 
Set |control| to 1 at the top of the script to do a control run without
XPCNativeWrappers....  Note that failure to pass this testcase just means a
difference in behavior; some of the differences could be purposeful.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w for maybe review ease

Summary of changes:  With this patch we call the resolve, setProperty, and
getProperty hooks of the nsIXPCScriptable in the XPCWrappedNative.  I changed
the latter two hooks to indicate whether they actually set or got anything.  If
the hooks do something useful for us, we skip the rest of the XPCNativeWrapper
class hooks and return.

This patch fixes most of the items listed at
<http://developer.mozilla.org/en/docs/XPCNativeWrapper#Limitations_of_XPCNativeWrapper>.
 The remaining items are:

1)  No document.all support.  This is purposeful -- I don't see an easy way to
safety-wrap the stuff document.all returns, and its impementation in terms of
the proto chain doesn't really work so well for XPCNativeWrapper.
2)  No global scope polluter.  Generally not an issue, since I don't see a way
to do an unqualified resolve/get on an XPCNativeWrapper.
3)  Setting or getting on* properties on anything that's an event receiver
throws.  This is by-design -- I see no way to make it do anything sane, so
throwing is the best bet.
4)  Scriptable plugins probably still have issues because they also use the
proto chain.  So calling scriptable plugin stuff on an XPCNativeWrapper might
not work.
5)  Same for XBL-bound nodes.  Calling XBL-defined stuff on an XPCNativeWrapper
probably does not work.
6)  Enumeration ("for (var p in wrapper)") doesn't work right; I'll file a
separate bug on this.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Filed bug 301498 for enumeration.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 189946 [details] [diff] [review]
Same as diff -w for maybe review ease

>+  if (HAS_FLAGS(flags, FLAG_DEEP) && !primitive) {
>+
>+    XPCWrappedNative* wrappedNative =
>+      XPCWrappedNative::GetWrappedNativeOfJSObject(cx, nativeObj);
>+    if (!wrappedNative) {
>+      // Not something we can protect... just make it JSVAL_VOID
>+      *rval = JSVAL_VOID;

JSVAL_NULL better, slightly?

>+  if ((aIsSet && NATIVE_HAS_FLAG(wrappedNative, WantSetProperty)) ||
>+      (!aIsSet && NATIVE_HAS_FLAG(wrappedNative, WantGetProperty))) {

That should be aIsSet ? NATIVE_HAS_FLAG(wrappedNative, WantSetProperty) : ...,
eh?

More detailed review in a bit.

/be

Comment 10

[Boris Zbarsky [:bzbarsky]]

No opinion on VOID vs NULL, really.  And yes, the ?: is probably better there...

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 189946 [details] [diff] [review]
Same as diff -w for maybe review ease

I'm really not excited about adding arguments to the getProperty() and
setProperty() hooks. We talked about this on IRC, storing actually resolved
properties on the object for later use etc should fix this w/o changing
nsIXPCScriptable, and while it's probably less efficient, but who cares?

r- until for now...

Comment 12

[Boris Zbarsky [:bzbarsky]]

So... I tried the "remember whether we resolved" method.  The problem is that
there's lots of stuff that get/set hooks do in classinfo that resolve hooks do
NOT do.  Examples include:

1)  ['name'] lookup on document
2)  ['name'] lookup on nodelists
3)  [n] lookup on nodelists
4)  reads of on* properties on event receivers

Maybe more.  I sort of stopped looking...

#4 is not a big deal, as far as I'm concerned, but #1-#3 are something we want
working, imo.  Thoughts?  Should the resolve hooks just be changed?

Comment 13

[Boris Zbarsky [:bzbarsky]]

So per discussion with jst:

1)  Need to reintroduce the boolean for sets.  For gets I can make use of
JSVAL_HOLE.
2)  The ObjectInWrappedNativeIs approach is no good since people can pass in
random JSObjects.  Need to actually check for |obj| being an XPCNativeWrapper,
which is fun because the JSClass for that is defined in xpconnect, not layout. 
Any ideas on how to do that usefully?  This is critical to working with
split-window.

With those out of the way we could probably make this work...  I'll do what I
can, but if it's not done by next tuesday, it's not happening till Aug 14.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch per discussion earlier today

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase with explicit test for location.toString()

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 190800 [details] [diff] [review]
Patch per discussion earlier today

- In XPC_NW_NewResolve():

+  if (NATIVE_HAS_FLAG(wrappedNative, WantNewResolve) &&
+      // For "constructor" we don't want to call into the resolve
+      // hooks on the wrapped native, since that would give the wrong
+      // constructor.
+      id != GetStringByIndex(cx, XPCJSRuntime::IDX_CONSTRUCTOR)) {

Nit of the day: Put that comment *above* the if, not in the middle of the
condition.

- In nsXPCComponents::GetProperty(), and all other hooks changed in this diff:

+    *_retval = PR_TRUE;

Per discussions with jband way back when nsIXPCScriptable was initially
invented and implemented, setting *_retval to true is a waste of CPU
instructions. The callers are instead required to not ever pass in anything but
true in *_retval. I doubt this is documented anywhere, but that's how I've
written every implementation so far (i.e. all of nsDOMClassInfo.cpp).

- In dom/src/base/nsJSEnvironment.h:

+  // aHolder should be holding our global object
+  nsresult InitXPCNativeWrapperClass(nsIXPConnectJSObjectHolder *aHolder);

Maby rename this to FindXPCNativeWrapperClass since you're really not
initializing the class, just looking up the pointer to it.

sr=jst with those changes.

Comment 17

[Boris Zbarsky [:bzbarsky]]

>  The callers are instead required to not ever pass in anything but
> true in *_retval. 

I really wonder why this is an idl interface at all... it acts nothing like one.  ;)

I'll add docs to the interface to make this clear, I guess, and remove my
*_retval sets.

Will make the other changes.

Comment 18

[Johnny Stenback (:jst)]

Yeah, I don't know exactly why it's an IDL interface really...

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 190800 [details] [diff] [review]
Patch per discussion earlier today

I'm not sure I understand exactly everything that's going on here, but based on
what I do understand, this seems to make sense, so r=me

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

requesting 1.8b4 approval

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Do we have some sort of (semi-?)automated DOM test suite that we could add this
testcase to?  I can rework its behavior to do whatever said test suite needs...

If there is no such suite, maybe we should create one?