Closed
Bug 296967
Opened 19 years ago
Closed 19 years ago
[FIX]Could XPCNativeWrapper call the classinfo helpers?
Categories
(Core :: DOM: Core & HTML, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.8beta4
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(4 files, 3 obsolete files)
4.08 KB,
patch
|
shaver
:
review+
brendan
:
approval1.8b4+
|
Details | Diff | Splinter Review |
62.95 KB,
patch
|
mrbkap
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
7.17 KB,
text/html
|
Details | |
56.32 KB,
patch
|
asa
:
approval1.8b4+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
This is to help bz with auto-wrapping DOM objects in addProperty hooks. /be
Attachment #189732 -
Flags: review?(shaver)
Attachment #189732 -
Flags: approval1.8b4+
Comment on attachment 189732 [details] [diff] [review] patch to restore JSClass.addProperty in/out *vp API compatibility Yeah, this is great. r=shaver.
Attachment #189732 -
Flags: review?(shaver) → review+
Comment 3•19 years ago
|
||
Fixed. /be
Status: NEW → RESOLVED
Closed: 19 years ago
Flags: blocking1.8b4+
Resolution: --- → FIXED
Comment 4•19 years ago
|
||
Oops. Not my bug, I was just doing a helper patch! How about I reopen and assign to you, bz? /be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•19 years ago
|
Assignee: general → bzbarsky
Status: REOPENED → NEW
Assignee | ||
Updated•19 years ago
|
Priority: -- → P1
Summary: Could XPCNativeWrapper call the classinfo helpers? → [FIX]Could XPCNativeWrapper call the classinfo helpers?
Target Milestone: --- → mozilla1.8beta4
Assignee | ||
Comment 5•19 years ago
|
||
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.
Assignee | ||
Comment 6•19 years ago
|
||
Assignee | ||
Comment 7•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #189946 -
Flags: superreview?(shaver)
Attachment #189946 -
Flags: review?(jst)
Assignee | ||
Comment 8•19 years ago
|
||
Filed bug 301498 for enumeration.
Comment 9•19 years ago
|
||
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
Assignee | ||
Comment 10•19 years ago
|
||
No opinion on VOID vs NULL, really. And yes, the ?: is probably better there...
Comment 11•19 years ago
|
||
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...
Attachment #189946 -
Flags: review?(jst) → review-
Assignee | ||
Comment 12•19 years ago
|
||
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?
Assignee | ||
Updated•19 years ago
|
Attachment #189946 -
Flags: superreview?(shaver)
Assignee | ||
Comment 13•19 years ago
|
||
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.
Assignee | ||
Comment 14•19 years ago
|
||
Attachment #189939 -
Attachment is obsolete: true
Attachment #189946 -
Attachment is obsolete: true
Attachment #190800 -
Flags: superreview?(jst)
Attachment #190800 -
Flags: review?(mrbkap)
Assignee | ||
Comment 15•19 years ago
|
||
Attachment #189937 -
Attachment is obsolete: true
Comment 16•19 years ago
|
||
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.
Attachment #190800 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 17•19 years ago
|
||
> 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•19 years ago
|
||
Yeah, I don't know exactly why it's an IDL interface really...
Comment 19•19 years ago
|
||
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
Attachment #190800 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 20•19 years ago
|
||
requesting 1.8b4 approval
Attachment #190901 -
Flags: approval1.8b4?
Updated•19 years ago
|
Attachment #190901 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Comment 21•19 years ago
|
||
Fixed.
Status: NEW → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 22•19 years ago
|
||
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?
Updated•17 years ago
|
Flags: in-testsuite?
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•