574924 - TM: implement remaining wrappers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

The only one missing now is systemonly.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Attachment: patch

Add a sow implementation. Pretty straight forward.

Comment 3

[Andreas Gal :gal]

Attachment: patch

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

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]

Attachment: patch

Comment 6

[Andreas Gal :gal]

Attachment: patch

Comment 7

[Andreas Gal :gal]

Attachment: patch

Comment 8

[Andreas Gal :gal]

Attachment: patch

Comment 9

[Andreas Gal :gal]

This patch rolls all wrappers into a single instance (no more double wrapping).

Comment 10

[Andreas Gal :gal]

Attachment: patch

Ok right bug, right patch. Ready for review.

Comment 11

[Andreas Gal :gal]

Attachment: patch

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

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]

Attachment: patch

Comment 14

[Andreas Gal :gal]

Comment on attachment 455569 [details] [diff] [review]
patch

Note: the .wrappedJS case was also missing a return true;

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 455569 [details] [diff] [review]
patch

Looks good, thanks.

Comment 16

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/d4caa61e69ab

Comment 17

[Andreas Gal :gal]

Luke, this patch has a trivial id<>jsval issue. Just a heads up.

Comment 18

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/efd06f813388

Comment 19

[Nickolay_Ponomarev]

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]]

Should wrappers be showing this way in the console?

/be

Comment 21

[Eric Shepherd [:sheppy]]

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]

It would really be better if someone from the people who understand these wrappers tried to explain what they are.