Bug 1127914 - Part 2 - Duplicate normal histograms for double submission. r=vladan

2015-02-25 23:54:32 +01:00 · 2015-02-25 23:54:32 +01:00 · 75c672149a
--- a/toolkit/components/telemetry/Telemetry.cpp
+++ b/toolkit/components/telemetry/Telemetry.cpp
@ -700,6 +700,9 @@ private:
  nsresult GetHistogramByName(const nsACString &name, Histogram **ret);
  bool ShouldReflectHistogram(Histogram *h);
  void IdentifyCorruptHistograms(StatisticsRecorder::Histograms &hs);
+  nsresult CreateHistogramSnapshots(JSContext *cx,
+                                    JS::MutableHandle<JS::Value> ret,
+                                    bool subsession);
  typedef StatisticsRecorder::Histograms::iterator HistogramIterator;

  struct AddonHistogramInfo {
@ -995,6 +998,89 @@ GetHistogramByEnumId(Telemetry::ID id, Histogram **ret)
  return NS_OK;
 }

+/**
+ * This clones a histogram |existing| with the id |existingId| to a
+ * new histogram with the name |newName|.
+ * For simplicity this is limited to registered histograms.
+ */
+Histogram*
+CloneHistogram(const nsACString& newName, Telemetry::ID existingId,
+               Histogram& existing)
+{
+  const TelemetryHistogram &info = gHistograms[existingId];
+  Histogram *clone = nullptr;
+  nsresult rv;
+
+  rv = HistogramGet(PromiseFlatCString(newName).get(), info.expiration(),
+                    info.histogramType, existing.declared_min(),
+                    existing.declared_max(), existing.bucket_count(),
+                    true, &clone);
+  if (NS_FAILED(rv)) {
+    return nullptr;
+  }
+
+  Histogram::SampleSet ss;
+  existing.SnapshotSample(&ss);
+  clone->AddSampleSet(ss);
+
+  return clone;
+}
+
+/**
+ * This clones a histogram with the id |existingId| to a new histogram
+ * with the name |newName|.
+ * For simplicity this is limited to registered histograms.
+ */
+Histogram*
+CloneHistogram(const nsACString& newName, Telemetry::ID existingId)
+{
+  Histogram *existing = nullptr;
+  nsresult rv = GetHistogramByEnumId(existingId, &existing);
+  if (NS_FAILED(rv)) {
+    return nullptr;
+  }
+
+  return CloneHistogram(newName, existingId, *existing);
+}
+
+Histogram*
+GetSubsessionHistogram(Histogram& existing)
+{
+  Telemetry::ID id;
+  nsresult rv = TelemetryImpl::GetHistogramEnumId(existing.histogram_name().c_str(), &id);
+  if (NS_FAILED(rv) || gHistograms[id].keyed) {
+    return nullptr;
+  }
+
+  static Histogram* subsession[Telemetry::HistogramCount] = {};
+  if (subsession[id]) {
+    return subsession[id];
+  }
+
+  NS_NAMED_LITERAL_CSTRING(prefix, SUBSESSION_HISTOGRAM_PREFIX);
+  nsDependentCString existingName(gHistograms[id].id());
+  if (StringBeginsWith(existingName, prefix)) {
+    return nullptr;
+  }
+
+  nsCString subsessionName(prefix);
+  subsessionName.Append(existingName);
+
+  subsession[id] = CloneHistogram(subsessionName, id, existing);
+  return subsession[id];
+}
+
+nsresult
+HistogramAdd(Histogram& histogram, int32_t value)
+{
+  histogram.Add(value);
+  if (Histogram* subsession = GetSubsessionHistogram(histogram)) {
+    subsession->Add(value);
+  }
+
+  return NS_OK;
+}
+
 bool
 FillRanges(JSContext *cx, JS::Handle<JSObject*> array, Histogram *h)
 {
@ -1093,6 +1179,7 @@ JSHistogram_Add(JSContext *cx, unsigned argc, JS::Value *vp)
  }

  Histogram *h = static_cast<Histogram*>(JS_GetPrivate(obj));
+  MOZ_ASSERT(h);
  Histogram::ClassType type = h->histogram_type();

  int32_t value = 1;
@ -1114,7 +1201,7 @@ JSHistogram_Add(JSContext *cx, unsigned argc, JS::Value *vp)
  }

  if (TelemetryImpl::CanRecord()) {
-    h->Add(value);
+    HistogramAdd(*h, value);
  }

  return true;
@ -1156,8 +1243,28 @@ JSHistogram_Clear(JSContext *cx, unsigned argc, JS::Value *vp)
    return false;
  }

+  bool onlySubsession = false;
+  JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
+
+  if (args.length() >= 1) {
+    if (!args[0].isBoolean()) {
+      JS_ReportError(cx, "Not a boolean");
+      return false;
+    }
+
+    onlySubsession = JS::ToBoolean(args[0]);
+  }
+
  Histogram *h = static_cast<Histogram*>(JS_GetPrivate(obj));
-  h->Clear();
+  MOZ_ASSERT(h);
+  if(!onlySubsession) {
+    h->Clear();
+  }
+
+  if (Histogram* subsession = GetSubsessionHistogram(*h)) {
+    subsession->Clear();
+  }
+
  return true;
 }

@ -1863,25 +1970,12 @@ TelemetryImpl::HistogramFrom(const nsACString &name, const nsACString &existing_
  if (NS_FAILED(rv)) {
    return rv;
  }
-  const TelemetryHistogram &p = gHistograms[id];

-  Histogram *existing;
-  rv = GetHistogramByEnumId(id, &existing);
-  if (NS_FAILED(rv)) {
-    return rv;
+  Histogram* clone = CloneHistogram(name, id);
+  if (!clone) {
+    return NS_ERROR_FAILURE;
  }

-  Histogram *clone;
-  rv = HistogramGet(PromiseFlatCString(name).get(), p.expiration(),
-                    p.histogramType, existing->declared_min(),
-                    existing->declared_max(), existing->bucket_count(),
-                    true, &clone);
-  if (NS_FAILED(rv))
-    return rv;
-
-  Histogram::SampleSet ss;
-  existing->SnapshotSample(&ss);
-  clone->AddSampleSet(ss);
  return WrapAndReturnHistogram(clone, cx, ret);
 }

@ -2064,8 +2158,10 @@ TelemetryImpl::UnregisterAddonHistograms(const nsACString &id)
  return NS_OK;
 }

-NS_IMETHODIMP
-TelemetryImpl::GetHistogramSnapshots(JSContext *cx, JS::MutableHandle<JS::Value> ret)
+nsresult
+TelemetryImpl::CreateHistogramSnapshots(JSContext *cx,
+                                        JS::MutableHandle<JS::Value> ret,
+                                        bool subsession)
 {
  JS::Rooted<JSObject*> root_obj(cx, JS_NewPlainObject(cx));
  if (!root_obj)
@ -2106,6 +2202,14 @@ TelemetryImpl::GetHistogramSnapshots(JSContext *cx, JS::MutableHandle<JS::Value>
      continue;
    }

+    Histogram* original = h;
+    if (subsession) {
+      h = GetSubsessionHistogram(*h);
+      if (!h) {
+        continue;
+      }
+    }
+
    hobj = JS_NewPlainObject(cx);
    if (!hobj) {
      return NS_ERROR_FAILURE;
@ -2119,8 +2223,8 @@ TelemetryImpl::GetHistogramSnapshots(JSContext *cx, JS::MutableHandle<JS::Value>
    case REFLECT_FAILURE:
      return NS_ERROR_FAILURE;
    case REFLECT_OK:
-      if (!JS_DefineProperty(cx, root_obj, h->histogram_name().c_str(), hobj,
-                             JSPROP_ENUMERATE)) {
+      if (!JS_DefineProperty(cx, root_obj, original->histogram_name().c_str(),
+                             hobj, JSPROP_ENUMERATE)) {
        return NS_ERROR_FAILURE;
      }
    }
@ -2128,6 +2232,18 @@ TelemetryImpl::GetHistogramSnapshots(JSContext *cx, JS::MutableHandle<JS::Value>
  return NS_OK;
 }

+NS_IMETHODIMP
+TelemetryImpl::GetHistogramSnapshots(JSContext *cx, JS::MutableHandle<JS::Value> ret)
+{
+  return CreateHistogramSnapshots(cx, ret, false);
+}
+
+NS_IMETHODIMP
+TelemetryImpl::GetSubsessionHistogramSnapshots(JSContext *cx, JS::MutableHandle<JS::Value> ret)
+{
+  return CreateHistogramSnapshots(cx, ret, true);
+}
+
 bool
 TelemetryImpl::CreateHistogramForAddon(const nsACString &name,
                                       AddonHistogramInfo &info)
@ -3358,7 +3474,7 @@ Accumulate(ID aHistogram, uint32_t aSample)
  Histogram *h;
  nsresult rv = GetHistogramByEnumId(aHistogram, &h);
  if (NS_SUCCEEDED(rv))
-    h->Add(aSample);
+    HistogramAdd(*h, aSample);
 }

 void
@ -3389,7 +3505,7 @@ Accumulate(const char* name, uint32_t sample)
  Histogram *h;
  rv = GetHistogramByEnumId(id, &h);
  if (NS_SUCCEEDED(rv)) {
-    h->Add(sample);
+    HistogramAdd(*h, sample);
  }
 }

--- a/toolkit/components/telemetry/nsITelemetry.idl
+++ b/toolkit/components/telemetry/nsITelemetry.idl
@ -12,7 +12,7 @@ interface nsIFetchTelemetryDataCallback : nsISupports
  void complete();
 };

-[scriptable, uuid(c782cf96-7f44-45ac-8d76-e0d1b174e562)]
+[scriptable, uuid(749d0bdf-8c7a-4b4a-bb32-eaf3d69af45e)]
 interface nsITelemetry : nsISupports
 {
  /**
@ -39,7 +39,7 @@ interface nsITelemetry : nsISupports
  const unsigned long DATASET_RELEASE_CHANNEL_OPTIN = 1;


-  /*
+  /**
   * An object containing a snapshot from all of the currently registered histograms.
   * { name1: {data1}, name2:{data2}...}
   * where data is consists of the following properties:
@ -55,6 +55,12 @@ interface nsITelemetry : nsISupports
  [implicit_jscontext]
  readonly attribute jsval histogramSnapshots;

+  /**
+   * As histogramSnapshots, except this contains the internally duplicated histograms for subsession telemetry.
+   */
+  [implicit_jscontext]
+  readonly attribute jsval subsessionHistogramSnapshots;
+
  /**
   * The amount of time, in milliseconds, that the last session took
   * to shutdown.  Reads as 0 to indicate failure.
--- a/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
+++ b/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@ -605,6 +605,57 @@ function test_datasets()
  Assert.ok(registered.has("TELEMETRY_TEST_KEYED_RELEASE_OPTOUT"));
 }

+function test_subsession() {
+  const ID = "TELEMETRY_TEST_COUNT";
+  let h = Telemetry.getHistogramById(ID);
+
+  // Both original and duplicate should start out the same.
+  h.clear();
+  let snapshot = Telemetry.histogramSnapshots;
+  let subsession = Telemetry.subsessionHistogramSnapshots;
+  Assert.ok(!(ID in snapshot));
+  Assert.ok(!(ID in subsession));
+
+  // They should instantiate and pick-up the count.
+  h.add(1);
+  snapshot = Telemetry.histogramSnapshots;
+  subsession = Telemetry.subsessionHistogramSnapshots;
+  Assert.ok(ID in snapshot);
+  Assert.ok(ID in subsession);
+  Assert.equal(snapshot[ID].sum, 1);
+  Assert.equal(subsession[ID].sum, 1);
+
+  // They should still reset properly.
+  h.clear();
+  snapshot = Telemetry.histogramSnapshots;
+  subsession = Telemetry.subsessionHistogramSnapshots;
+  Assert.ok(!(ID in snapshot));
+  Assert.ok(!(ID in subsession));
+
+  // Both should instantiate and pick-up the count.
+  h.add(1);
+  snapshot = Telemetry.histogramSnapshots;
+  subsession = Telemetry.subsessionHistogramSnapshots;
+  Assert.equal(snapshot[ID].sum, 1);
+  Assert.equal(subsession[ID].sum, 1);
+
+  // Check that we are able to only reset the duplicate histogram.
+  h.clear(true);
+  snapshot = Telemetry.histogramSnapshots;
+  subsession = Telemetry.subsessionHistogramSnapshots;
+  Assert.ok(ID in snapshot);
+  Assert.ok(ID in subsession);
+  Assert.equal(snapshot[ID].sum, 1);
+  Assert.equal(subsession[ID].sum, 0);
+
+  // Both should register the next count.
+  h.add(1);
+  snapshot = Telemetry.histogramSnapshots;
+  subsession = Telemetry.subsessionHistogramSnapshots;
+  Assert.equal(snapshot[ID].sum, 2);
+  Assert.equal(subsession[ID].sum, 1);
+}
+
 function test_keyed_subsession() {
  let h = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_FLAG");
  const KEY = "foo";
@ -673,5 +724,6 @@ function run_test()
  test_expired_histogram();
  test_keyed_histogram();
  test_datasets();
+  test_subsession();
  test_keyed_subsession();
 }