Commit fd6ffb84 authored by Jon Coppeard's avatar Jon Coppeard Committed by Richard Pospesel
Browse files

Bug 1816158 - Part 1: Disallow GC while iterating global's debugger vector r=sfink 

GC can mutate this vector so don't allow that while we are iterating. I think
it would be safe to use index-based iteration but it's safer to just ban it
entirely.

This fixes the crash produced by the testcase.

Differential Revision: https://phabricator.services.mozilla.com/D169701
parent 56ac62b9

js/src/debugger/Debugger.cpp

+24 −13
Original line number Diff line number Diff line
@@ -1264,8 +1264,9 @@ bool DebugAPI::slowPathOnNewGenerator(JSContext* cx, AbstractFramePtr frame, 
      MakeScopeExit([&] { Debugger::terminateDebuggerFrames(cx, frame); });



  bool ok = true;

  gc::AutoSuppressGC nogc(cx);

  Debugger::forEachOnStackDebuggerFrame(

      frame, [&](Debugger* dbg, DebuggerFrame* frameObjPtr) {

      frame, nogc, [&](Debugger* dbg, DebuggerFrame* frameObjPtr) {

        if (!ok) {

          return;

        }
@@ -3290,7 +3291,10 @@ bool Debugger::updateExecutionObservabilityOfScripts( 


template <typename FrameFn>

/* static */

void Debugger::forEachOnStackDebuggerFrame(AbstractFramePtr frame, FrameFn fn) {

void Debugger::forEachOnStackDebuggerFrame(AbstractFramePtr frame,

                                           const JS::AutoRequireNoGC& nogc,

                                           FrameFn fn) {

  // GC is disallowed because it may mutate the vector we are iterating.

  for (Realm::DebuggerVectorEntry& entry : frame.global()->getDebuggers()) {

    Debugger* dbg = entry.dbg;

    if (FrameMap::Ptr frameEntry = dbg->frames.lookup(frame)) {
@@ -3301,13 +3305,14 @@ void Debugger::forEachOnStackDebuggerFrame(AbstractFramePtr frame, FrameFn fn) { 


template <typename FrameFn>

/* static */

void Debugger::forEachOnStackOrSuspendedDebuggerFrame(JSContext* cx,

                                                      AbstractFramePtr frame,

void Debugger::forEachOnStackOrSuspendedDebuggerFrame(

    JSContext* cx, AbstractFramePtr frame, const JS::AutoRequireNoGC& nogc,

    FrameFn fn) {

  Rooted<AbstractGeneratorObject*> genObj(

      cx, frame.isGeneratorFrame() ? GetGeneratorObjectForFrame(cx, frame)

                                   : nullptr);



  // GC is disallowed because it may mutate the vector we are iterating.

  for (Realm::DebuggerVectorEntry& entry : frame.global()->getDebuggers()) {

    Debugger* dbg = entry.dbg;
@@ -3329,7 +3334,9 @@ void Debugger::forEachOnStackOrSuspendedDebuggerFrame(JSContext* cx, 
bool Debugger::getDebuggerFrames(AbstractFramePtr frame,

                                 MutableHandle<DebuggerFrameVector> frames) {

  bool hadOOM = false;

  forEachOnStackDebuggerFrame(frame, [&](Debugger*, DebuggerFrame* frameobj) {

  JS::AutoAssertNoGC nogc;

  forEachOnStackDebuggerFrame(frame, nogc,

                              [&](Debugger*, DebuggerFrame* frameobj) {

                                if (!hadOOM && !frames.append(frameobj)) {

                                  hadOOM = true;

                                }
@@ -6535,8 +6542,10 @@ bool Debugger::replaceFrameGuts(JSContext* cx, AbstractFramePtr from, 
/* static */

bool DebugAPI::inFrameMaps(AbstractFramePtr frame) {

  bool foundAny = false;

  JS::AutoAssertNoGC nogc;

  Debugger::forEachOnStackDebuggerFrame(

      frame, [&](Debugger*, DebuggerFrame* frameobj) { foundAny = true; });

      frame, nogc,

      [&](Debugger*, DebuggerFrame* frameobj) { foundAny = true; });

  return foundAny;

}
@@ -6544,8 +6553,9 @@ bool DebugAPI::inFrameMaps(AbstractFramePtr frame) { 
void Debugger::suspendGeneratorDebuggerFrames(JSContext* cx,

                                              AbstractFramePtr frame) {

  JS::GCContext* gcx = cx->gcContext();

  JS::AutoAssertNoGC nogc;

  forEachOnStackDebuggerFrame(

      frame, [&](Debugger* dbg, DebuggerFrame* dbgFrame) {

      frame, nogc, [&](Debugger* dbg, DebuggerFrame* dbgFrame) {

        dbg->frames.remove(frame);



#if DEBUG
@@ -6564,8 +6574,9 @@ void Debugger::suspendGeneratorDebuggerFrames(JSContext* cx, 
void Debugger::terminateDebuggerFrames(JSContext* cx, AbstractFramePtr frame) {

  JS::GCContext* gcx = cx->gcContext();



  JS::AutoAssertNoGC nogc;

  forEachOnStackOrSuspendedDebuggerFrame(

      cx, frame, [&](Debugger* dbg, DebuggerFrame* dbgFrame) {

      cx, frame, nogc, [&](Debugger* dbg, DebuggerFrame* dbgFrame) {

        Debugger::terminateDebuggerFrame(gcx, dbg, dbgFrame, frame);

      });

js/src/debugger/Debugger.h

+6 −4
Original line number Diff line number Diff line
@@ -937,10 +937,12 @@ class Debugger : private mozilla::LinkedListElement<Debugger> { 
      IsObserving observing);



  template <typename FrameFn /* void (Debugger*, DebuggerFrame*) */>

  static void forEachOnStackDebuggerFrame(AbstractFramePtr frame, FrameFn fn);

  static void forEachOnStackDebuggerFrame(AbstractFramePtr frame,

                                          const JS::AutoRequireNoGC& nogc,

                                          FrameFn fn);

  template <typename FrameFn /* void (Debugger*, DebuggerFrame*) */>

  static void forEachOnStackOrSuspendedDebuggerFrame(JSContext* cx,

                                                     AbstractFramePtr frame,

  static void forEachOnStackOrSuspendedDebuggerFrame(

      JSContext* cx, AbstractFramePtr frame, const JS::AutoRequireNoGC& nogc,

      FrameFn fn);



  /*

js/src/gc/GC.h

+1 −1
Original line number Diff line number Diff line
@@ -204,7 +204,7 @@ static inline void MaybeVerifyBarriers(JSContext* cx, bool always = false) {} 
 * This works by updating the |JSContext::suppressGC| counter which is checked

 * at the start of GC.

 */

class MOZ_RAII JS_HAZ_GC_SUPPRESSED AutoSuppressGC {

class MOZ_RAII JS_HAZ_GC_SUPPRESSED AutoSuppressGC : public JS::AutoRequireNoGC {

  int32_t& suppressGC_;



 public: