Refactor ConsoleAPIStorage event handling and add tests for ring buffer behavior

dom/console/ConsoleAPIStorage.js

@@ -94,13 +94,24 @@ ConsoleAPIStorageService.prototype = {
   getEvents: function CS_getEvents(aId)
   {
     if (aId != null) {
-      return (_consoleStorage.get(aId) || []).slice(0);
+      let storage = _consoleStorage.get(aId);
+      if (!storage) {
+        return [];
+      }
+      let { events, next } = storage;
+      return next === 0 ? events.slice() :
+             events.slice(next).concat(events.slice(0, next));
     }
 
     let result = [];
 
-    for (let [id, events] of _consoleStorage) {
-      result.push.apply(result, events);
+    for (let { events, next } of _consoleStorage.values()) {
+      for (let i = next; i < events.length; i++) {
+        result.push(events[i]);
+      }
+      for (let i = 0; i < next; i++) {
+        result.push(events[i]);
+      }
     }
 
     return result.sort(function(a, b) {
@@ -122,16 +133,21 @@ ConsoleAPIStorageService.prototype = {
    */
   recordEvent: function CS_recordEvent(aId, aOuterId, aEvent)
   {
-    if (!_consoleStorage.has(aId)) {
-      _consoleStorage.set(aId, []);
+    let storage = _consoleStorage.get(aId);
+    if (!storage) {
+      storage = { events: [], next: 0 };
+      _consoleStorage.set(aId, storage);
     }
 
-    let storage = _consoleStorage.get(aId);
-    storage.push(aEvent);
-
-    // truncate
-    if (storage.length > STORAGE_MAX_EVENTS) {
-      storage.shift();
+    // Overwrite the oldest event once full, without moving the other entries.
+    // Advance before notifying observers, which may read or reenter storage.
+    if (storage.events.length < STORAGE_MAX_EVENTS) {
+      storage.events.push(aEvent);
+    } else {
+      storage.events[storage.next] = aEvent;
+      if (++storage.next === STORAGE_MAX_EVENTS) {
+        storage.next = 0;
+      }
     }
 
     Services.obs.notifyObservers(aEvent, "console-api-log-event", aOuterId);

dom/console/moz.build

@@ -40,5 +40,6 @@ LOCAL_INCLUDES += [
 
 MOCHITEST_MANIFESTS += [ 'tests/mochitest.ini' ]
 MOCHITEST_CHROME_MANIFESTS += [ 'tests/chrome.ini' ]
+XPCSHELL_TESTS_MANIFESTS += [ 'tests/xpcshell.ini' ]
 
 FINAL_LIBRARY = 'xul'

dom/console/tests/test_console_storage_ring.js

@@ -0,0 +1,53 @@
+function run_test() {
+  const storage = Components.classes["@mozilla.org/consoleAPI-storage;1"]
+                            .getService(Components.interfaces.nsIConsoleAPIStorage);
+  Components.utils.import("resource://gre/modules/Services.jsm");
+  storage.clearEvents();
+  try {
+    for (let i = 0; i < 3501; i++) {
+      storage.recordEvent("ring-test", "outer", { timeStamp: i });
+      if (i === 998 || i === 999 || i === 1000 || i === 1999 || i === 3500) {
+        let events = storage.getEvents("ring-test");
+        equal(events.length, Math.min(i + 1, 1000));
+        for (let j = 0; j < events.length; j++) {
+          equal(events[j].timeStamp, i + 1 - events.length + j);
+        }
+        events.length = 0;
+        equal(storage.getEvents("ring-test").length, Math.min(i + 1, 1000));
+      }
+    }
+    storage.recordEvent("other", "outer", { timeStamp: 2500.5 });
+    let all = storage.getEvents();
+    equal(all.length, 1001);
+    equal(all[0].timeStamp, 2500.5);
+    equal(all[1].timeStamp, 2501);
+    equal(all[1000].timeStamp, 3500);
+
+    let reentered = false;
+    let observer = {
+      observe(subject, topic, data) {
+        if (data === "ring-test" && !reentered) {
+          equal(storage.getEvents(data)[999].timeStamp, 3501);
+          reentered = true;
+          storage.recordEvent("ring-test", "outer", { timeStamp: 3502 });
+        }
+      }
+    };
+    Services.obs.addObserver(observer, "console-storage-cache-event", false);
+    try {
+      storage.recordEvent("ring-test", "outer", { timeStamp: 3501 });
+    } finally {
+      Services.obs.removeObserver(observer, "console-storage-cache-event");
+    }
+    equal(reentered, true);
+    equal(storage.getEvents("ring-test")[999].timeStamp, 3502);
+    storage.clearEvents("ring-test");
+    equal(storage.getEvents("ring-test").length, 0);
+    equal(storage.getEvents("other").length, 1);
+    storage.recordEvent("ring-test", "outer", { timeStamp: 4000 });
+    equal(storage.getEvents("ring-test")[0].timeStamp, 4000);
+  } finally {
+    storage.clearEvents();
+  }
+  equal(storage.getEvents().length, 0);
+}

dom/console/tests/xpcshell.ini

@@ -0,0 +1,5 @@
+[DEFAULT]
+head =
+tail =
+
+[test_console_storage_ring.js]

js/src/builtin/Map.js

@@ -81,9 +81,10 @@ function MapGroupBy(items, callbackfn) {
         var key = callContentFunction(callbackfn, undefined, value, k);
 
         // Steps 6.c-d.
-        var elements;
-        if (callFunction(std_Map_has, groups, key)) {
-            elements = callFunction(std_Map_get, groups, key);
+        // Group values are always arrays, so undefined also tells us whether
+        // this is a new key without a second hash-table lookup.
+        var elements = callFunction(std_Map_get, groups, key);
+        if (elements !== undefined) {
             callFunction(std_Array_push, elements, value);
         } else {
             elements = [value];

js/src/jit-test/tests/collections/Map-groupBy-lookup.js

@@ -0,0 +1,47 @@
+// Repeated keys must reuse their group, including SameValueZero keys.
+var objectKey = {};
+var symbolKey = Symbol();
+var keys = [undefined, null, false, 0, -0, NaN, "key", objectKey, symbolKey];
+for (var iteration = 0; iteration < 100; iteration++) {
+    var input = [];
+    for (var repeat = 0; repeat < 4; repeat++) {
+        for (var key of keys)
+            input.push(key);
+    }
+    var calls = 0;
+    var groups = Map.groupBy(input, function(value, index) {
+        assertEq(index, calls++);
+        return value;
+    });
+    assertEq(calls, input.length);
+    assertEq(groups.size, 8);
+    for (var key of keys)
+        assertEq(groups.get(key).length, key === 0 ? 8 : 4);
+    assertEq(groups.get(objectKey)[0], objectKey);
+    assertEq(groups.get(symbolKey)[0], symbolKey);
+    assertEq(groups.get(undefined)[0], undefined);
+    var order = Array.from(groups.keys());
+    assertEq(order[0], undefined);
+    assertEq(order[3], 0);
+    assertEq(order[4], NaN);
+    assertEq(order[7], symbolKey);
+}
+
+// A throwing callback must still close the input iterator.
+var closed = false;
+function* values() {
+    try {
+        yield 1;
+        yield 2;
+    } finally {
+        closed = true;
+    }
+}
+var sentinel = {};
+try {
+    Map.groupBy(values(), function() { throw sentinel; });
+    throw new Error("callback did not throw");
+} catch (error) {
+    assertEq(error, sentinel);
+}
+assertEq(closed, true);