574539 - TM: implement new chrome wrappers (aka COW)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Attachment: patch

Comment 3

[Andreas Gal :gal]

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

Actually this is incomplete. I have to process props and iterators, too.

Comment 4

[Andreas Gal :gal]

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.

Comment 5

[Andreas Gal :gal]

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(''))) {}

Comment 6

[Andreas Gal :gal]

Attachment: patch

Comment 8

[Andreas Gal :gal]

Attachment: 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).

Comment 9

[Andreas Gal :gal]

Attachment: patch

Comment 10

[Andreas Gal :gal]

Attachment: 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.

Comment 11

[Brendan Eich [:brendan]]

What are x-ray wrappers?

/be

Comment 12

[Andreas Gal :gal]

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 13

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 14

[Andreas Gal :gal]

Removed the code the question was targeting.

http://hg.mozilla.org/tracemonkey/rev/57f85cb4d91e

Comment 15

[Brendan Eich [:brendan]]

> Does this leave |yesno| uninitialized if !ssm?

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

/be

Comment 16

[Andreas Gal :gal]

Attachment: patch

Comment 17

[Andreas Gal :gal]

Attachment: patch

Comment 18

[Andreas Gal :gal]

Attachment: patch

Fixed a bunch of linker nastiness.

Comment 19

[Andreas Gal :gal]

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

Wrong bug.

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/57f85cb4d91e