TM: implement remaining wrappers

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

({dev-doc-needed})

Other Branch
x86
Mac OS X
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

7 years ago
The only one missing now is systemonly.
(Assignee)

Comment 1

7 years ago
Created attachment 454243 [details] [diff] [review]
patch
Assignee: general → gal
(Assignee)

Updated

7 years ago
Summary: TM: implement cross-origin and x-ray wrappers → TM: implement cross-origin, x-ray and sow wrappers
(Assignee)

Comment 2

7 years ago
Created attachment 454245 [details] [diff] [review]
patch

Add a sow implementation. Pretty straight forward.
Attachment #454243 - Attachment is obsolete: true
(Assignee)

Comment 3

7 years ago
Created attachment 454279 [details] [diff] [review]
patch
Attachment #454245 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #454279 - Flags: review?(mrbkap)
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.
Attachment #454279 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 5

7 years ago
Created attachment 454297 [details] [diff] [review]
patch
(Assignee)

Updated

7 years ago
Attachment #454297 - Flags: review?(mrbkap)
(Assignee)

Comment 6

7 years ago
Created attachment 454299 [details] [diff] [review]
patch
Attachment #454297 - Attachment is obsolete: true
Attachment #454297 - Flags: review?(mrbkap)
(Assignee)

Comment 7

7 years ago
Created attachment 454301 [details] [diff] [review]
patch
Attachment #454299 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #454301 - Flags: review?(mrbkap)
(Assignee)

Comment 8

7 years ago
Created attachment 454736 [details] [diff] [review]
patch
Attachment #454279 - Attachment is obsolete: true
Attachment #454301 - Attachment is obsolete: true
Attachment #454301 - Flags: review?(mrbkap)
(Assignee)

Updated

7 years ago
Attachment #454736 - Flags: review?(mrbkap)
(Assignee)

Comment 9

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

Updated

7 years ago
Summary: TM: implement cross-origin, x-ray and sow wrappers → TM: implement remaining wrappers
(Assignee)

Comment 10

7 years ago
Created attachment 455347 [details] [diff] [review]
patch

Ok right bug, right patch. Ready for review.
Attachment #454736 - Attachment is obsolete: true
Attachment #455347 - Flags: review?(mrbkap)
Attachment #454736 - Flags: review?(mrbkap)
(Assignee)

Comment 11

7 years ago
Created attachment 455541 [details] [diff] [review]
patch
Attachment #455347 - Attachment is obsolete: true
Attachment #455541 - Flags: review?(mrbkap)
Attachment #455347 - Flags: review?(mrbkap)
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.
Attachment #455541 - Flags: review?(mrbkap)
(Assignee)

Comment 13

7 years ago
Created attachment 455569 [details] [diff] [review]
patch
Attachment #455541 - Attachment is obsolete: true
(Assignee)

Comment 14

7 years ago
Comment on attachment 455569 [details] [diff] [review]
patch

Note: the .wrappedJS case was also missing a return true;
Attachment #455569 - Flags: review?(mrbkap)
Comment on attachment 455569 [details] [diff] [review]
patch

Looks good, thanks.
Attachment #455569 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 16

7 years ago
http://hg.mozilla.org/tracemonkey/rev/d4caa61e69ab
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 17

7 years ago
Luke, this patch has a trivial id<>jsval issue. Just a heads up.
Depends on: 576774

Comment 18

7 years ago
http://hg.mozilla.org/mozilla-central/rev/efd06f813388
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 588738

Comment 19

7 years ago
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?
Keywords: dev-doc-needed
Should wrappers be showing this way in the console?

/be
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.
Keywords: dev-doc-needed → dev-doc-complete

Comment 22

6 years ago
It would really be better if someone from the people who understand these wrappers tried to explain what they are.
Keywords: dev-doc-complete → dev-doc-needed
You need to log in before you can comment on or make changes to this bug.