Last Comment Bug 574924 - TM: implement remaining wrappers
: TM: implement remaining wrappers
Status: RESOLVED FIXED
fixed-in-tracemonkey
: dev-doc-needed
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Andreas Gal :gal
:
Mentors:
Depends on: 576774 588738
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-25 23:16 PDT by Andreas Gal :gal
Modified: 2010-12-20 15:21 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (35.13 KB, patch)
2010-06-25 23:16 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (42.11 KB, patch)
2010-06-25 23:49 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (43.63 KB, patch)
2010-06-26 10:18 PDT, Andreas Gal :gal
mrbkap: review+
Details | Diff | Splinter Review
patch (61.17 KB, patch)
2010-06-26 14:19 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (63.62 KB, patch)
2010-06-26 14:48 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (65.88 KB, patch)
2010-06-26 15:03 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (111.96 KB, patch)
2010-06-28 18:38 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (122.29 KB, patch)
2010-06-30 19:42 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (122.34 KB, patch)
2010-07-01 14:02 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (123.01 KB, patch)
2010-07-01 15:41 PDT, Andreas Gal :gal
mrbkap: review+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2010-06-25 23:16:15 PDT
The only one missing now is systemonly.
Comment 1 Andreas Gal :gal 2010-06-25 23:16:30 PDT
Created attachment 454243 [details] [diff] [review]
patch
Comment 2 Andreas Gal :gal 2010-06-25 23:49:20 PDT
Created attachment 454245 [details] [diff] [review]
patch

Add a sow implementation. Pretty straight forward.
Comment 3 Andreas Gal :gal 2010-06-26 10:18:07 PDT
Created attachment 454279 [details] [diff] [review]
patch
Comment 4 Blake Kaplan (:mrbkap) 2010-06-26 11:17:07 PDT
Comment on attachment 454279 [details] [diff] [review]
patch

Please use |if (| instead of |if(|. We're not using XPConnect style in the new directory, and the bastardization looks worse than either of the original styles.

>diff --git a/js/src/xpconnect/src/wrappers/AccessCheck.cpp b/js/src/xpconnect/src/wrappers/AccessCheck.cpp
>+// Hardcoded policy for cross origin property access.

Comment that this list was culled from the prefs file?

>+AccessCheck::isSystemOnlyAccessPermitted(JSContext *cx)
> {
>+    if(fp &&
>+       (filename = fp->script->filename) &&
>+       !strncmp(filename, prefix, NS_ARRAY_LENGTH(prefix) - 1)) {

Nit: strncmp line is underhung by one space.

>diff --git a/js/src/xpconnect/src/wrappers/SystemOnlyWrapper.cpp b/js/src/xpconnect/src/wrappers/SystemOnlyWrapper.cpp
>+JSString *
>+SystemOnlyWrapper::obj_toString(JSContext *cx, JSObject *wrapper)
>+{
>+    if(!Check(cx, JSVAL_VOID)) {
>+        return NULL;
>+    }
>+    return JSWrapper::obj_toString(cx, wrapper);
>+}
>+
>+JSString *
>+SystemOnlyWrapper::fun_toString(JSContext *cx, JSObject *wrapper, uintN indent)
>+{
>+    if(!Check(cx, JSVAL_VOID)) {
>+        return NULL;
>+    }
>+    return JSWrapper::fun_toString(cx, wrapper, indent);
>+}

Why not use && for these two since you did for every other function in this file?

>diff --git a/js/src/xpconnect/src/wrappers/XrayWrapper.cpp b/js/src/xpconnect/src/wrappers/XrayWrapper.cpp
>+ResolveNativeProperty(JSContext *cx, JSObject *holder, jsval id, bool set, JSPropertyDescriptor *desc)
>+{
>+    desc->obj = NULL;
>+
>+    NS_ASSERTION(holder->getClass() == &HolderClass, "expected a native property holder object");
>+    JSObject *wnObject = GetWrappedNativeObject(holder);
>+    XPCWrappedNative *wn = GetWrappedNative(wnObject);
>+
>+    // This will do verification and the method lookup for us.
>+    XPCCallContext ccx(JS_CALLER, cx, holder, nsnull, id);
>+
>+    // Run the resolve hook of the wrapped native.
>+    JSBool retval = true;
>+    JSObject *pobj = NULL;
>+    uintN flags = cx->resolveFlags | (set ? JSRESOLVE_ASSIGNING : 0);
>+    nsresult rv = wn->GetScriptableInfo()->GetCallback()->NewResolve(wn, cx, holder, id, flags,
>+                                                                     &pobj, &retval);

We need some way for NewResolve to tell that this is an XRay wrapper (right now we use a combination of wn->GetFlatJSObject() != obj and ObjectIsNativeWrapper()). Otherwise, the scriptable helper will resolve too many properties.
Comment 5 Andreas Gal :gal 2010-06-26 14:19:51 PDT
Created attachment 454297 [details] [diff] [review]
patch
Comment 6 Andreas Gal :gal 2010-06-26 14:48:18 PDT
Created attachment 454299 [details] [diff] [review]
patch
Comment 7 Andreas Gal :gal 2010-06-26 15:03:53 PDT
Created attachment 454301 [details] [diff] [review]
patch
Comment 8 Andreas Gal :gal 2010-06-28 18:38:17 PDT
Created attachment 454736 [details] [diff] [review]
patch
Comment 9 Andreas Gal :gal 2010-06-28 18:39:23 PDT
This patch rolls all wrappers into a single instance (no more double wrapping).
Comment 10 Andreas Gal :gal 2010-06-30 19:42:45 PDT
Created attachment 455347 [details] [diff] [review]
patch

Ok right bug, right patch. Ready for review.
Comment 11 Andreas Gal :gal 2010-07-01 14:02:56 PDT
Created attachment 455541 [details] [diff] [review]
patch
Comment 12 Blake Kaplan (:mrbkap) 2010-07-01 15:38:52 PDT
Comment on attachment 455541 [details] [diff] [review]
patch

Stuff pointed out in more detail over IRC:

* CHECKED and PIERCE need to leave, even if op() fails.
* Last ditch UniversalXPConnect in the SOW check.
* COWs __exposedProps__ duplicate 'r' and 'w' error messages.
* +        if (!!(flags & WAIVE_XRAY_WRAPPER_FLAG)) { no need for !!.
* Seems like XrayWrapper should check for .wrappedJSObject before asking if the underlying object has a .wrappedJSObject property (content objects won't, so this shouldn't matter, but seems like good hygiene).

I'll stamp the next patch.
Comment 13 Andreas Gal :gal 2010-07-01 15:41:02 PDT
Created attachment 455569 [details] [diff] [review]
patch
Comment 14 Andreas Gal :gal 2010-07-01 15:41:47 PDT
Comment on attachment 455569 [details] [diff] [review]
patch

Note: the .wrappedJS case was also missing a return true;
Comment 15 Blake Kaplan (:mrbkap) 2010-07-01 15:43:07 PDT
Comment on attachment 455569 [details] [diff] [review]
patch

Looks good, thanks.
Comment 16 Andreas Gal :gal 2010-07-01 15:45:57 PDT
http://hg.mozilla.org/tracemonkey/rev/d4caa61e69ab
Comment 17 Andreas Gal :gal 2010-07-01 15:46:20 PDT
Luke, this patch has a trivial id<>jsval issue. Just a heads up.
Comment 19 Nickolay_Ponomarev 2010-10-26 12:56:07 PDT
Since the xray wrappers are showing in the Web Console ("[object XrayWrapper [object HTMLDocument]]"), they should get a mention in the docs, e.g. on https://developer.mozilla.org/en/XPConnect_wrappers

FWIW, it wasn't clear to me (an outside reader) from the comment in the header how they affect people writing JS (e.g. in extensions) and what is their difference to other wrappers:
// Xray wrappers re-resolve the original native properties on the native
// object and always directly access to those properties.

It sounds similar to XPCNW, but what's the difference?
Comment 20 Brendan Eich [:brendan] 2010-10-26 14:03:00 PDT
Should wrappers be showing this way in the console?

/be
Comment 21 Eric Shepherd [:sheppy] 2010-12-20 13:51:16 PST
I've added notes here:

https://developer.mozilla.org/en/XPConnect_wrappers
https://developer.mozilla.org/en/Using_the_Web_Console#The_Web_Console_log

Please let me know if this is inadequate.
Comment 22 Nickolay_Ponomarev 2010-12-20 15:21:51 PST
It would really be better if someone from the people who understand these wrappers tried to explain what they are.

Note You need to log in before you can comment on or make changes to this bug.