TM: implement new chrome wrappers (aka COW)

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

Other Branch
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 8 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

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

Comment 2

9 years ago
Created attachment 453946 [details] [diff] [review]
patch
Attachment #453945 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #453946 - Flags: review?(mrbkap)
(Assignee)

Comment 3

9 years ago
Comment on attachment 453946 [details] [diff] [review]
patch

Actually this is incomplete. I have to process props and iterators, too.
Attachment #453946 - Flags: review?(mrbkap)
(Assignee)

Comment 4

9 years ago
This patch is a bit bigger than I was hoping for. It contains a correctness fix for proxies, which didn't support foreach in the default iterator hook implementation (which delegates to enumerate()). For Chrome Wrappers we always bypass the iterate hook and go for enumerate() behavior, which enforces that the filtering in CheckAccess always happens.
(Assignee)

Comment 5

9 years ago
The patch also has a fast path for iteration across compartment boundaries. Non-escaping iterator objects are reified instead of being wrapped. This avoids asking them for their compartment, which currently crashes since we are trying to walk the parent chain (non-escaping objects have none).

Test case: for each(y in (evalcx(''))) {}
(Assignee)

Comment 6

9 years ago
Created attachment 453980 [details] [diff] [review]
patch
Attachment #453946 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #453980 - Flags: review?(mrbkap)
(Assignee)

Updated

9 years ago
Duplicate of this bug: 574262
(Assignee)

Comment 8

9 years ago
Created attachment 453986 [details] [diff] [review]
patch

Extra bonus: split ContentWrapper into ContentWrapper and OpaqueWrapper and perform the principals check in WrapperFactory at wrapper creation time. If we don't have access, the OpaqueWrapper always denies access. The ContentWrapper only pushes the target compartment's principals but doesn't check them (we already did that).
Attachment #453980 - Attachment is obsolete: true
Attachment #453980 - Flags: review?(mrbkap)
(Assignee)

Comment 9

9 years ago
Created attachment 453987 [details] [diff] [review]
patch
Attachment #453986 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #453987 - Flags: review?(mrbkap)
(Assignee)

Comment 10

9 years ago
Created attachment 454056 [details] [diff] [review]
patch

Remove the opaque wrappers. We don't need them. The wrapper factory now automatically decides what wrappers to apply (starting with chrome or content wrappers). x-ray and crossorigin will be filter wrappers, to be added separately.
Attachment #453987 - Attachment is obsolete: true
Attachment #453987 - Flags: review?(mrbkap)
(Assignee)

Updated

9 years ago
Attachment #454056 - Flags: review?(mrbkap)
What are x-ray wrappers?

/be
(Assignee)

Comment 12

9 years ago
A more thorough writeup will follow once the architecture is stable, but x-ray wrappers bypass scripted code and go straight to the native code (NativeWrapper is the previous name). The new wrappers compose and do less than NativeWrapper, hence the different name.
Comment on attachment 454056 [details] [diff] [review]
patch

Please search for |if(| and make any occurances |if (|.

diff --git a/js/src/jsiter.cpp b/js/src/jsiter.cpp
+static bool
+IdToIteratorValue(JSContext *cx, JSObject *obj, jsid id, uintN flags, jsval *vp)
+{
+    if (!(flags & JSITER_FOREACH)) {
+        *vp = ID_TO_VALUE(id);
+        return true;
+    }
+    /* Do the lookup on the original object instead of the prototype. */

Nit: newline above the comment.

diff --git a/js/src/jswrapper.cpp b/js/src/jswrapper.cpp
+static bool
+Reify(JSContext *cx, JSCompartment *origin, jsval *vp)
+{
+    JSObject *iterObj = JSVAL_TO_OBJECT(*vp);
+    NativeIterator *ni = iterObj->getNativeIterator();
+    AutoValueVector props(cx);
+    size_t length = ni->length();
+    if (length > 0) {
+        props.resize(length);
+        for (size_t n = 0; n < length; ++n)
+            props[n] = origin->wrap(cx, &ni->begin()[n]);
+    }
+    JSObject *obj = ni->obj;

Nit: newline after the }.

diff --git a/js/src/xpconnect/src/wrappers/AccessCheck.cpp b/js/src/xpconnect/src/wrappers/AccessCheck.cpp
+bool
+AccessCheck::subsumes(JSCompartment *subject, JSCompartment *object, bool *yesno)
+{
+    nsIScriptSecurityManager *ssm = XPCWrapper::GetSecurityManager();
+    if(!ssm) {
+        return true;
+    }
+    *yesno = subject->principals->subsume(subject->principals, object->principals);

Does this leave |yesno| uninitialized if !ssm?

+bool
+AccessCheck::isChrome(JSCompartment *compartment)
+{
+    JSPrincipals *principals = compartment->principals;
+    nsIPrincipal *principal = static_cast<nsJSPrincipals *>(principals)->nsIPrincipalPtr;

Seems like it might be worth having an

nsIPrincipal *
GetCompartmentPrincipal(JSCompartment *compartment)

to hide the static_cast ugliness (and maybe nuke a couple local variables, too).

diff --git a/js/src/xpconnect/src/wrappers/ChromeWrapper.cpp b/js/src/xpconnect/src/wrappers/ChromeWrapper.cpp
+typedef enum { READ = 1<<0, WRITE = 1<<1, DENIED=0 } Permission;

Nit: spaces around the left-shift operator.

+static bool
+CheckAccess(JSContext *cx, JSObject *hallpass, jsid id, JSCrossCompartmentWrapper::Mode mode, bool *yesno = NULL)

Nit: please split this over multiple lines.

diff --git a/js/src/xpconnect/src/wrappers/Makefile.in b/js/src/xpconnect/src/wrappers/Makefile.in
@@ -42,14 +42,17 @@ VPATH		= @srcdir@
 
 include $(DEPTH)/config/autoconf.mk
 
 MODULE		= xpcwrappers
 LIBRARY_NAME	= xpcwrappers_s
 FORCE_STATIC_LIB = 1
 LIBXUL_LIBRARY = 1
 
-CPPSRCS		= ContentWrapper.cpp
+CPPSRCS		= ContentWrapper.cpp \
+                  ChromeWrapper.cpp \
+                  AccessCheck.cpp \
+                  WrapperFactory.cpp

Use hard tabs here.

looks good otherwise. r=me with my nits picked and questions answered.
Attachment #454056 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 14

9 years ago
Removed the code the question was targeting.

http://hg.mozilla.org/tracemonkey/rev/57f85cb4d91e
Whiteboard: fixed-in-tracemonkey
> Does this leave |yesno| uninitialized if !ssm?

In the name of that which does not suck, s/yesno/answer/g!

/be
(Assignee)

Comment 16

9 years ago
Created attachment 455290 [details] [diff] [review]
patch
Attachment #454056 - Attachment is obsolete: true
(Assignee)

Comment 17

9 years ago
Created attachment 455298 [details] [diff] [review]
patch
Attachment #455290 - Attachment is obsolete: true
Attachment #455298 - Flags: review?(mrbkap)
(Assignee)

Comment 18

9 years ago
Created attachment 455343 [details] [diff] [review]
patch

Fixed a bunch of linker nastiness.
Attachment #455298 - Attachment is obsolete: true
Attachment #455343 - Flags: review?(mrbkap)
Attachment #455298 - Flags: review?(mrbkap)
(Assignee)

Comment 19

9 years ago
Comment on attachment 455343 [details] [diff] [review]
patch

Wrong bug.
Attachment #455343 - Attachment is obsolete: true
Attachment #455343 - Attachment is patch: false
Attachment #455343 - Flags: review?(mrbkap)
(Assignee)

Updated

9 years ago
Attachment #455343 - Attachment is patch: true
(Assignee)

Updated

9 years ago
Attachment #454056 - Attachment is obsolete: false

Comment 20

9 years ago
http://hg.mozilla.org/mozilla-central/rev/57f85cb4d91e
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.