Check for "SuperNotCalled" on "delete this" in a constructor

V8 implements "delete this" as "LdaTrue", but an error needs to be thrown
if done in a constructor before calling super. ThrowIfHole checks the
accumulator, so we need to load 'this' into the accumulator. The check is
inserted by the load since it has HoleCheckMode::kRequired

Bug: https://bugs.chromium.org/p/v8/issues/detail?id=6711

Change-Id: I9f2ce4439505cec4327d88d1195898782edea721
Reviewed-on: https://chromium-review.googlesource.com/c/1419084
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Commit-Queue: Matt Gardner <magardn@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#59007}

src/interpreter/bytecode-generator.cc

@@ -4573,56 +4573,51 @@ void BytecodeGenerator::VisitUnaryOperation(UnaryOperation* expr) {
   }
 }
 
-void BytecodeGenerator::VisitDelete(UnaryOperation* expr) {
-  if (expr->expression()->IsProperty()) {
+void BytecodeGenerator::VisitDelete(UnaryOperation* unary) {
+  Expression* expr = unary->expression();
+  if (expr->IsProperty()) {
     // Delete of an object property is allowed both in sloppy
     // and strict modes.
-    Property* property = expr->expression()->AsProperty();
+    Property* property = expr->AsProperty();
     Register object = VisitForRegisterValue(property->obj());
     VisitForAccumulatorValue(property->key());
     builder()->Delete(object, language_mode());
-  } else if (expr->expression()->IsVariableProxy()) {
+  } else if (expr->IsVariableProxy() && !expr->AsVariableProxy()->is_this() &&
+             !expr->AsVariableProxy()->is_new_target()) {
     // Delete of an unqualified identifier is allowed in sloppy mode but is
-    // not allowed in strict mode. Deleting 'this' and 'new.target' is allowed
-    // in both modes.
-    VariableProxy* proxy = expr->expression()->AsVariableProxy();
-    DCHECK(is_sloppy(language_mode()) || proxy->is_this() ||
-           proxy->is_new_target());
-    if (proxy->is_this() || proxy->is_new_target()) {
-      builder()->LoadTrue();
-    } else {
-      Variable* variable = proxy->var();
-      switch (variable->location()) {
-        case VariableLocation::PARAMETER:
-        case VariableLocation::LOCAL:
-        case VariableLocation::CONTEXT: {
-          // Deleting local var/let/const, context variables, and arguments
-          // does not have any effect.
-          builder()->LoadFalse();
-          break;
-        }
-        case VariableLocation::UNALLOCATED:
-        // TODO(adamk): Falling through to the runtime results in correct
-        // behavior, but does unnecessary context-walking (since scope
-        // analysis has already proven that the variable doesn't exist in
-        // any non-global scope). Consider adding a DeleteGlobal bytecode
-        // that knows how to deal with ScriptContexts as well as global
-        // object properties.
-        case VariableLocation::LOOKUP: {
-          Register name_reg = register_allocator()->NewRegister();
-          builder()
-              ->LoadLiteral(variable->raw_name())
-              .StoreAccumulatorInRegister(name_reg)
-              .CallRuntime(Runtime::kDeleteLookupSlot, name_reg);
-          break;
-        }
-        default:
-          UNREACHABLE();
+    // not allowed in strict mode.
+    DCHECK(is_sloppy(language_mode()));
+    Variable* variable = expr->AsVariableProxy()->var();
+    switch (variable->location()) {
+      case VariableLocation::PARAMETER:
+      case VariableLocation::LOCAL:
+      case VariableLocation::CONTEXT: {
+        // Deleting local var/let/const, context variables, and arguments
+        // does not have any effect.
+        builder()->LoadFalse();
+        break;
       }
+      case VariableLocation::UNALLOCATED:
+      // TODO(adamk): Falling through to the runtime results in correct
+      // behavior, but does unnecessary context-walking (since scope
+      // analysis has already proven that the variable doesn't exist in
+      // any non-global scope). Consider adding a DeleteGlobal bytecode
+      // that knows how to deal with ScriptContexts as well as global
+      // object properties.
+      case VariableLocation::LOOKUP: {
+        Register name_reg = register_allocator()->NewRegister();
+        builder()
+            ->LoadLiteral(variable->raw_name())
+            .StoreAccumulatorInRegister(name_reg)
+            .CallRuntime(Runtime::kDeleteLookupSlot, name_reg);
+        break;
+      }
+      default:
+        UNREACHABLE();
     }
   } else {
-    // Delete of an unresolvable reference returns true.
-    VisitForEffect(expr->expression());
+    // Delete of an unresolvable reference, new.target, and this returns true.
+    VisitForEffect(expr);
     builder()->LoadTrue();
   }
 }

test/cctest/interpreter/bytecode_expectations/Delete.golden

@@ -137,3 +137,20 @@ constant pool: [
 handlers: [
 ]
 
+---
+snippet: "
+  return delete this;
+"
+frame size: 0
+parameter count: 1
+bytecode array length: 3
+bytecodes: [
+  /*   30 E> */ B(StackCheck),
+  /*   34 S> */ B(LdaTrue),
+  /*   53 S> */ B(Return),
+]
+constant pool: [
+]
+handlers: [
+]
+

test/cctest/interpreter/test-bytecode-generator.cc

@@ -1499,6 +1499,8 @@ TEST(Delete) {
       "return delete a[1];\n",
 
       "return delete 'test';\n",
+
+      "return delete this;\n",
   };
 
   CHECK(CompareTexts(BuildActual(printer, snippets),

test/mjsunit/regress/regress-6711.js

@@ -0,0 +1,21 @@
+// Copyright 2019 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.
+
+// ensure `delete this` throws before `super` is called.
+assertThrows(()=>{
+  new class extends Object {
+    constructor() {
+      delete this;
+      super();
+    }
+  }
+}, ReferenceError);
+
+// ensure `delete this` doesn't throw after `super` is called.
+new class extends Object {
+  constructor() {
+    super();
+    delete this;
+  }
+}