Bug 1539262 - Support multiple origins in the same metric in Origin Telemetry r=janerik
authorChris H-C <chutten@mozilla.com>
Tue, 02 Apr 2019 16:58:50 +0000
changeset 467641 d13bf5b0872ca507b911ce85dfe6aac071bb4cde
parent 467640 80e1339ed57ff9441123533bc448311e29cf6cf7
child 467642 0f5833c5f5f7dd895a8b57d0be645825fa75aba1
push id35806
push userrgurzau@mozilla.com
push dateWed, 03 Apr 2019 04:07:39 +0000
treeherdermozilla-central@45808ab18609 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjanerik
bugs1539262
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1539262 - Support multiple origins in the same metric in Origin Telemetry r=janerik Since reporting intervals are ~1 day/1 session, the Origin Telemetry prototype must support the possibility that multiple origins will be recorded for the same metric. For example, if the user is sampled to record two pageloads where the same ultra-common tracker is present and blocked we must record that tracker as having been blocked twice. This requires a bit of a shift in storage and plaintext snapshot. Instead of being an array of origins with duplicates, now we're storing origins as a bag (aka multiset, aka hashtable of origin->count). Differential Revision: https://phabricator.services.mozilla.com/D25283
toolkit/components/telemetry/core/TelemetryOrigin.cpp
toolkit/components/telemetry/core/nsITelemetry.idl
toolkit/components/telemetry/docs/collection/origin.rst
toolkit/components/telemetry/tests/gtest/TestOrigins.cpp
--- a/toolkit/components/telemetry/core/TelemetryOrigin.cpp
+++ b/toolkit/components/telemetry/core/TelemetryOrigin.cpp
@@ -104,31 +104,27 @@ namespace {
 // cannot make sure that no other function is called before this point.
 static StaticMutex gTelemetryOriginMutex;
 
 nsTArray<const char*>* gOriginsList = nullptr;
 
 typedef nsDataHashtable<nsCStringHashKey, size_t> OriginToIndexMap;
 OriginToIndexMap* gOriginToIndexMap;
 
-typedef nsDataHashtable<OriginMetricIDHashKey, nsTArray<nsCString>>
-    IdToOriginsMap;
+typedef nsDataHashtable<nsCStringHashKey, uint32_t> OriginBag;
+typedef nsDataHashtable<OriginMetricIDHashKey, OriginBag> IdToOriginBag;
 
-IdToOriginsMap* gMetricToOriginsMap;
+IdToOriginBag* gMetricToOriginBag;
 
 mozilla::Atomic<bool, mozilla::Relaxed> gInitDone(false);
 
 // Useful for app-encoded data
 typedef nsTArray<Pair<OriginMetricID, nsTArray<nsTArray<bool>>>>
     IdBoolsPairArray;
 
-// The number of prioData elements needed to encode the contents of storage.
-// Will be some whole multiple of gPrioDatasPerMetric.
-static uint32_t gPrioDataCount = 0;
-
 // Prio has a maximum supported number of bools it can encode at a time.
 // This means a single metric may result in several encoded payloads if the
 // number of origins exceeds the number of bools.
 // Each encoded payload corresponds to an element in the `prioData` array in the
 // "prio" ping.
 // This number is the number of encoded payloads needed per metric, and is
 // equivalent to "how many bitvectors do we need to encode this whole list of
 // origins?"
@@ -149,51 +145,89 @@ NS_NAMED_LITERAL_CSTRING(kUnknownOrigin,
 
 namespace {
 
 const char* GetNameForMetricID(OriginMetricID aId) {
   MOZ_ASSERT(aId < OriginMetricID::Count);
   return mozilla::Telemetry::MetricIDToString[static_cast<uint32_t>(aId)];
 }
 
+// Calculates the number of `prioData` elements we'd need if we were asked for
+// an encoded snapshot right now.
+uint32_t PrioDataCount(const StaticMutexAutoLock& lock) {
+  uint32_t count = 0;
+  auto iter = gMetricToOriginBag->ConstIter();
+  for (; !iter.Done(); iter.Next()) {
+    auto originIt = iter.Data().ConstIter();
+    uint32_t maxOriginCount = 0;
+    for (; !originIt.Done(); originIt.Next()) {
+      maxOriginCount = std::max(maxOriginCount, originIt.Data());
+    }
+    count += gPrioDatasPerMetric * maxOriginCount;
+  }
+  return count;
+}
+
+// Takes the storage and turns it into bool arrays for Prio to encode, turning
+// { metric1: [origin1, origin2, ...], ...}
+// into
+// [(metric1, [[shard1], [shard2], ...]), ...]
+// Note: if an origin is present multiple times for a given metric, we must
+// generate multiple (id, boolvectors) pairs so that they are all reported.
+// Meaning
+// { metric1: [origin1, origin2, origin2] }
+// must turn into (with a pretend gNumBooleans of 1)
+// [(metric1, [[1], [1]]), (metric1, [[0], [1]])]
 nsresult AppEncodeTo(const StaticMutexAutoLock& lock,
                      IdBoolsPairArray& aResult) {
-  auto iter = gMetricToOriginsMap->ConstIter();
+  auto iter = gMetricToOriginBag->ConstIter();
   for (; !iter.Done(); iter.Next()) {
     OriginMetricID id = iter.Key();
+    const OriginBag& bag = iter.Data();
 
-    // Fill in the result bool vectors with `false`s.
-    nsTArray<nsTArray<bool>> metricData(gPrioDatasPerMetric);
-    metricData.SetLength(gPrioDatasPerMetric);
-    for (size_t i = 0; i < metricData.Length() - 1; ++i) {
-      metricData[i].SetLength(PrioEncoder::gNumBooleans);
-      for (auto& metricDatum : metricData[i]) {
+    uint32_t generation = 1;
+    uint32_t maxGeneration = 1;
+    do {
+      // Fill in the result bool vectors with `false`s.
+      nsTArray<nsTArray<bool>> metricData(gPrioDatasPerMetric);
+      metricData.SetLength(gPrioDatasPerMetric);
+      for (size_t i = 0; i < metricData.Length() - 1; ++i) {
+        metricData[i].SetLength(PrioEncoder::gNumBooleans);
+        for (auto& metricDatum : metricData[i]) {
+          metricDatum = false;
+        }
+      }
+      auto& lastArray = metricData[metricData.Length() - 1];
+      lastArray.SetLength(gOriginsList->Length() % PrioEncoder::gNumBooleans);
+      for (auto& metricDatum : lastArray) {
         metricDatum = false;
       }
-    }
-    auto& lastArray = metricData[metricData.Length() - 1];
-    lastArray.SetLength(gOriginsList->Length() % PrioEncoder::gNumBooleans);
-    for (auto& metricDatum : lastArray) {
-      metricDatum = false;
-    }
+
+      auto originIt = bag.ConstIter();
+      for (; !originIt.Done(); originIt.Next()) {
+        uint32_t originCount = originIt.Data();
+        if (originCount >= generation) {
+          maxGeneration = std::max(maxGeneration, originCount);
 
-    for (const auto& origin : iter.Data()) {
-      size_t index;
-      if (!gOriginToIndexMap->Get(origin, &index)) {
-        return NS_ERROR_FAILURE;
+          const nsACString& origin = originIt.Key();
+          size_t index;
+          if (!gOriginToIndexMap->Get(origin, &index)) {
+            return NS_ERROR_FAILURE;
+          }
+          MOZ_ASSERT(index < gOriginsList->Length());
+          size_t shardIndex =
+              ceil(static_cast<double>(index) / PrioEncoder::gNumBooleans);
+          MOZ_ASSERT(shardIndex < metricData.Length());
+          MOZ_ASSERT(index % PrioEncoder::gNumBooleans <
+                     metricData[shardIndex].Length());
+          metricData[shardIndex][index % PrioEncoder::gNumBooleans] = true;
+        }
       }
-      MOZ_ASSERT(index < gOriginsList->Length());
-      size_t shardIndex =
-          ceil(static_cast<double>(index) / PrioEncoder::gNumBooleans);
-      MOZ_ASSERT(shardIndex < metricData.Length());
-      MOZ_ASSERT(index % PrioEncoder::gNumBooleans <
-                 metricData[shardIndex].Length());
-      metricData[shardIndex][index % PrioEncoder::gNumBooleans] = true;
-    }
-    aResult.AppendElement(MakePair(id, metricData));
+      aResult.AppendElement(MakePair(id, metricData));
+    } while (generation < maxGeneration);
   }
   return NS_OK;
 }
 
 }  // anonymous namespace
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
@@ -227,17 +261,17 @@ void TelemetryOrigin::InitializeGlobalSt
     MOZ_ASSERT(!kUnknownOrigin.Equals((*gOriginsList)[i]),
                "Unknown origin literal is reserved in Origin Telemetry");
     gOriginToIndexMap->Put(nsDependentCString((*gOriginsList)[i]), i);
   }
 
   // Add the meta-origin for tracking recordings to untracked origins.
   gOriginToIndexMap->Put(kUnknownOrigin, gOriginsList->Length());
 
-  gMetricToOriginsMap = new IdToOriginsMap();
+  gMetricToOriginBag = new IdToOriginBag();
 
   // This map shouldn't change at runtime, so make debug builds complain
   // if it tries.
 #ifdef DEBUG
   gOriginToIndexMap->MarkImmutable();
 #endif  // DEBUG
 
   gInitDone = true;
@@ -255,69 +289,63 @@ void TelemetryOrigin::DeInitializeGlobal
     return;
   }
 
   delete gOriginsList;
 
   delete gOriginToIndexMap;
   gOriginToIndexMap = nullptr;
 
-  delete gMetricToOriginsMap;
-  gMetricToOriginsMap = nullptr;
+  delete gMetricToOriginBag;
+  gMetricToOriginBag = nullptr;
 
   gInitDone = false;
 }
 
 nsresult TelemetryOrigin::RecordOrigin(OriginMetricID aId,
                                        const nsACString& aOrigin) {
   if (!XRE_IsParentProcess()) {
     return NS_ERROR_FAILURE;
   }
 
-  StaticMutexAutoLock locker(gTelemetryOriginMutex);
+  uint32_t prioDataCount;
+  {
+    StaticMutexAutoLock locker(gTelemetryOriginMutex);
 
-  // Common Telemetry error-handling practices for recording functions:
-  // only illegal calls return errors whereas merely incorrect ones are mutely
-  // ignored.
-  if (!gInitDone) {
-    return NS_OK;
-  }
-
-  nsCString origin(aOrigin);
-  if (!gOriginToIndexMap->Contains(aOrigin)) {
-    // Only record one unknown origin per metric per snapshot.
-    // (otherwise we may get swamped and blow our data budget.)
-    if (gMetricToOriginsMap->Contains(aId) &&
-        gMetricToOriginsMap->GetOrInsert(aId).Contains(kUnknownOrigin)) {
+    // Common Telemetry error-handling practices for recording functions:
+    // only illegal calls return errors whereas merely incorrect ones are mutely
+    // ignored.
+    if (!gInitDone) {
       return NS_OK;
     }
-    origin = kUnknownOrigin;
-  }
-
-  if (!gMetricToOriginsMap->Contains(aId)) {
-    // If we haven't recorded anything for this metric yet, we're adding some
-    // prioDatas.
-    gPrioDataCount += gPrioDatasPerMetric;
-  }
 
-  auto& originArray = gMetricToOriginsMap->GetOrInsert(aId);
+    nsCString origin(aOrigin);
+    if (!gOriginToIndexMap->Contains(aOrigin)) {
+      // Only record one unknown origin per metric per snapshot.
+      // (otherwise we may get swamped and blow our data budget.)
+      if (gMetricToOriginBag->Contains(aId) &&
+          gMetricToOriginBag->GetOrInsert(aId).Contains(kUnknownOrigin)) {
+        return NS_OK;
+      }
+      origin = kUnknownOrigin;
+    }
 
-  if (originArray.Contains(origin)) {
-    // If we've already recorded this metric for this origin, then we're going
-    // to need more prioDatas to encode that it happened again.
-    gPrioDataCount += gPrioDatasPerMetric;
+    auto& originBag = gMetricToOriginBag->GetOrInsert(aId);
+    originBag.GetOrInsert(origin)++;
+
+    prioDataCount = PrioDataCount(locker);
   }
 
-  originArray.AppendElement(origin);
-
   static uint32_t sPrioPingLimit =
       mozilla::Preferences::GetUint("toolkit.telemetry.prioping.dataLimit", 10);
-  if (gPrioDataCount >= sPrioPingLimit) {
+  if (prioDataCount >= sPrioPingLimit) {
     nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
     if (os) {
+      // Ensure we don't notify while holding the lock in case of synchronous
+      // dispatch. May deadlock ourselves if we then trigger a snapshot.
       os->NotifyObservers(nullptr, "origin-telemetry-storage-limit-reached",
                           nullptr);
     }
   }
 
   return NS_OK;
 }
 
@@ -327,61 +355,62 @@ nsresult TelemetryOrigin::GetOriginSnaps
     return NS_ERROR_FAILURE;
   }
 
   if (!gInitDone) {
     return NS_OK;
   }
 
   // Step 1: Grab the lock, copy into stack-local storage, optionally clear.
-  IdToOriginsMap copy;
+  IdToOriginBag copy;
   {
     StaticMutexAutoLock locker(gTelemetryOriginMutex);
 
     if (aClear) {
       // I'd really prefer to clear after we're sure the snapshot didn't go
       // awry, but we can't hold a lock preventing recording while using JS
       // APIs. And replaying any interleaving recording sounds like too much
       // squeeze for not enough juice.
 
-      gMetricToOriginsMap->SwapElements(copy);
-      gPrioDataCount = 0;
+      gMetricToOriginBag->SwapElements(copy);
     } else {
-      auto iter = gMetricToOriginsMap->ConstIter();
+      auto iter = gMetricToOriginBag->ConstIter();
       for (; !iter.Done(); iter.Next()) {
-        copy.Put(iter.Key(), iter.Data());
+        OriginBag& bag = copy.GetOrInsert(iter.Key());
+        auto originIt = iter.Data().ConstIter();
+        for (; !originIt.Done(); originIt.Next()) {
+          bag.Put(originIt.Key(), originIt.Data());
+        }
       }
     }
   }
 
   // Step 2: Without the lock, generate JS datastructure for snapshotting
   JS::Rooted<JSObject*> rootObj(aCx, JS_NewPlainObject(aCx));
   if (NS_WARN_IF(!rootObj)) {
     return NS_ERROR_FAILURE;
   }
   aResult.setObject(*rootObj);
   for (auto iter = copy.ConstIter(); !iter.Done(); iter.Next()) {
-    const auto& origins = iter.Data();
-    JS::RootedObject originsArrayObj(aCx,
-                                     JS_NewArrayObject(aCx, origins.Length()));
-    if (NS_WARN_IF(!originsArrayObj)) {
+    JS::RootedObject originsObj(aCx, JS_NewPlainObject(aCx));
+    if (NS_WARN_IF(!originsObj)) {
       return NS_ERROR_FAILURE;
     }
     if (!JS_DefineProperty(aCx, rootObj, GetNameForMetricID(iter.Key()),
-                           originsArrayObj, JSPROP_ENUMERATE)) {
+                           originsObj, JSPROP_ENUMERATE)) {
       NS_WARNING("Failed to define property in origin snapshot.");
       return NS_ERROR_FAILURE;
     }
 
-    for (uint32_t i = 0; i < origins.Length(); ++i) {
-      JS::RootedValue origin(aCx);
-      origin.setString(ToJSString(aCx, origins[i]));
-      if (!JS_DefineElement(aCx, originsArrayObj, i, origin,
-                            JSPROP_ENUMERATE)) {
-        NS_WARNING("Failed to define element in origin snapshot array.");
+    auto originIt = iter.Data().ConstIter();
+    for (; !originIt.Done(); originIt.Next()) {
+      if (!JS_DefineProperty(aCx, originsObj,
+                             nsPromiseFlatCString(originIt.Key()).get(),
+                             originIt.Data(), JSPROP_ENUMERATE)) {
+        NS_WARNING("Failed to define origin and count in snapshot.");
         return NS_ERROR_FAILURE;
       }
     }
   }
 
   return NS_OK;
 }
 
@@ -407,17 +436,17 @@ nsresult TelemetryOrigin::GetEncodedOrig
     }
 
     if (aClear) {
       // I'd really prefer to clear after we're sure the snapshot didn't go
       // awry, but we can't hold a lock preventing recording while using JS
       // APIs. And replaying any interleaving recording sounds like too much
       // squeeze for not enough juice.
 
-      gMetricToOriginsMap->Clear();
+      gMetricToOriginBag->Clear();
     }
   }
 
   // Step 2: Don't need the lock to prio-encode and base64-encode
   nsTArray<Pair<nsCString, Pair<nsCString, nsCString>>> prioData;
   for (auto& metricData : appEncodedMetricData) {
     auto& boolVectors = metricData.second();
     for (uint32_t i = 0; i < boolVectors.Length(); ++i) {
@@ -512,34 +541,31 @@ nsresult TelemetryOrigin::GetEncodedOrig
  */
 void TelemetryOrigin::ClearOrigins() {
   StaticMutexAutoLock lock(gTelemetryOriginMutex);
 
   if (!gInitDone) {
     return;
   }
 
-  gMetricToOriginsMap->Clear();
+  gMetricToOriginBag->Clear();
 }
 
 size_t TelemetryOrigin::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) {
   StaticMutexAutoLock locker(gTelemetryOriginMutex);
   size_t n = 0;
 
   if (!gInitDone) {
     return 0;
   }
 
-  n += gMetricToOriginsMap->ShallowSizeOfIncludingThis(aMallocSizeOf);
-  auto iter = gMetricToOriginsMap->ConstIter();
+  n += gMetricToOriginBag->ShallowSizeOfIncludingThis(aMallocSizeOf);
+  auto iter = gMetricToOriginBag->ConstIter();
   for (; !iter.Done(); iter.Next()) {
+    // The string hashkey and count should both be contained by the hashtable.
     n += iter.Data().ShallowSizeOfIncludingThis(aMallocSizeOf);
-    for (const auto& origin : iter.Data()) {
-      // nsTArray's shallow size should include the strings' `this`
-      n += origin.SizeOfExcludingThisIfUnshared(aMallocSizeOf);
-    }
   }
 
   // The string hashkey and ID should both be contained within the hashtable.
   n += gOriginToIndexMap->ShallowSizeOfIncludingThis(aMallocSizeOf);
 
   return n;
 }
--- a/toolkit/components/telemetry/core/nsITelemetry.idl
+++ b/toolkit/components/telemetry/core/nsITelemetry.idl
@@ -661,17 +661,17 @@ interface nsITelemetry : nsISupports
   [implicit_jscontext]
   Promise gatherMemory();
 
   /**
    * Serializes the per-origin data in plain text, optionally clearing
    * the storage. Only to be used by about:telemetry.
    *
    * The returned structure looks like:
-   *   { metric: [origin1, origin2, ...], ...}
+   *   { metric: {origin1: count1, origin2: count2, ...}, ...}
    *
    * @param aClear Whether to clear the storage. Default false.
    * @return a snapshot of the per-origin data.
    */
   [implicit_jscontext]
   jsval getOriginSnapshot([optional] in boolean aClear);
 
   /**
--- a/toolkit/components/telemetry/docs/collection/origin.rst
+++ b/toolkit/components/telemetry/docs/collection/origin.rst
@@ -88,21 +88,21 @@ It returns a Promise which resolves to a
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 This JS API provides a snapshot of the unencrypted storage of unsent Origin Telemetry, optionally clearing that storage.
 It returns a structure of the form:
 
 .. code-block:: js
 
   {
-    "categoryname.metric_name": [
-      "origin1",
-      "origin2",
+    "categoryname.metric_name": {
+      "origin1": count1,
+      "origin2": count2,
       ...
-    ],
+    },
     ...
   }
 
 .. important::
 
     This API is only intended to be used by ``about:telemetry`` and tests.
 
 .. _origin.example:
@@ -119,18 +119,18 @@ This means we need two metrics ``content
 Say "example.net" was blocked and "example.com" was exempted from blocking.
 Content Blocking calls ``Telemetry::RecordOrigin(OriginMetricID::ContentBlocking_Blocked, NS_LITERAL_CSTRING("example.net"))`` and ``Telemetry::RecordOrigin(OriginMetricID::ContentBlocking_Exempt, NS_LITERAL_CSTRING("example.com"))``.
 
 At this time a call to ``Telemetry.getOriginSnapshot()`` would return:
 
 .. code-block:: js
 
   {
-    "contentblocking.blocked": ["example.net"],
-    "contentblocking.exempt": ["example.com"],
+    "contentblocking.blocked": {"example.net": 1},
+    "contentblocking.exempt": {"example.com": 1},
   }
 
 Later, Origin Telemetry will get the encoded snapshot (clearing the storage) and assemble it with other information into a :doc:`"prio" ping <../data/prio-ping>` which will then be submitted.
 
 .. _origin.encoding:
 
 Encoding
 ========
--- a/toolkit/components/telemetry/tests/gtest/TestOrigins.cpp
+++ b/toolkit/components/telemetry/tests/gtest/TestOrigins.cpp
@@ -39,31 +39,20 @@ TEST_F(TelemetryTestFixture, RecordOrigi
       << "Origin snapshot must not be null/undefined.";
 
   JS::RootedValue origins(aCx);
   JS::RootedObject snapshotObj(aCx, &originSnapshot.toObject());
   ASSERT_TRUE(JS_GetProperty(aCx, snapshotObj, telemetryTest1.get(), &origins))
       << "telemetry.test_test1 must be in the snapshot.";
 
   JS::RootedObject originsObj(aCx, &origins.toObject());
-  bool isArray = false;
-  ASSERT_TRUE(JS_IsArrayObject(aCx, originsObj, &isArray) && isArray)
-      << "The metric's origins must be in an array.";
-
-  uint32_t length = 0;
-  ASSERT_TRUE(JS_GetArrayLength(aCx, originsObj, &length) && length == 1)
-      << "Length of returned array must be 1.";
-
-  JS::RootedValue origin(aCx);
-  ASSERT_TRUE(JS_GetElement(aCx, originsObj, 0, &origin));
-  ASSERT_TRUE(origin.isString());
-  nsAutoJSString jsStr;
-  ASSERT_TRUE(jsStr.init(aCx, origin));
-  ASSERT_TRUE(NS_ConvertUTF16toUTF8(jsStr) == doubleclick)
-      << "Origin must be faithfully stored and snapshotted.";
+  JS::RootedValue count(aCx);
+  ASSERT_TRUE(JS_GetProperty(aCx, originsObj, doubleclick.get(), &count));
+  ASSERT_TRUE(count.isInt32() && count.toInt32() == 1)
+      << "Must have recorded the origin exactly once.";
 
   // Now test that the snapshot didn't clear things out.
   GetOriginSnapshot(aCx, &originSnapshot);
   ASSERT_FALSE(originSnapshot.isNullOrUndefined());
   JS::RootedObject unemptySnapshotObj(aCx, &originSnapshot.toObject());
   JS::Rooted<JS::IdVector> ids(aCx, JS::IdVector(aCx));
   ASSERT_TRUE(JS_Enumerate(aCx, unemptySnapshotObj, &ids));
   ASSERT_GE(ids.length(), (unsigned)0) << "Returned object must not be empty.";
@@ -88,33 +77,20 @@ TEST_F(TelemetryTestFixture, RecordOrigi
       << "Origin snapshot must not be null/undefined.";
 
   JS::RootedValue origins(aCx);
   JS::RootedObject snapshotObj(aCx, &originSnapshot.toObject());
   ASSERT_TRUE(JS_GetProperty(aCx, snapshotObj, telemetryTest1.get(), &origins))
       << "telemetry.test_test1 must be in the snapshot.";
 
   JS::RootedObject originsObj(aCx, &origins.toObject());
-  bool isArray = false;
-  ASSERT_TRUE(JS_IsArrayObject(aCx, originsObj, &isArray) && isArray)
-      << "The metric's origins must be in an array.";
-
-  uint32_t length = 0;
-  ASSERT_TRUE(JS_GetArrayLength(aCx, originsObj, &length) && length == 2)
-      << "Length of returned array must be 1.";
-
-  for (uint32_t i = 0; i < length; ++i) {
-    JS::RootedValue origin(aCx);
-    ASSERT_TRUE(JS_GetElement(aCx, originsObj, i, &origin));
-    ASSERT_TRUE(origin.isString());
-    nsAutoJSString jsStr;
-    ASSERT_TRUE(jsStr.init(aCx, origin));
-    ASSERT_TRUE(NS_ConvertUTF16toUTF8(jsStr) == doubleclick)
-        << "Origin must be faithfully stored and snapshotted.";
-  }
+  JS::RootedValue count(aCx);
+  ASSERT_TRUE(JS_GetProperty(aCx, originsObj, doubleclick.get(), &count));
+  ASSERT_TRUE(count.isInt32() && count.toInt32() == 2)
+      << "Must have recorded the origin exactly twice.";
 
   // Now check that snapshotting with clear actually cleared it.
   GetOriginSnapshot(aCx, &originSnapshot);
   ASSERT_FALSE(originSnapshot.isNullOrUndefined());
   JS::RootedObject emptySnapshotObj(aCx, &originSnapshot.toObject());
   JS::Rooted<JS::IdVector> ids(aCx, JS::IdVector(aCx));
   ASSERT_TRUE(JS_Enumerate(aCx, emptySnapshotObj, &ids));
   ASSERT_EQ(ids.length(), (unsigned)0) << "Returned object must be empty.";
@@ -139,52 +115,38 @@ TEST_F(TelemetryTestFixture, RecordUnkno
       << "Origin snapshot must not be null/undefined.";
 
   JS::RootedValue origins(aCx);
   JS::RootedObject snapshotObj(aCx, &originSnapshot.toObject());
   ASSERT_TRUE(JS_GetProperty(aCx, snapshotObj, telemetryTest1.get(), &origins))
       << "telemetry.test_test1 must be in the snapshot.";
 
   JS::RootedObject originsObj(aCx, &origins.toObject());
-  bool isArray = false;
-  ASSERT_TRUE(JS_IsArrayObject(aCx, originsObj, &isArray) && isArray)
-      << "The metric's origins must be in an array.";
-
-  uint32_t length = 0;
-  ASSERT_TRUE(JS_GetArrayLength(aCx, originsObj, &length) && length == 1)
-      << "Length of returned array must be 1.";
-
-  JS::RootedValue origin(aCx);
-  ASSERT_TRUE(JS_GetElement(aCx, originsObj, 0, &origin));
-  ASSERT_TRUE(origin.isString());
-  nsAutoJSString jsStr;
-  ASSERT_TRUE(jsStr.init(aCx, origin));
-  ASSERT_TRUE(NS_ConvertUTF16toUTF8(jsStr) == nsLiteralCString("__UNKNOWN__"))
-      << "Unknown origin must be faithfully stored and snapshotted.";
+  JS::RootedValue count(aCx);
+  ASSERT_TRUE(JS_GetProperty(aCx, originsObj, "__UNKNOWN__", &count));
+  ASSERT_TRUE(count.isInt32() && count.toInt32() == 1)
+      << "Must have recorded the unknown origin exactly once.";
 
   // Record a second, different unknown origin and ensure only one is stored.
   Telemetry::RecordOrigin(OriginMetricID::TelemetryTest_Test1, unknown2);
 
   GetOriginSnapshot(aCx, &originSnapshot);
 
   ASSERT_FALSE(originSnapshot.isNullOrUndefined())
       << "Origin snapshot must not be null/undefined.";
 
   JS::RootedObject snapshotObj2(aCx, &originSnapshot.toObject());
   ASSERT_TRUE(JS_GetProperty(aCx, snapshotObj2, telemetryTest1.get(), &origins))
       << "telemetry.test_test1 must be in the snapshot.";
 
   JS::RootedObject originsObj2(aCx, &origins.toObject());
-  isArray = false;
-  ASSERT_TRUE(JS_IsArrayObject(aCx, originsObj2, &isArray) && isArray)
-      << "The metric's origins must be in an array.";
-
-  length = 0;
-  ASSERT_TRUE(JS_GetArrayLength(aCx, originsObj2, &length) && length == 1)
-      << "Length of returned array must be 1.";
+  JS::RootedValue count2(aCx);
+  ASSERT_TRUE(JS_GetProperty(aCx, originsObj2, "__UNKNOWN__", &count2));
+  ASSERT_TRUE(count2.isInt32() && count2.toInt32() == 1)
+      << "Must have recorded the unknown origin exactly once.";
 }
 
 TEST_F(TelemetryTestFixture, EncodedSnapshot) {
   AutoJSContextWithGlobal cx(mCleanGlobal);
   JSContext* aCx = cx.GetJSContext();
 
   Unused << mTelemetry->ClearOrigins();
 
@@ -238,25 +200,32 @@ class MockObserver final : public nsIObs
 
  private:
   ~MockObserver() = default;
 };
 
 NS_IMPL_ISUPPORTS(MockObserver, nsIObserver);
 
 TEST_F(TelemetryTestFixture, OriginTelemetryNotifiesTopic) {
+  Unused << mTelemetry->ClearOrigins();
+
   const char* kTopic = "origin-telemetry-storage-limit-reached";
   NS_NAMED_LITERAL_CSTRING(doubleclick, "doubleclick.de");
+  NS_NAMED_LITERAL_CSTRING(fb, "fb.com");
 
   MockObserver* mo = new MockObserver();
   nsCOMPtr<nsIObserver> nsMo(mo);
   EXPECT_CALL(*mo, Mobserve(StrEq(kTopic))).Times(1);
 
   nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
   ASSERT_TRUE(os);
   os->AddObserver(nsMo, kTopic, false);
 
   for (int i = 0; i < 10; ++i) {
+    if (i < 9) {
+      // Let's ensure we only notify the once.
+      Telemetry::RecordOrigin(OriginMetricID::TelemetryTest_Test1, fb);
+    }
     Telemetry::RecordOrigin(OriginMetricID::TelemetryTest_Test1, doubleclick);
   }
 
   os->RemoveObserver(nsMo, kTopic);
 }