Bug 1647342 - Part 1: Remove FrameIter::Data from DebuggerFrame public API. r=arai! draft
authorLogan Smyth <loganfsmyth@gmail.com>
Wed, 24 Jun 2020 00:01:06 +0000
changeset 3000024 532ef3b8baaea38204c1dabf05151a793de4266b
parent 3000023 bbdb1d53384cb07fe324114cc2d3f4ec42a319e9
child 3000025 13cd016748a2c43c42bbb14c2dd8002c1cdb39ad
push id558604
push userreviewbot
push dateWed, 24 Jun 2020 00:01:54 +0000
treeherdertry@997f6b6c671f [default view] [failures only]
reviewersarai
bugs1647342
milestone79.0a1
Bug 1647342 - Part 1: Remove FrameIter::Data from DebuggerFrame public API. r=arai! Summary: Exposing frameIterData() and setFrameIterData() isn't great since it makes it harder to follow what state transitions for frame data we expect the frame to have. By hiding this, it helps clarify that frame data can be cleared or replaced, but public usage of the FrameIter::Data object itself isn't expected outside of the class itself. Differential Revision: https://phabricator.services.mozilla.com/D80734 Depends on D80762 Test Plan: Reviewers: Subscribers: Bug #: 1647342 Differential Diff: PHID-DIFF-yyvo3sxpaodbm57o36o2
js/src/debugger/Debugger.cpp
js/src/debugger/Frame.cpp
js/src/debugger/Frame.h
--- a/js/src/debugger/Debugger.cpp
+++ b/js/src/debugger/Debugger.cpp
@@ -5989,17 +5989,17 @@ bool Debugger::CallData::adoptFrame() {
   RootedValue objVal(cx, ObjectValue(*obj));
   RootedDebuggerFrame frameObj(cx, DebuggerFrame::check(cx, objVal));
   if (!frameObj) {
     return false;
   }
 
   RootedDebuggerFrame adoptedFrame(cx);
   if (frameObj->isOnStack()) {
-    FrameIter iter(*frameObj->frameIterData());
+    FrameIter iter = frameObj->getFrameIter(cx);
     if (!dbg->observesFrame(iter)) {
       JS_ReportErrorASCII(cx, "Debugger.Frame's global is not a debuggee");
       return false;
     }
     if (!dbg->getFrame(cx, iter, &adoptedFrame)) {
       return false;
     }
   } else if (frameObj->isSuspended()) {
@@ -6276,30 +6276,19 @@ bool Debugger::replaceFrameGuts(JSContex
     return false;
   }
 
   for (size_t i = 0; i < frames.length(); i++) {
     HandleDebuggerFrame frameobj = frames[i];
     Debugger* dbg = Debugger::fromChildJSObject(frameobj);
 
     // Update frame object's ScriptFrameIter::data pointer.
-    frameobj->freeFrameIterData(cx->runtime()->defaultFreeOp());
-    ScriptFrameIter::Data* data = iter.copyData();
-    if (!data) {
-      // An OOM here means that some Debuggers' frame maps may still
-      // contain entries for 'from' and some Debuggers' frame maps may
-      // also contain entries for 'to'. Thus removeDebuggerFramesOnExit
-      // must run.
-      //
-      // The current frameobj in question is still in its Debugger's
-      // frame map keyed by 'from', so it will be covered by
-      // removeDebuggerFramesOnExit.
+    if (!frameobj->replaceFrameIterData(cx, iter)) {
       return false;
     }
-    frameobj->setFrameIterData(data);
 
     // Remove old frame.
     dbg->frames.remove(from);
 
     // Add the frame object with |to| as key.
     if (!dbg->frames.putNew(to, frameobj)) {
       // This OOM is subtle. At this point, removeDebuggerFramesOnExit
       // must run for the same reason given above.
--- a/js/src/debugger/Frame.cpp
+++ b/js/src/debugger/Frame.cpp
@@ -474,21 +474,17 @@ bool DebuggerFrame::getCallee(JSContext*
 
   return frame->owner()->wrapNullableDebuggeeObject(cx, callee, result);
 }
 
 /* static */
 bool DebuggerFrame::getIsConstructing(JSContext* cx, HandleDebuggerFrame frame,
                                       bool& result) {
   if (frame->isOnStack()) {
-    Maybe<FrameIter> maybeIter;
-    if (!DebuggerFrame::getFrameIter(cx, frame, maybeIter)) {
-      return false;
-    }
-    FrameIter& iter = *maybeIter;
+    FrameIter iter = frame->getFrameIter(cx);
 
     result = iter.isFunctionFrame() && iter.isConstructing();
   } else {
     MOZ_ASSERT(frame->isSuspended());
 
     // Generators and async functions can't be constructed.
     result = false;
   }
@@ -544,21 +540,17 @@ static void UpdateFrameIterPc(FrameIter&
 
 /* static */
 bool DebuggerFrame::getEnvironment(JSContext* cx, HandleDebuggerFrame frame,
                                    MutableHandleDebuggerEnvironment result) {
   Debugger* dbg = frame->owner();
   Rooted<Env*> env(cx);
 
   if (frame->isOnStack()) {
-    Maybe<FrameIter> maybeIter;
-    if (!DebuggerFrame::getFrameIter(cx, frame, maybeIter)) {
-      return false;
-    }
-    FrameIter& iter = *maybeIter;
+    FrameIter iter = frame->getFrameIter(cx);
 
     {
       AutoRealm ar(cx, iter.abstractFramePtr().environmentChain());
       UpdateFrameIterPc(iter);
       env = GetDebugEnvironmentForFrame(cx, iter.abstractFramePtr(), iter.pc());
     }
   } else {
     MOZ_ASSERT(frame->isSuspended());
@@ -579,21 +571,17 @@ bool DebuggerFrame::getEnvironment(JSCon
 
   return dbg->wrapEnvironment(cx, env, result);
 }
 
 /* static */
 bool DebuggerFrame::getOffset(JSContext* cx, HandleDebuggerFrame frame,
                               size_t& result) {
   if (frame->isOnStack()) {
-    Maybe<FrameIter> maybeIter;
-    if (!DebuggerFrame::getFrameIter(cx, frame, maybeIter)) {
-      return false;
-    }
-    FrameIter& iter = *maybeIter;
+    FrameIter iter = frame->getFrameIter(cx);
 
     AbstractFramePtr referent = DebuggerFrame::getReferent(frame);
     if (referent.isWasmDebugFrame()) {
       iter.wasmUpdateBytecodeOffset();
       result = iter.wasmBytecodeOffset();
     } else {
       JSScript* script = iter.script();
       UpdateFrameIterPc(iter);
@@ -611,22 +599,17 @@ bool DebuggerFrame::getOffset(JSContext*
   return true;
 }
 
 /* static */
 bool DebuggerFrame::getOlder(JSContext* cx, HandleDebuggerFrame frame,
                              MutableHandleDebuggerFrame result) {
   if (frame->isOnStack()) {
     Debugger* dbg = frame->owner();
-
-    Maybe<FrameIter> maybeIter;
-    if (!DebuggerFrame::getFrameIter(cx, frame, maybeIter)) {
-      return false;
-    }
-    FrameIter& iter = *maybeIter;
+    FrameIter iter = frame->getFrameIter(cx);
 
     while (true) {
       Activation& activation = *iter.activation();
       ++iter;
 
       // If the parent frame crosses an explicit async stack boundary, we
       // treat that as an indication to stop traversing sync frames, so that
       // the on-stack Debugger.Frame instances align with what you would
@@ -692,22 +675,17 @@ bool DebuggerFrame::getAsyncPromise(JSCo
 bool DebuggerFrame::getThis(JSContext* cx, HandleDebuggerFrame frame,
                             MutableHandleValue result) {
   Debugger* dbg = frame->owner();
 
   if (frame->isOnStack()) {
     if (!requireScriptReferent(cx, frame)) {
       return false;
     }
-
-    Maybe<FrameIter> maybeIter;
-    if (!DebuggerFrame::getFrameIter(cx, frame, maybeIter)) {
-      return false;
-    }
-    FrameIter& iter = *maybeIter;
+    FrameIter iter = frame->getFrameIter(cx);
 
     {
       AbstractFramePtr frame = iter.abstractFramePtr();
       AutoRealm ar(cx, frame.environmentChain());
 
       UpdateFrameIterPc(iter);
 
       if (!GetThisValueForDebuggerFrameMaybeOptimizedOut(cx, frame, iter.pc(),
@@ -1149,22 +1127,17 @@ Result<Completion> js::DebuggerGenericEv
 /* static */
 Result<Completion> DebuggerFrame::eval(JSContext* cx, HandleDebuggerFrame frame,
                                        mozilla::Range<const char16_t> chars,
                                        HandleObject bindings,
                                        const EvalOptions& options) {
   MOZ_ASSERT(frame->isOnStack());
 
   Debugger* dbg = frame->owner();
-
-  Maybe<FrameIter> maybeIter;
-  if (!DebuggerFrame::getFrameIter(cx, frame, maybeIter)) {
-    return cx->alreadyReportedError();
-  }
-  FrameIter& iter = *maybeIter;
+  FrameIter iter = frame->getFrameIter(cx);
 
   UpdateFrameIterPc(iter);
 
   return DebuggerGenericEval(cx, chars, bindings, options, dbg, nullptr, &iter);
 }
 
 bool DebuggerFrame::isOnStack() const { return !!getPrivate(); }
 
@@ -1209,21 +1182,22 @@ FrameIter::Data* DebuggerFrame::frameIte
 }
 
 /* static */
 AbstractFramePtr DebuggerFrame::getReferent(HandleDebuggerFrame frame) {
   FrameIter iter(*frame->frameIterData());
   return iter.abstractFramePtr();
 }
 
-/* static */
-bool DebuggerFrame::getFrameIter(JSContext* cx, HandleDebuggerFrame frame,
-                                 Maybe<FrameIter>& result) {
-  result.emplace(*frame->frameIterData());
-  return true;
+FrameIter DebuggerFrame::getFrameIter(JSContext* cx) {
+  auto data = frameIterData();
+  MOZ_ASSERT(data);
+  MOZ_ASSERT(data->cx_ == cx);
+
+  return FrameIter(*data);
 }
 
 /* static */
 bool DebuggerFrame::requireScriptReferent(JSContext* cx,
                                           HandleDebuggerFrame frame) {
   AbstractFramePtr referent = DebuggerFrame::getReferent(frame);
   if (!referent.hasScript()) {
     RootedValue frameobj(cx, ObjectValue(*frame));
@@ -1242,16 +1216,26 @@ void DebuggerFrame::setFrameIterData(Fra
 
 void DebuggerFrame::freeFrameIterData(JSFreeOp* fop) {
   if (FrameIter::Data* data = frameIterData()) {
     fop->delete_(this, data, MemoryUse::DebuggerFrameIterData);
     setPrivate(nullptr);
   }
 }
 
+bool DebuggerFrame::replaceFrameIterData(JSContext* cx, const FrameIter& iter) {
+  FrameIter::Data* data = iter.copyData();
+  if (!data) {
+    return false;
+  }
+  freeFrameIterData(cx->runtime()->defaultFreeOp());
+  setFrameIterData(data);
+  return true;
+}
+
 /* static */
 void DebuggerFrame::finalize(JSFreeOp* fop, JSObject* obj) {
   MOZ_ASSERT(fop->onMainThread());
 
   DebuggerFrame& frameobj = obj->as<DebuggerFrame>();
 
   // Connections between dying Debugger.Frames and their
   // AbstractGeneratorObjects should have been broken in DebugAPI::sweepAll.
@@ -1516,17 +1500,17 @@ bool DebuggerFrame::CallData::constructi
 
 bool DebuggerFrame::CallData::asyncPromiseGetter() {
   if (!ensureOnStackOrSuspended()) {
     return false;
   }
 
   RootedScript script(cx);
   if (frame->isOnStack()) {
-    FrameIter iter(*frame->frameIterData());
+    FrameIter iter = frame->getFrameIter(cx);
     AbstractFramePtr framePtr = iter.abstractFramePtr();
 
     if (!framePtr.isWasmDebugFrame()) {
       script = framePtr.script();
     }
   } else {
     MOZ_ASSERT(frame->isSuspended());
     script = frame->generatorInfo()->generatorScript();
@@ -1561,22 +1545,17 @@ bool DebuggerFrame::CallData::olderSaved
   return true;
 }
 
 /* static */
 bool DebuggerFrame::getOlderSavedFrame(JSContext* cx, HandleDebuggerFrame frame,
                                        MutableHandleSavedFrame result) {
   if (frame->isOnStack()) {
     Debugger* dbg = frame->owner();
-
-    Maybe<FrameIter> maybeIter;
-    if (!DebuggerFrame::getFrameIter(cx, frame, maybeIter)) {
-      return false;
-    }
-    FrameIter& iter = *maybeIter;
+    FrameIter iter = frame->getFrameIter(cx);
 
     while (true) {
       Activation& activation = *iter.activation();
       ++iter;
 
       // If the parent frame crosses an explicit async stack boundary, or we
       // have hit the end of the synchronous frames, we want to switch over
       // to using SavedFrames.
@@ -1654,18 +1633,18 @@ static bool DebuggerArguments_getArg(JSC
 
   RootedValue framev(cx, argsobj->as<NativeObject>().getReservedSlot(
                              JSSLOT_DEBUGARGUMENTS_FRAME));
   RootedDebuggerFrame thisobj(cx, DebuggerFrame::check(cx, framev));
   if (!thisobj || !EnsureOnStack(cx, thisobj)) {
     return false;
   }
 
-  FrameIter frameIter(*thisobj->frameIterData());
-  AbstractFramePtr frame = frameIter.abstractFramePtr();
+  FrameIter iter = thisobj->getFrameIter(cx);
+  AbstractFramePtr frame = iter.abstractFramePtr();
 
   // TODO handle wasm frame arguments -- they are not yet reflectable.
   MOZ_ASSERT(!frame.isWasmDebugFrame(), "a wasm frame args");
 
   // Since getters can be extracted and applied to other objects,
   // there is no guarantee this object has an ith argument.
   MOZ_ASSERT(i >= 0);
   RootedValue arg(cx);
@@ -1765,17 +1744,17 @@ bool DebuggerFrame::CallData::getScript(
   if (!ensureOnStackOrSuspended()) {
     return false;
   }
 
   RootedDebuggerScript scriptObject(cx);
 
   Debugger* debug = Debugger::fromChildJSObject(frame);
   if (frame->isOnStack()) {
-    FrameIter iter(*frame->frameIterData());
+    FrameIter iter = frame->getFrameIter(cx);
     AbstractFramePtr framePtr = iter.abstractFramePtr();
 
     if (framePtr.isWasmDebugFrame()) {
       RootedWasmInstanceObject instance(cx, framePtr.wasmInstance()->object());
       scriptObject = debug->wrapWasmScript(cx, instance);
     } else {
       RootedScript script(cx, framePtr.script());
       scriptObject = debug->wrapScript(cx, script);
--- a/js/src/debugger/Frame.h
+++ b/js/src/debugger/Frame.h
@@ -263,19 +263,16 @@ class DebuggerFrame : public NativeObjec
   static const JSClassOps classOps_;
 
   static const JSPropertySpec properties_[];
   static const JSFunctionSpec methods_[];
 
   static void finalize(JSFreeOp* fop, JSObject* obj);
 
   static AbstractFramePtr getReferent(HandleDebuggerFrame frame);
-  static MOZ_MUST_USE bool getFrameIter(JSContext* cx,
-                                        HandleDebuggerFrame frame,
-                                        mozilla::Maybe<FrameIter>& result);
   static MOZ_MUST_USE bool requireScriptReferent(JSContext* cx,
                                                  HandleDebuggerFrame frame);
 
   static MOZ_MUST_USE bool construct(JSContext* cx, unsigned argc, Value* vp);
 
   struct CallData;
 
   Debugger* owner() const;
@@ -284,19 +281,24 @@ class DebuggerFrame : public NativeObjec
   void setHasIncrementedStepper(bool incremented);
 
   MOZ_MUST_USE bool maybeIncrementStepperCounter(JSContext* cx,
                                                  AbstractFramePtr referent);
   MOZ_MUST_USE bool maybeIncrementStepperCounter(JSContext* cx,
                                                  JSScript* script);
   void maybeDecrementStepperCounter(JSFreeOp* fop, JSScript* script);
 
- public:
   FrameIter::Data* frameIterData() const;
   void setFrameIterData(FrameIter::Data*);
+
+ public:
+  FrameIter getFrameIter(JSContext* cx);
+
+  MOZ_MUST_USE bool replaceFrameIterData(JSContext* cx, const FrameIter&);
+
   void freeFrameIterData(JSFreeOp* fop);
   void maybeDecrementStepperCounter(JSFreeOp* fop, AbstractFramePtr referent);
 
   class GeneratorInfo;
   inline GeneratorInfo* generatorInfo() const;
 };
 
 } /* namespace js */