Bug 1662559 - Part 13: Disallow invocation of private methods inside of evalInFrame r=arai

In order to correctly support this with the optimized storage for methods we
would need new bytecode like GetAliasedVar which is able to traverse
non-EnvironmentObject environments like DebugEnvironmentProxy.

Unfortunately, for safety this means we will ultimately disable this for a number of
different private field and accessor invocations.

Differential Revision: https://phabricator.services.mozilla.com/D109402

js/public/friend/ErrorNumbers.msg

@@ -560,6 +560,7 @@ MSG_DEF(JSMSG_DEBUG_PROMISE_NOT_RESOLVED, 0, JSEXN_TYPEERR, "Promise hasn't been
 MSG_DEF(JSMSG_DEBUG_PROMISE_NOT_FULFILLED, 0, JSEXN_TYPEERR, "Promise hasn't been fulfilled")
 MSG_DEF(JSMSG_DEBUG_PROMISE_NOT_REJECTED, 0, JSEXN_TYPEERR, "Promise hasn't been rejected")
 MSG_DEF(JSMSG_DEBUG_NO_BINARY_SOURCE,  0, JSEXN_ERR, "WebAssembly binary source is not available")
+MSG_DEF(JSMSG_DEBUG_NO_PRIVATE_METHOD, 0, JSEXN_ERR, "private method calls aren't available in this context")
 
 // Testing functions
 MSG_DEF(JSMSG_TESTING_SCRIPTS_ONLY, 0, JSEXN_TYPEERR, "only works on scripts")

js/src/frontend/BytecodeEmitter.cpp

@@ -201,9 +201,10 @@ NameLocation BytecodeEmitter::lookupName(TaggedParserAtomIndex name) {
   return innermostEmitterScope()->lookup(this, name);
 }
 
-NameLocation BytecodeEmitter::lookupPrivate(TaggedParserAtomIndex name,
+bool BytecodeEmitter::lookupPrivate(TaggedParserAtomIndex name,
-                                            Maybe<NameLocation>& brandLoc) {
+                                    NameLocation& loc,
-  return innermostEmitterScope()->lookupPrivate(this, name, brandLoc);
+                                    Maybe<NameLocation>& brandLoc) {
+  return innermostEmitterScope()->lookupPrivate(this, name, loc, brandLoc);
 }
 
 Maybe<NameLocation> BytecodeEmitter::locationOfNameBoundInScope(

js/src/frontend/BytecodeEmitter.h

@@ -226,9 +226,9 @@ struct MOZ_STACK_CLASS BytecodeEmitter {
 
   NameLocation lookupName(TaggedParserAtomIndex name);
 
-  // See EmitterScope::lookupPrivate for details around brandLoc.
+  // See EmitterScope::lookupPrivate for details around brandLoc
-  NameLocation lookupPrivate(TaggedParserAtomIndex name,
+  bool lookupPrivate(TaggedParserAtomIndex name, NameLocation& loc,
-                             mozilla::Maybe<NameLocation>& brandLoc);
+                     mozilla::Maybe<NameLocation>& brandLoc);
 
   // To implement Annex B and the formal parameter defaults scope semantics
   // requires accessing names that would otherwise be shadowed. This method

js/src/frontend/EmitterScope.cpp

@@ -990,15 +990,43 @@ NameLocation EmitterScope::lookup(BytecodeEmitter* bce,
   return searchAndCache(bce, name);
 }
 
-NameLocation EmitterScope::lookupPrivate(
+bool EmitterScope::lookupPrivate(BytecodeEmitter* bce,
-    BytecodeEmitter* bce, TaggedParserAtomIndex name,
+                                 TaggedParserAtomIndex name, NameLocation& loc,
-    mozilla::Maybe<NameLocation>& brandLoc) {
+                                 mozilla::Maybe<NameLocation>& brandLoc) {
-  NameLocation loc = lookup(bce, name);
+  loc = lookup(bce, name);
 
-  // The parser ensures that `name` is the name of a private member in the
+  // Private Brand checking relies on the ability to construct a new
-  // current scope. Since classes are strict mode code, we're certain to find
+  // environment coordinate for a name at a fixed offset, which will
-  // that scope statically; there will be no intervening dynamic scope.
+  // correspond to the private brand for that class.
-  MOZ_RELEASE_ASSERT(loc.kind() == NameLocation::Kind::EnvironmentCoordinate);
+  //
+  // Despite easily being able to gather the information required to
+  // synthesize an environment coordinate (see cachePrivateFieldsForEval, and
+  // Bug 1638309), we don't have bytecode that is able to bypass the
+  // DebugEnvironmentProxy on the environment to load the right value. As a
+  // result, we cannot currently correctly brand check a private method.
+  //
+  // See also Bug 793345 which argues that we should remove the
+  // DebugEnvironmentProxy.
+  if (loc.kind() != NameLocation::Kind::EnvironmentCoordinate) {
+    MOZ_ASSERT(loc.kind() == NameLocation::Kind::Dynamic ||
+               loc.kind() == NameLocation::Kind::Global);
+    // Private fields don't require brand checking and can be correctly
+    // code-generated with dynamic name lookup bytecode we have today.
+    //
+    // In an ideal world, we could check if we have a private field or a private
+    // method, relying  on the private field eval cache; if we have dynamic or
+    // global name location kind, and we successfully parsed the private
+    // identifier, then it must be in the eval cache, which tracks binding kind.
+    // However, we don't initialize the eval cache on delazifications that are
+    // nested inside an eval execution, so we can't rely on it.
+    //
+    // Instead, at this point we throw up our hands and tell the user their
+    // invocation isn't supported (even if it's actually the invocation of a
+    // private field, which conceptually -should- work, but we cannot
+    // disambiguate from a private method invocation without the binding kind.)
+    bce->reportError(nullptr, JSMSG_DEBUG_NO_PRIVATE_METHOD);
+    return false;
+  }
 
   if (loc.bindingKind() == BindingKind::PrivateMethod) {
     brandLoc = Some(NameLocation::EnvironmentCoordinate(
@@ -1007,7 +1035,7 @@ NameLocation EmitterScope::lookupPrivate(
   } else {
     brandLoc = Nothing();
   }
-  return loc;
+  return true;
 }
 
 Maybe<NameLocation> EmitterScope::locationBoundInScope(

js/src/frontend/EmitterScope.h

@@ -193,8 +193,10 @@ class EmitterScope : public Nestable<EmitterScope> {
   // target object (anything other than a non-static private method), then
   // `brandLoc` is set to Nothing.
   //
-  NameLocation lookupPrivate(BytecodeEmitter* bce, TaggedParserAtomIndex name,
+  // To handle cases where it's not possible to find the private brand, this
-                             mozilla::Maybe<NameLocation>& brandLoc);
+  // method has to be fallible.
+  bool lookupPrivate(BytecodeEmitter* bce, TaggedParserAtomIndex name,
+                     NameLocation& loc, mozilla::Maybe<NameLocation>& brandLoc);
 
   mozilla::Maybe<NameLocation> locationBoundInScope(TaggedParserAtomIndex name,
                                                     EmitterScope* target);