Bug 1927464 part 9 - Make remove and clear operations infallible. r=jonco

Shrinking the table is an optimization and JS code can already observe this state
if it catches the OOM exception, so it's simpler to ignore it.

The callers are still fallible due to `CallObjFunc`, but as a follow-up we can probably
change how that works and make `JS::SetClear` etc infallible too.

Differential Revision: https://phabricator.services.mozilla.com/D227155

js/src/builtin/MapObject.cpp

@@ -972,18 +972,11 @@ bool MapObject::delete_(JSContext* cx, HandleObject obj, HandleValue key,
     return false;
   }
 
-  bool ok;
   if (mapObject->isTenured()) {
-    ok = Table(mapObject).remove(k, rval);
+    *rval = Table(mapObject).remove(k);
   } else {
-    ok = PreBarrieredTable(mapObject).remove(k, rval);
+    *rval = PreBarrieredTable(mapObject).remove(k);
   }
-
-  if (!ok) {
-    ReportOutOfMemory(cx);
-    return false;
-  }
-
   return true;
 }
 
@@ -1075,19 +1068,11 @@ bool MapObject::clear(JSContext* cx, unsigned argc, Value* vp) {
 
 bool MapObject::clear(JSContext* cx, HandleObject obj) {
   MapObject* mapObject = &obj->as<MapObject>();
-
-  bool ok;
   if (mapObject->isTenured()) {
-    ok = Table(mapObject).clear();
+    Table(mapObject).clear();
   } else {
-    ok = PreBarrieredTable(mapObject).clear();
+    PreBarrieredTable(mapObject).clear();
   }
-
-  if (!ok) {
-    ReportOutOfMemory(cx);
-    return false;
-  }
-
   return true;
 }
 
@@ -1717,11 +1702,7 @@ bool SetObject::delete_(JSContext* cx, HandleObject obj, HandleValue key,
   }
 
   SetObject* setObj = &obj->as<SetObject>();
-
+  *rval = Table(setObj).remove(k);
-  if (!Table(setObj).remove(k, rval)) {
-    ReportOutOfMemory(cx);
-    return false;
-  }
   return true;
 }
 
@@ -1732,11 +1713,7 @@ bool SetObject::delete_impl(JSContext* cx, const CallArgs& args) {
 
   SetObject* setObj = &args.thisv().toObject().as<SetObject>();
 
-  bool found;
+  bool found = Table(setObj).remove(key);
-  if (!Table(setObj).remove(key, &found)) {
-    ReportOutOfMemory(cx);
-    return false;
-  }
   args.rval().setBoolean(found);
   return true;
 }
@@ -1792,19 +1769,13 @@ bool SetObject::entries(JSContext* cx, unsigned argc, Value* vp) {
 bool SetObject::clear(JSContext* cx, HandleObject obj) {
   MOZ_ASSERT(SetObject::is(obj));
   SetObject* setObj = &obj->as<SetObject>();
-  if (!Table(setObj).clear()) {
+  Table(setObj).clear();
-    ReportOutOfMemory(cx);
-    return false;
-  }
   return true;
 }
 
 bool SetObject::clear_impl(JSContext* cx, const CallArgs& args) {
   SetObject* setObj = &args.thisv().toObject().as<SetObject>();
-  if (!Table(setObj).clear()) {
+  Table(setObj).clear();
-    ReportOutOfMemory(cx);
-    return false;
-  }
   args.rval().setUndefined();
   return true;
 }

js/src/builtin/OrderedHashTableObject.h

@@ -454,15 +454,10 @@ class MOZ_STACK_CLASS OrderedHashTableImpl {
   }
 
   /*
-   * If the table contains an element matching l, remove it and set *foundp
+   * If the table contains an element matching l, remove it and return true.
-   * to true. Otherwise set *foundp to false.
+   * Otherwise return false.
-   *
-   * Return true on success, false if we tried to shrink the table and hit an
-   * allocation failure. Even if this returns false, *foundp is set correctly
-   * and the matching element was removed. Shrinking is an optimization and
-   * it's OK for it to fail.
    */
-  bool remove(const Lookup& l, bool* foundp) {
+  bool remove(const Lookup& l) {
     // Note: This could be optimized so that removing the last entry,
     // data[dataLength - 1], decrements dataLength. LIFO use cases would
     // benefit.
@@ -470,15 +465,9 @@ class MOZ_STACK_CLASS OrderedHashTableImpl {
     // If a matching entry exists, empty it.
     Data* e = lookup(l, prepareHash(l));
     if (e == nullptr) {
-      *foundp = false;
+      return false;
-      return true;
     }
 
-    *foundp = true;
-    return remove(e);
-  }
-
-  bool remove(Data* e) {
     MOZ_ASSERT(uint32_t(e - getData()) < getDataCapacity());
 
     uint32_t liveCount = getLiveCount();
@@ -490,12 +479,12 @@ class MOZ_STACK_CLASS OrderedHashTableImpl {
     uint32_t pos = e - getData();
     forEachRange([this, pos](Range* range) { range->onRemove(obj, pos); });
 
-    // If many entries have been removed, try to shrink the table.
+    // If many entries have been removed, try to shrink the table. Ignore OOM
+    // because shrinking the table is an optimization and it's okay for it to
+    // fail.
     if (hashBuckets() > InitialBuckets &&
         liveCount < getDataLength() * MinDataFill) {
-      if (!rehash(getHashShift() + 1)) {
+      (void)rehash(getHashShift() + 1);
-        return false;
-      }
     }
 
     return true;
@@ -504,15 +493,11 @@ class MOZ_STACK_CLASS OrderedHashTableImpl {
   /*
    * Remove all entries.
    *
-   * Return true on success, false if we tried to shrink the table and hit an
-   * allocation failure. Even if this returns false, the table was cleared.
-   * Shrinking is an optimization and it's OK for it to fail.
-   *
    * The effect on live Ranges is the same as removing all entries; in
    * particular, those Ranges are still live and will see any entries added
-   * after a successful clear().
+   * after a clear().
    */
-  [[nodiscard]] bool clear() {
+  void clear() {
     if (getDataLength() != 0) {
       destroyData(getData(), getDataLength());
       setDataLength(0);
@@ -523,11 +508,10 @@ class MOZ_STACK_CLASS OrderedHashTableImpl {
 
       forEachRange([](Range* range) { range->onClear(); });
 
-      // Try to shrink the table.
+      // Try to shrink the table. Ignore OOM because shrinking the table is an
+      // optimization and it's okay for it to fail.
       if (buckets > InitialBuckets) {
-        if (!rehash(InitialHashShift)) {
+        (void)rehash(InitialHashShift);
-          return false;
-        }
       }
     }
 
@@ -535,7 +519,6 @@ class MOZ_STACK_CLASS OrderedHashTableImpl {
     MOZ_ASSERT(getData());
     MOZ_ASSERT(getDataLength() == 0);
     MOZ_ASSERT(getLiveCount() == 0);
-    return true;
   }
 
   /*
@@ -1103,10 +1086,8 @@ class MOZ_STACK_CLASS OrderedHashMapImpl {
   bool has(const Lookup& key) const { return impl.has(key); }
   Range all() const { return impl.all(); }
   Entry* get(const Lookup& key) { return impl.get(key); }
-  bool remove(const Lookup& key, bool* foundp) {
+  bool remove(const Lookup& key) { return impl.remove(key); }
-    return impl.remove(key, foundp);
+  void clear() { impl.clear(); }
-  }
-  [[nodiscard]] bool clear() { return impl.clear(); }
 
   void destroy(JS::GCContext* gcx) { impl.destroy(gcx); }
 
@@ -1199,10 +1180,8 @@ class MOZ_STACK_CLASS OrderedHashSetImpl {
   [[nodiscard]] bool put(Input&& value) {
     return impl.put(std::forward<Input>(value));
   }
-  bool remove(const Lookup& value, bool* foundp) {
+  bool remove(const Lookup& value) { return impl.remove(value); }
-    return impl.remove(value, foundp);
+  void clear() { impl.clear(); }
-  }
-  [[nodiscard]] bool clear() { return impl.clear(); }
 
   void destroy(JS::GCContext* gcx) { impl.destroy(gcx); }