Closed Bug 1862922 Opened 7 months ago Closed 6 months ago

Run telemetry to see whether we can drop explicit support for day of week late in the date format

Categories

(Core :: JavaScript: Standard Library, task, P3)

task

Tracking

()

RESOLVED FIXED
121 Branch
Tracking Status
firefox121 --- fixed

People

(Reporter: vinny.diehl, Assigned: vinny.diehl)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

While working on day of week support in bug 1617562, I discovered that we accept day of week in places which other engines reject:

Format JSC SM V8
"Sep 26 1995 Tues" ✔️
"Sep 26 Tues 1995" ✔️
"Sep 26 1995 10:Tues:00" ✔️

See the aforementioned bug and its patch for more details about where day of week should be accepted.

The purpose of this bug is to collect data on whether removing support for these oddball patterns would break anything. If my calculations are correct, after D192560 lands, action should never == 0 except in the oddball cases mentioned, so we'll wait for that to land.

Blocks: 1274354
Priority: -- → P3

Obviously I'll need to pass the JSContext into ParseDate, but then what should I use as the JSObject required by JSRuntime::setUseCounter?

Flags: needinfo?(arai.unmht)

Thank you for looking into this!

Here's a document for the use counter.
Sorry I should've pointed this document first.

https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/collection/use-counters.html

Inside SpiderMonkey, the use counter is implemented as a callback to the embedding (Gecko).

https://searchfox.org/mozilla-central/rev/f9ac1c0115f4ba80893c3c305a14907ad5852473/js/src/vm/Runtime.cpp#290-299

void JSRuntime::setUseCounter(JSObject* obj, JSUseCounter counter) {
  if (useCounterCallback) {
    (*useCounterCallback)(obj, counter);
  }
}

void JSRuntime::setUseCounterCallback(JSRuntime* rt,
                                      JSSetUseCounterCallback callback) {
  rt->useCounterCallback = callback;
}

The callback is set by JS_SetSetUseCounterCallback,

https://searchfox.org/mozilla-central/rev/f9ac1c0115f4ba80893c3c305a14907ad5852473/js/src/vm/UsageStatistics.cpp#17-19

void JS_SetSetUseCounterCallback(JSContext* cx,
                                 JSSetUseCounterCallback callback) {
  cx->runtime()->setUseCounterCallback(cx->runtime(), callback);

and it's set here, with SetUseCounterCallback.

https://searchfox.org/mozilla-central/rev/f9ac1c0115f4ba80893c3c305a14907ad5852473/js/xpconnect/src/XPCJSRuntime.cpp#2891

JS_SetSetUseCounterCallback(cx, SetUseCounterCallback);

the SetUseCounterCallback converts the JS-specific use couter type (JSUseCounter) to the global use counter type (UseCounter).

https://searchfox.org/mozilla-central/rev/f9ac1c0115f4ba80893c3c305a14907ad5852473/js/xpconnect/src/XPCJSRuntime.cpp#2584-2591

static void SetUseCounterCallback(JSObject* obj, JSUseCounter counter) {
  switch (counter) {
    case JSUseCounter::ASMJS:
      SetUseCounter(obj, eUseCounter_custom_JS_asmjs);
      break;
    case JSUseCounter::WASM:
      SetUseCounter(obj, eUseCounter_custom_JS_wasm);
      break;

The passed JSObject* is used for retrieving the document associated for the counter
(so, the counter value means "this feature is used in this document").
The object can be any object that belongs to the document's global.

In the Date's case, it can for example be Date constructor or the Date prototype, or just the global object cx->global().

https://searchfox.org/mozilla-central/rev/f9ac1c0115f4ba80893c3c305a14907ad5852473/dom/bindings/BindingUtils.cpp#4096-4100

void SetUseCounter(JSObject* aObject, UseCounter aUseCounter) {
  nsGlobalWindowInner* win =
      xpc::WindowGlobalOrNull(js::UncheckedUnwrap(aObject));
  if (win && win->GetDocument()) {
    win->GetDocument()->SetUseCounter(aUseCounter);

The existing consumer in wasm uses the wasm instance object as the JSObject* parameter.

https://searchfox.org/mozilla-central/rev/f9ac1c0115f4ba80893c3c305a14907ad5852473/js/src/wasm/WasmModule.cpp#1011,1015,1056-1058

instance.set(WasmInstanceObject::create(
...
    std::move(maybeDebug)));
...
JSUseCounter useCounter =
    metadata().isAsmJS() ? JSUseCounter::ASMJS : JSUseCounter::WASM;
cx->runtime()->setUseCounter(instance, useCounter);

Then, the use counter type is defined in dom/base/UseCounters.conf.
SpiderMonkey's use counter uses custom type (see the document above).

https://searchfox.org/mozilla-central/rev/f9ac1c0115f4ba80893c3c305a14907ad5852473/dom/base/UseCounters.conf#71-73

// JavaScript feature usage
custom JS_asmjs uses asm.js
custom JS_wasm uses WebAssembly

and that's processed by gen-usecounters.py script:

https://searchfox.org/mozilla-central/rev/f9ac1c0115f4ba80893c3c305a14907ad5852473/dom/base/moz.build#607-611

GeneratedFile(
    "UseCounterList.h",
    script="gen-usecounters.py",
    entry_point="use_counter_list",
    inputs=["UseCounters.conf"],

The JS-specific use counter type is defined in js/public/friend/UsageStatistics.h,
and it's converted in SetUseCounterCallback above.

https://searchfox.org/mozilla-central/rev/f9ac1c0115f4ba80893c3c305a14907ad5852473/js/public/friend/UsageStatistics.h#94

enum class JSUseCounter { ASMJS, WASM };

let me know if there's anything unclear.

Flags: needinfo?(arai.unmht)

Thank you for the incredibly detailed breakdown!

My plan is to set a use counter for every successfully parsed implementation-specific Date format (so, ParseISOStyleDate and rejected formats will not be counted), and another counter when one of the offending formats are encountered, so that we may calculate what percentage of implementation-specific dates encountered require support for such formats.

I'd like to make unit tests to confirm that the offending formats are indeed sending telemetry, and that the normal formats parsed by the bug 1617562 code are not sending telemetry. I found this in the documentation, but an example of how to use these functions would be helpful.

Flags: needinfo?(arai.unmht)
Assignee: nobody → vinny.diehl
Status: NEW → ASSIGNED

The patch I've just uploaded has been manually tested using about:telemetry, but if there's an easier/automated way, it'd be good to know for next time (and, I still will add the tests, just to be sure).

Attachment #9363144 - Flags: data-review?(chutten)

(In reply to Vinny Diehl from comment #3)

I'd like to make unit tests to confirm that the offending formats are indeed sending telemetry, and that the normal formats parsed by the bug 1617562 code are not sending telemetry. I found this in the documentation, but an example of how to use these functions would be helpful.

:chutten, how is the use counter supposed to be tested?
I've looked into the history of UseCounters.conf, but couldn't locate a patch with tests.

Flags: needinfo?(arai.unmht) → needinfo?(chutten)

It looks like I should be able to pull the histogram with Services.telemetry.getHistogramById("USE_COUNTER2_JS_LATE_WEEKDAY_DOCUMENT"), but I'm having trouble getting Date.parse to run in a way that affects the telemetry. I'm running as an xpcshell-test like this one:
https://searchfox.org/mozilla-central/source/js/xpconnect/tests/unit/test_js_memory_telemetry.js

However, simply executing new Date(...) directly in the add_task callback doesn't affect the telemetry, even after a few seconds delay. I also tried to eval the Date parse in Cu.evalInSandbox, but still nothing. I've also looked at the data from Services.telemetry.getSnapshotForHistograms("main", true).parent and can't find the histogram there either.

Any ideas what I'm doing wrong arai? Is there some particular way that I need to run the Date.parse function, or am I using the wrong type of test? If not, perhaps I'm going about pulling the telemetry data incorrectly.

Flags: needinfo?(arai.unmht)

good find :)

My guess is that, xpcshell doesn't work well with the use counter, given the use counter's data is associated with document, while the code executed in xpcshell isn't necessarily associated with document.

[1] https://searchfox.org/mozilla-central/rev/f9ac1c0115f4ba80893c3c305a14907ad5852473/dom/bindings/BindingUtils.cpp#4096-4100

void SetUseCounter(JSObject* aObject, UseCounter aUseCounter) {
  nsGlobalWindowInner* win =
      xpc::WindowGlobalOrNull(js::UncheckedUnwrap(aObject));
  if (win && win->GetDocument()) {
    win->GetDocument()->SetUseCounter(aUseCounter);

You could use "mochitest browser" instead.

example tests in https://searchfox.org/mozilla-central/source/js/xpconnect/tests/browser

Tests for mochitest browser is named browser_*.js in the source. e.g.: browser_dead_object.js,
and the file is listed in browser.toml file in the same directory. e.g.: browser.toml

The function passed to add_task is executed with the browser's scope, so I suppose the telemetry doesn't work here as well.
You need to open a tab and let the page opened in the tab execute the new Date(...), so that the result is associated with the document, and eventually reflected to the telemetry result.

https://searchfox.org/mozilla-central/rev/fe8b30e982c86d26ccf1f14d825c0de870b91f27/js/xpconnect/tests/browser/browser_dead_object.js#11

let newTab = await BrowserTestUtils.openNewForegroundTab(gBrowser, url);

the test can be run by ./mach test PATH_TO_TEST after ./mach build for browser.

Flags: needinfo?(arai.unmht)

Good point! Alas, I still seem to be screwing up somewhere. Here is the code I have so far, just to try and figure out how to get the telemetry to register:

const HIST_ID = "USE_COUNTER2_JS_LATE_WEEKDAY_DOCUMENT";
// This URL sets telemetry if I try it manually in a browser via `mach run`
const URL = "data:text/html;charset=utf-8,<script>new Date('Sep 26 1995 Tues')</script>";

add_task(async function test_date_telemetry() {
  console.log("***************************************************\n",
              "Before:", Services.telemetry.getHistogramById(HIST_ID)); //=> {}

  let newTab = await BrowserTestUtils.openNewForegroundTab(gBrowser, URL);

  // ghetto sleep for now, will eventually use a proper timeout loop if necessary
  let d = Date.now();
  while (Date.now() - d < 5000);

  console.log("***************************************************\n",
              "After:", Services.telemetry.getHistogramById(HIST_ID)); //=> {}
});

I have also tried saving the code to a proper .html file and opening the page in the mochitest that way, still nothing. I've also grepped the full list of histograms from getSnapshotForHistograms and there's still nothing in there.

Flags: needinfo?(arai.unmht)

I think you need to close the tab before getting the telemetry result.

SetUseCounter above calls Document::SetUseCounter,

https://searchfox.org/mozilla-central/rev/fe8b30e982c86d26ccf1f14d825c0de870b91f27/dom/bindings/BindingUtils.cpp#4096,4100

void SetUseCounter(JSObject* aObject, UseCounter aUseCounter) {
...
    win->GetDocument()->SetUseCounter(aUseCounter);

and Document::SetUseCounter stores the result into Document::mUseCounters

https://searchfox.org/mozilla-central/rev/fe8b30e982c86d26ccf1f14d825c0de870b91f27/dom/base/Document.h#3613-3614

void SetUseCounter(UseCounter aUseCounter) {
  mUseCounters[aUseCounter] = true;

the Document::mUseCounters is used in the following

https://searchfox.org/mozilla-central/rev/fe8b30e982c86d26ccf1f14d825c0de870b91f27/dom/base/Document.cpp#16141,16171-16174,16184

void Document::ReportDocumentUseCounters() {
...
  // Report our per-document use counters.
  for (int32_t c = 0; c < eUseCounter_Count; ++c) {
    auto uc = static_cast<UseCounter>(c);
    if (!mUseCounters[uc]) {
...
    Telemetry::Accumulate(id, 1);

and Document::ReportDocumentUseCounters is called in Document::Destroy.

https://searchfox.org/mozilla-central/rev/fe8b30e982c86d26ccf1f14d825c0de870b91f27/dom/base/Document.cpp#11464,11471

void Document::Destroy() {
...
  ReportDocumentUseCounters();

so, try closing the tab with BrowserTestUtils.removeTab(tab); and then get the telemetry data

Flags: needinfo?(arai.unmht)

Man, you are fast with the detailed info! Unfortunately, that didn't work :(

When manually testing, the tab isn't required to be closed before the result shows up in about:telemetry, which is funny, because ReportDocumentUseCounters doesn't appear to be called anywhere but the destructor and nsXMLContentSink::OnTransformDone... perhaps this is being periodically updated elsewhere? Regardless, this may help to process the telemetry more quickly so that a timeout won't be necessary once we do figure this out, so I'll keep this in mind.

Any more ideas?

Flags: needinfo?(arai.unmht)

if the behavior can be observed in manual test, the thing to check is that whether it also works with mochitest's environment.

You can put await new Promise(() => {}); inside the test to pause it (the test waits for never-resolving promise there).
With the change, run the test, and once it pauses, manually operate on the browser window in the same way as manual test, and check about:telemetry.
(you'll need to pass --timeout=60000 or so to ./mach test ..., to avoid hitting timeout during manual test)

If you can observe the same behavior there, then the issue would be somewhere between the Services.telemetry.getHistogramById vs the about:telemetry's data source
If you cannot observe the same behavior there, then the issue would be something in the mochitest's environment
(possibility is that you need some configuration to enable telemetry there)

Flags: needinfo?(arai.unmht)

Nice tip on pausing the test, thanks!

The histograms for USE_COUNTER2_JS_LATE_WEEKDAY_DOCUMENT and USE_COUNTER2_JS_LATE_WEEKDAY_PAGE are both present in about:telemetry during the mochitest, so we've at least landed on the right way to trigger the telemetry during testing.

I'll wait to see if :chutten has any more insight on how to retrieve that data. :)

There are a couple of things likely going awry here:

  1. Telemetry.getHistogramById(...) returns a JSHistogram object which has methods on it. One of those methods is snapshot() which would give you what you needed if your histogram's data is on the parent process. If the use counter is on the content process, you get to use getSnapshotForHistograms, and learn about IPC in point (2).
  2. Telemetry IPC operates every couple of seconds. To wait for the data to arrive, the current art is to use waitForCondition and have it wait until the provided value matches the expected value.

You might find it easier to integrate your use counter test with an existing one like the ones in https://searchfox.org/mozilla-central/source/dom/base/test/browser_use_counters.js -- it has it all sorted out and ought to make your life easier. (And mine, as I'm adding Glean versions of use counters in bug 1852098, so I'll be adding Glean tests while I'm in the neighbourhood.). Another option is to wait for bug 1852098 to land, and then test the Glean version instead with its much nicer test APIs... but I think adding it to browser_use_counters.js is probably best.

Flags: needinfo?(chutten)

One of those methods is snapshot() which would give you what you needed if your histogram's data is on the parent process

Yesssss, thank you! The {} in the console.log output was throwing me off, I thought it just wasn't finding any histogram at all. In light of this information, I think a bespoke test is the easiest option, given what I'm trying to do. I have it working now, I'll have a patch up in a minute.

Comment on attachment 9363144 [details]
Request for data collection form

PRELIMINARY NOTES:

Use Counters have no in-built expiry mechanism, so I'm going to review this as though it will actually be a permanent collection.

Note that, after bug 1862546, most new use counters will be covered by its data review and not need individual reviews thereafter.

DATA COLLECTION REVIEW RESPONSE:

Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes.

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection can be controlled through Firefox's Preferences.

If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, Vinny Diehl is responsible.

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

Is the data collection request for default-on or default-off?

Default on for all channels.

Does the instrumentation include the addition of any new identifiers?

No.

Is the data collection covered by the existing Firefox privacy notice?

Yes.

Does the data collection use a third-party collection tool?

No.


Result: datareview+

Attachment #9363144 - Flags: data-review?(chutten) → data-review+
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/c6647b193462
Run telemetry on day of week support r=arai
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 121 Branch
See Also: → 1872793
See Also: → 1873186
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: