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

src/ast/variables.h

Index: src/ast/variables.h
diff --git a/src/ast/variables.h b/src/ast/variables.h
index 7d54bc09c4d8d77d52f2b53ba8b398d6bd50c499..620ffc287aec55421b8b39e6ff10596722b5190b 100644
--- a/src/ast/variables.h
+++ b/src/ast/variables.h
@@ -55,10 +55,6 @@ class Variable: public ZoneObject {
   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;
   }
@@ -90,13 +86,20 @@ class Variable: public ZoneObject {
   // 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 {
@@ -122,6 +125,18 @@ class Variable: public ZoneObject {
   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_;