[Interpreter] Avoid dereferencing handles on BytecodeGenerator for AST operations.

src/ast/variables.h

 // Copyright 2011 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef V8_AST_VARIABLES_H_
 #define V8_AST_VARIABLES_H_
 
 #include "src/ast/ast-value-factory.h"
 #include "src/zone.h"
 
@@ skipping 37 matching lines @@
     force_context_allocation_ = true;
   }
   bool is_used() { return is_used_; }
   void set_is_used() { is_used_ = true; }
   MaybeAssignedFlag maybe_assigned() const { return maybe_assigned_; }
   void set_maybe_assigned() { maybe_assigned_ = kMaybeAssigned; }
 
   int initializer_position() { return initializer_position_; }
   void set_initializer_position(int pos) { initializer_position_ = pos; }
 
-  bool IsVariable(Handle<String> n) const {
-    return !is_this() && name().is_identical_to(n);
-  }
-
   bool IsUnallocated() const {
     return location_ == VariableLocation::UNALLOCATED;
   }
   bool IsParameter() const { return location_ == VariableLocation::PARAMETER; }
   bool IsStackLocal() const { return location_ == VariableLocation::LOCAL; }
   bool IsStackAllocated() const { return IsParameter() || IsStackLocal(); }
   bool IsContextSlot() const { return location_ == VariableLocation::CONTEXT; }
   bool IsGlobalSlot() const { return location_ == VariableLocation::GLOBAL; }
   bool IsUnallocatedOrGlobalSlot() const {
     return IsUnallocated() || IsGlobalSlot();
@@ skipping 11 matching lines @@
   bool is_function() const { return kind_ == FUNCTION; }
   bool is_this() const { return kind_ == THIS; }
   bool is_arguments() const { return kind_ == ARGUMENTS; }
 
   // For script scopes, the "this" binding is provided by a ScriptContext added
   // to the global's ScriptContextTable.  This binding might not statically
   // resolve to a Variable::THIS binding, instead being DYNAMIC_LOCAL.  However
   // any variable named "this" does indeed refer to a Variable::THIS binding;
   // the grammar ensures this to be the case.  So wherever a "this" binding
   // might be provided by the global, use HasThisName instead of is_this().
-  bool HasThisName(Isolate* isolate) const {
-    return is_this() || *name() == *isolate->factory()->this_string();
+  bool HasThisName(Isolate* isolate,
+                   HandleDereferenceMode deref_mode =
+                       HandleDereferenceMode::kHandleDereferenceAllowed) const {
+    return is_this() ||
+           name_is_identical_to(isolate->factory()->this_string(), deref_mode);

[marja, 2016/08/08 08:20:13]

If handle dereferencing is not allowed (i.e., we are on a background thread), it
looks scary that we do "isolate->factory()->this_string()". The conservative
approach would be to not access Isolate / Heap at all. (That's what the
streaming parser does.) Does Ignition take a different approach, with some
reasoning that reading data from Isolate / Heap is ok because <something>?

Also, it's nontrivial to see what guarantees that
isolate->factory()->this_string() is affected by the CanonicalHandleScope. (Just
reading the comments above CanonicalHandleScope... I don't know the
implementation details..) It says "It changes the existing HandleScope so that
Handles created within are canonicalized.". But is this_string() in the previous
HandleScope? Who guarantees that there are no HandleScopes inbetween (and there
will never be)? What does this mean for global handles?

[rmcilroy, 2016/08/08 13:05:56]

I believe it is OK to access constant heap roots off the main thread because
these will never change (they are in the immortal immovable heap page and are
constant, so the object pointer will never change. There is special casing in
HandleBase::IsDereferenceAllowed for these types of values for this reason
(otherwise this CL would crash here since it is called from within a
DisallowHandleDereferenceScope(). 
Yes sorry that comment didn't make this too clear. The CanonicalHandleScope uses
the root handle for any handles pointing to valid roots created within the
scope, so this guarantees that any Handles which point to
isolate->factory()->this_string() will all have the same root handle location. 

Updated the comments to clarify these points.

   }
 
   // True if the variable is named eval and not known to be shadowed.
-  bool is_possibly_eval(Isolate* isolate) const {
-    return IsVariable(isolate->factory()->eval_string());
+  bool is_possibly_eval(
+      Isolate* isolate,
+      HandleDereferenceMode deref_mode =
+          HandleDereferenceMode::kHandleDereferenceAllowed) const {
+    return !is_this() &&
+           name_is_identical_to(isolate->factory()->eval_string(), deref_mode);
   }
 
   Variable* local_if_not_shadowed() const {
     DCHECK(mode_ == DYNAMIC_LOCAL && local_if_not_shadowed_ != NULL);
     return local_if_not_shadowed_;
   }
 
   void set_local_if_not_shadowed(Variable* local) {
     local_if_not_shadowed_ = local;
   }
 
   VariableLocation location() const { return location_; }
   int index() const { return index_; }
   InitializationFlag initialization_flag() const {
     return initialization_flag_;
   }
 
   void AllocateTo(VariableLocation location, int index) {
     location_ = location;
     index_ = index;
   }
 
   static int CompareIndex(Variable* const* v, Variable* const* w);
 
  private:
+  bool name_is_identical_to(Handle<Object> object,
+                            HandleDereferenceMode deref_mode) const {
+    if (deref_mode == HandleDereferenceMode::kHandleDereferenceAllowed) {
+      return *name() == *object;
+    } else {
+      // If handle dereference isn't allowed use the handle address for
+      // identity. This depends on the variable name being internalized in a
+      // CanonicalHandleScope.
+      return name().address() == object.address();
+    }
+  }
+
   Scope* scope_;
   const AstRawString* name_;
   VariableMode mode_;
   Kind kind_;
   VariableLocation location_;
   int index_;
   int initializer_position_;
 
   // If this field is set, this variable references the stored locally bound
   // variable, but it might be shadowed by variable bindings introduced by
   // sloppy 'eval' calls between the reference scope (inclusive) and the
   // binding scope (exclusive).
   Variable* local_if_not_shadowed_;
 
   // Usage info.
   bool force_context_allocation_;  // set by variable resolver
   bool is_used_;
   InitializationFlag initialization_flag_;
   MaybeAssignedFlag maybe_assigned_;
 };
 }  // namespace internal
 }  // namespace v8
 
 #endif  // V8_AST_VARIABLES_H_