[inspector] Throw during debug-eval when accessing function prototypes

Function prototypes can be lazily allocated. This means they go into the
temporary objects set that debug-eval uses to figure out if a write
will be side-effect free.

We were incorrectly classifying writes to function prototypes as
side-effect free because the prototype happened to be lazily allocated
when we first accessed it during debug-eval, but was actually reachable
from the function (not allocated temporarily).

To do this we introduced a way to temporarily turn off the temporary
object tracking, and we use it when lazily allocating function
prototypes.

This could mean that we incorrectly report side-effects when writing to
function prototypes for functions which were themselves created during
debug-eval side-effect free mode. However, it's unclear if this is a
problem, because function declarations set global variables which would
already throw due to side-effects.

Bug: chromium:1154193
Change-Id: I444a673662095f6deabaafdce3cdf3d86b71446d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2581968Reviewed-by: Simon Zünd <szuend@chromium.org>
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71692}

src/builtins/accessors.cc

@@ -309,6 +309,12 @@ Handle<AccessorInfo> Accessors::MakeStringLengthInfo(Isolate* isolate) {
 static Handle<Object> GetFunctionPrototype(Isolate* isolate,
                                            Handle<JSFunction> function) {
   if (!function->has_prototype()) {
+    // We lazily allocate .prototype for functions, which confuses debug
+    // evaluate which assumes we can write to temporary objects we allocated
+    // during evaluation. We err on the side of caution here and prevent the
+    // newly allocated prototype from going into the temporary objects set,
+    // which means writes to it will be considered a side effect.
+    DisableTemporaryObjectTracking no_temp_tracking(isolate->debug());
     Handle<JSObject> proto = isolate->factory()->NewFunctionPrototype(function);
     JSFunction::SetPrototype(function, proto);
   }

src/debug/debug.cc

@@ -50,7 +50,10 @@ class Debug::TemporaryObjectsTracker : public HeapObjectAllocationTracker {
   TemporaryObjectsTracker(const TemporaryObjectsTracker&) = delete;
   TemporaryObjectsTracker& operator=(const TemporaryObjectsTracker&) = delete;
 
-  void AllocationEvent(Address addr, int) override { objects_.insert(addr); }
+  void AllocationEvent(Address addr, int) override {
+    if (disabled) return;
+    objects_.insert(addr);
+  }
 
   void MoveEvent(Address from, Address to, int) override {
     if (from == to) return;
@@ -79,6 +82,8 @@ class Debug::TemporaryObjectsTracker : public HeapObjectAllocationTracker {
     return objects_.find(obj->address()) != objects_.end();
   }
 
+  bool disabled = false;
+
  private:
   std::unordered_set<Address> objects_;
   base::Mutex mutex_;
@@ -2475,5 +2480,19 @@ bool Debug::PerformSideEffectCheckForObject(Handle<Object> object) {
   isolate_->TerminateExecution();
   return false;
 }
+
+void Debug::SetTemporaryObjectTrackingDisabled(bool disabled) {
+  if (temporary_objects_) {
+    temporary_objects_->disabled = disabled;
+  }
+}
+
+bool Debug::GetTemporaryObjectTrackingDisabled() const {
+  if (temporary_objects_) {
+    return temporary_objects_->disabled;
+  }
+  return false;
+}
+
 }  // namespace internal
 }  // namespace v8

src/debug/debug.h

@@ -464,6 +464,9 @@ class V8_EXPORT_PRIVATE Debug {
                      DebugInfoListNode** curr);
   void FreeDebugInfoListNode(DebugInfoListNode* prev, DebugInfoListNode* node);
 
+  void SetTemporaryObjectTrackingDisabled(bool disabled);
+  bool GetTemporaryObjectTrackingDisabled() const;
+
   debug::DebugDelegate* debug_delegate_ = nullptr;
 
   // Debugger is active, i.e. there is a debug event listener attached.
@@ -556,6 +559,7 @@ class V8_EXPORT_PRIVATE Debug {
   friend class Isolate;
   friend class DebugScope;
   friend class DisableBreak;
+  friend class DisableTemporaryObjectTracking;
   friend class LiveEdit;
   friend class SuppressDebug;
 
@@ -613,6 +617,28 @@ class DisableBreak {
   bool previous_break_disabled_;
 };
 
+// Stack allocated class for disabling temporary object tracking.
+class DisableTemporaryObjectTracking {
+ public:
+  explicit DisableTemporaryObjectTracking(Debug* debug)
+      : debug_(debug),
+        previous_tracking_disabled_(
+            debug->GetTemporaryObjectTrackingDisabled()) {
+    debug_->SetTemporaryObjectTrackingDisabled(true);
+  }
+  ~DisableTemporaryObjectTracking() {
+    debug_->SetTemporaryObjectTrackingDisabled(previous_tracking_disabled_);
+  }
+  DisableTemporaryObjectTracking(const DisableTemporaryObjectTracking&) =
+      delete;
+  DisableTemporaryObjectTracking& operator=(
+      const DisableTemporaryObjectTracking&) = delete;
+
+ private:
+  Debug* debug_;
+  bool previous_tracking_disabled_;
+};
+
 class SuppressDebug {
  public:
   explicit SuppressDebug(Debug* debug)

test/inspector/runtime/evaluate-without-side-effects-expected.txt

@@ -10,6 +10,33 @@ Test throwOnSideEffect: false
         }
     }
 }
+Test prototype extension expression with side-effect, with throwOnSideEffect: true
+{
+    id : <messageId>
+    result : {
+        exceptionDetails : {
+            columnNumber : -1
+            exception : {
+                className : EvalError
+                description : EvalError: Possible side-effect in debug-evaluate
+                objectId : <objectId>
+                subtype : error
+                type : object
+            }
+            exceptionId : <exceptionId>
+            lineNumber : -1
+            scriptId : <scriptId>
+            text : Uncaught
+        }
+        result : {
+            className : EvalError
+            description : EvalError: Possible side-effect in debug-evaluate
+            objectId : <objectId>
+            subtype : error
+            type : object
+        }
+    }
+}
 Test expression with side-effect, with throwOnSideEffect: true
 {
     id : <messageId>

test/inspector/runtime/evaluate-without-side-effects.js

@@ -24,6 +24,12 @@ Protocol.Debugger.onPaused(message => {
     throwOnSideEffect: false
   }));
 
+  InspectorTest.log("Test prototype extension expression with side-effect, with throwOnSideEffect: true");
+  InspectorTest.logMessage(await Protocol.Runtime.evaluate({
+    expression: "f.prototype.test = () => console.log('test fn');",
+    throwOnSideEffect: true
+  }));
+
   InspectorTest.log("Test expression with side-effect, with throwOnSideEffect: true");
   InspectorTest.logMessage(await Protocol.Runtime.evaluate({
     expression: "x = 3; x;",