Bug 823283 - Don't query for JSRESOLVE_QUALIFIED when determining whether to check for an undeclared variable, when possibly adding a fast-path expando to the global object for assignment to a non-existent property. Instead examine the current bytecode to see if it's an undeclared variable access. r=bz, r=luke
authorJeff Walden <jwalden@mit.edu>
Wed, 19 Dec 2012 16:49:30 -0500
changeset 116848 8d09962f1e7995334ec1117aa8226983b5fba36a
parent 116847 847d24f0a8da938a961afeb8639a98d51faad2a2
child 116849 047534c2220799f6e74de47f0b23af1ee78df4ee
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewersbz, luke
bugs823283
milestone20.0a1
Bug 823283 - Don't query for JSRESOLVE_QUALIFIED when determining whether to check for an undeclared variable, when possibly adding a fast-path expando to the global object for assignment to a non-existent property. Instead examine the current bytecode to see if it's an undeclared variable access. r=bz, r=luke
dom/base/nsDOMClassInfo.cpp
dom/encoding/TextDecoder.h
dom/encoding/TextEncoder.h
js/src/frontend/BytecodeEmitter.cpp
js/src/jsfriendapi.h
js/src/jsobj.cpp
--- a/dom/base/nsDOMClassInfo.cpp
+++ b/dom/base/nsDOMClassInfo.cpp
@@ -7363,30 +7363,28 @@ nsWindowSH::NewResolve(nsIXPConnectWrapp
 
         if (pobj) {
           // A property was found on the prototype chain.
           *objp = pobj;
           return NS_OK;
         }
       }
 
-      // Define a fast expando, the key here is to use JS_PropertyStub
-      // as the getter/setter, which makes us stay out of XPConnect
-      // when using this property.
+      // Define a fast expando.  The key here is to use JS_PropertyStub as the
+      // getter/setter, which makes us stay out of XPConnect when using this
+      // property.
       //
-      // We don't need to worry about property attributes here as we
-      // know here we're dealing with an undefined property set, so
-      // we're not declaring readonly or permanent properties.
+      // We're adding a new property here, so we don't need to worry about
+      // conflicting with any existing ones.
       //
-      // Since we always create the undeclared property here without given a
-      // chance for the interpreter to report applicable strict mode warnings,
-      // we must take care to check those warnings here.
-      JSString *str = JSID_TO_STRING(id);
-      if ((!(flags & JSRESOLVE_QUALIFIED) &&
-           !js::CheckUndeclaredVarAssignment(cx, str)) ||
+      // Since we always create the undeclared property here, shortcutting the
+      // normal process, we go out of our way to tell the JS engine to report
+      // strict warnings/errors using js::ReportIfUndeclaredVarAssignment.
+      js::Rooted<JSString*> str(cx, JSID_TO_STRING(id));
+      if (!js::ReportIfUndeclaredVarAssignment(cx, str) ||
           !::JS_DefinePropertyById(cx, obj, id, JSVAL_VOID, JS_PropertyStub,
                                    JS_StrictPropertyStub, JSPROP_ENUMERATE)) {
         *_retval = JS_FALSE;
 
         return NS_OK;
       }
 
       *objp = obj;
--- a/js/src/frontend/BytecodeEmitter.cpp
+++ b/js/src/frontend/BytecodeEmitter.cpp
@@ -1179,16 +1179,18 @@ TryConvertToGname(BytecodeEmitter *bce, 
         return true;
     }
     if (bce->script->compileAndGo &&
         bce->hasGlobalScope &&
         !(bce->sc->isFunction && bce->sc->asFunbox()->mightAliasLocals()) &&
         !pn->isDeoptimized() &&
         !bce->sc->strict)
     {
+        // If you change anything here, you might also need to change
+        // js::CheckUndeclaredVarAssignment.
         switch (*op) {
           case JSOP_NAME:     *op = JSOP_GETGNAME; break;
           case JSOP_SETNAME:  *op = JSOP_SETGNAME; break;
           case JSOP_INCNAME:  *op = JSOP_INCGNAME; break;
           case JSOP_NAMEINC:  *op = JSOP_GNAMEINC; break;
           case JSOP_DECNAME:  *op = JSOP_DECGNAME; break;
           case JSOP_NAMEDEC:  *op = JSOP_GNAMEDEC; break;
           case JSOP_SETCONST:
--- a/js/src/jsfriendapi.h
+++ b/js/src/jsfriendapi.h
@@ -233,23 +233,24 @@ JS_FRIEND_API(JSBool) obj_defineSetter(J
 
 extern JS_FRIEND_API(bool)
 IsSystemCompartment(const JSCompartment *compartment);
 
 extern JS_FRIEND_API(bool)
 IsAtomsCompartment(const JSCompartment *c);
 
 /*
- * Check whether it is OK to assign an undeclared property with name
- * propname of the global object in the current script on cx.  Reports
- * an error if one needs to be reported (in particular in all cases
- * when it returns false).
+ * Check whether it is OK to assign an undeclared variable with the name
+ * |propname| at the current location in script.  It is not an error if there is
+ * no current script location, or if that location is not an assignment to an
+ * undeclared variable.  Reports an error if one needs to be reported (and,
+ * particularly, always reports when it returns false).
  */
 extern JS_FRIEND_API(bool)
-CheckUndeclaredVarAssignment(JSContext *cx, JSString *propname);
+ReportIfUndeclaredVarAssignment(JSContext *cx, HandleString propname);
 
 struct WeakMapTracer;
 
 /*
  * Weak map tracer callback, called once for every binding of every
  * weak map that was live at the time of the last garbage collection.
  *
  * m will be NULL if the weak map is not contained in a JS Object.
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -3469,18 +3469,18 @@ js::GetMethod(JSContext *cx, HandleObjec
     }
 #if JS_HAS_XML_SUPPORT
     if (obj->isXML())
         return js_GetXMLMethod(cx, obj, id, vp);
 #endif
     return op(cx, obj, obj, id, vp);
 }
 
-JS_FRIEND_API(bool)
-js::CheckUndeclaredVarAssignment(JSContext *cx, JSString *propname)
+static bool
+MaybeReportUndeclaredVarAssignment(JSContext *cx, JSString *propname)
 {
     {
         UnrootedScript script = cx->stack.currentScript(NULL, ContextStack::ALLOW_CROSS_COMPARTMENT);
         if (!script)
             return true;
 
         /* If neither cx nor the code is strict, then no check is needed. */
         if (!script->strict && !cx->hasStrictOption())
@@ -3492,16 +3492,66 @@ js::CheckUndeclaredVarAssignment(JSConte
            JS_ReportErrorFlagsAndNumber(cx,
                                         (JSREPORT_WARNING | JSREPORT_STRICT
                                          | JSREPORT_STRICT_MODE_ERROR),
                                         js_GetErrorMessage, NULL,
                                         JSMSG_UNDECLARED_VAR, bytes.ptr());
 }
 
 bool
+js::ReportIfUndeclaredVarAssignment(JSContext *cx, HandleString propname)
+{
+    {
+        jsbytecode *pc;
+        UnrootedScript script = cx->stack.currentScript(&pc, ContextStack::ALLOW_CROSS_COMPARTMENT);
+        if (!script)
+            return true;
+
+        /* If neither cx nor the code is strict, then no check is needed. */
+        if (!script->strict && !cx->hasStrictOption())
+            return true;
+
+        /*
+         * We only need to check for bare name mutations: we shouldn't be
+         * warning, or throwing, or whatever, if we're not doing a variable
+         * access.
+         *
+         * TryConvertToGname in frontend/BytecodeEmitter.cpp checks for rather
+         * more opcodes when it does, in the normal course of events, what this
+         * method does in the abnormal course of events.  Because we're called
+         * in narrower circumstances, we only need check two.  We don't need to
+         * check for the increment/decrement opcodes because they're no-ops:
+         * the actual semantics are implemented by desugaring.  And we don't
+         * need to check name-access because this method is only supposed to be
+         * called in assignment contexts.
+         */
+        MOZ_ASSERT(*pc != JSOP_INCNAME);
+        MOZ_ASSERT(*pc != JSOP_INCGNAME);
+        MOZ_ASSERT(*pc != JSOP_NAMEINC);
+        MOZ_ASSERT(*pc != JSOP_GNAMEINC);
+        MOZ_ASSERT(*pc != JSOP_DECNAME);
+        MOZ_ASSERT(*pc != JSOP_DECGNAME);
+        MOZ_ASSERT(*pc != JSOP_NAMEDEC);
+        MOZ_ASSERT(*pc != JSOP_GNAMEDEC);
+        MOZ_ASSERT(*pc != JSOP_NAME);
+        MOZ_ASSERT(*pc != JSOP_GETGNAME);
+        if (*pc != JSOP_SETNAME && *pc != JSOP_SETGNAME)
+            return true;
+    }
+
+    JSAutoByteString bytes(cx, propname);
+    return !!bytes &&
+           JS_ReportErrorFlagsAndNumber(cx,
+                                        JSREPORT_WARNING | JSREPORT_STRICT |
+                                        JSREPORT_STRICT_MODE_ERROR,
+                                        js_GetErrorMessage, NULL,
+                                        JSMSG_UNDECLARED_VAR, bytes.ptr());
+}
+
+bool
 JSObject::reportReadOnly(JSContext *cx, jsid id, unsigned report)
 {
     RootedValue val(cx, IdToValue(id));
     return js_ReportValueErrorFlags(cx, report, JSMSG_READ_ONLY,
                                     JSDVG_IGNORE_STACK, val, NullPtr(),
                                     NULL, NULL);
 }
 
@@ -3581,18 +3631,19 @@ baseops::SetPropertyHelper(JSContext *cx
             shape = NULL;
         }
     } else {
         /* We should never add properties to lexical blocks. */
         JS_ASSERT(!obj->isBlock());
 
         if (obj->isGlobal() &&
             (defineHow & DNP_UNQUALIFIED) &&
-            !js::CheckUndeclaredVarAssignment(cx, JSID_TO_STRING(id))) {
-            return JS_FALSE;
+            !MaybeReportUndeclaredVarAssignment(cx, JSID_TO_STRING(id)))
+        {
+            return false;
         }
     }
 
     /*
      * Now either shape is null, meaning id was not found in obj or one of its
      * prototypes; or shape is non-null, meaning id was found directly in pobj.
      */
     attrs = JSPROP_ENUMERATE;