What were you doing?

(Tested on macOS, but like 90% sure this is a cross-platform issue)

1. open Firefox
2. enable devtools.chrome.enabled
3. open browser console
4. evaluate Services.prompt

What happened?

NS_ERROR_FAILURE exception logged in the console.

What should have happened?

It should show me all the properties on the object.

This broke sometime in January 2023, it appears:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=351f01ad9a4469b841cb66dab2fb4d1ec0fe7f0b&tochange=23c1be504632a25d402332e7db55eb5699509d9f

Given that pushlog, this looks like it's likely a result of bug 1808173, though it's not clear to me how/why off-hand, as in general the prompt service continued functioning just fine.

I notice that Services.droppedLinkHandler is also affected.

TBH, I'm not sure what's up here. It looks like devtools can't create the object for some reason, but it seems to work within the code...

The class and constructor names seem to match fine between the code & the components.conf files. They should possibly have 'singleton': True, but that doesn't seem to affect anything (other items on Services work just fine without that).

Also not sure why just changing from jsm to ESModule would affect this.

Maybe :arai might have an idea.

Set release status flags based on info from the regressing bug 1808173

Could this be related to Bug 1811826?

Symptoms at least seem vaguely similar. Hopefully Arai knows more.

It should be caused by the eager evaluation aborts the module evaluation after detecting effectful operation in the module script, and that results in the module being "failed to evaluate", and the failure persists in ES module loader.
There was exactly same type of issue before, and I think the solution was to mark the property effectful, so that eager evaluation doesn't try to read the value.
let me look for the bug and patch.

(I couldn't find the previous bug so far, but both this and bug 1811826 should be the same issue as that one...)

Then, nika's comment in bug 1811826 comment #6 is correct. Services properties are implemented with a resolve hook, and it's not detectable,
and the solution with tweaking the "pure functions" list won't work here.

Possible options are:

• (a) implement a new debugger hook for the resolve hook, and abort the eager evaluation there
• (b-1) add a special case in Services' resolve hook to abort on eager evaluation, and prevent the property resolution
• (b-2) add a special debugger hook in Services' resolve hook, and abort the eager evaluation there
• (c) add debugger hook for importESModule call from native code, and prevent importing module
• (d) use lazy JS getter with ChromeUtils.importESModule call, instead of resolve hook for Services properties, so that eager evaluation can detect ChromeUtils.importESModule call and abort there
• (e) suppress eager evaluation's force-termination while evaluating modules in Services' resolve hook
• (f) make the ES module loader somehow recoverable from eager evaluation's force-termination, and re-evaluate on the next import

but iirc (f) wasn't chosen last time because it's difficult to handle properly, which means things should be stopped before importing the module (a, b, c, and d), or avoid terminating the module evaluation (e).

(a) is the straightforward solution. the hook can receive the object and optionally the property name, and check if the object or its class is on the allow list (e.g. Function should be allowed to resolve name etc). The main concern is the performance effect.

(b-1) is slightly tricky. we need to provide a way to detect if "eager evaluation is ongoing".
(b-2) can solve it, but this is too specific and not sure if it fits "debugger API".

(c) is more generic than (b). given the issue is around module evaluation and a part of problem is that debugger cannot detect function calls inside the native code, adding the hook there would solve other module-related cases as well

(d) is the simplest solution, where it achieves the same thing as (c) while not introducing any new things. The main concern is the performance. if it doesn't affect the performance too much, I'm leaning toward it.

(e) might work well for eager evaluation's case, but it can break the case where people actually want's to debug the module evaluation inside the Services' resolve hook.

I don't know enough about the internals to really compare the relevant merits/costs of the described different approaches. However, I would say that on the whole performance of the Services object is actually pretty important given how frequently it is used in frontend JS - so much so that I could live with this being wontfixed if the alternative was paying a perf penalty here. But I would hope that in this specific circumstance it should be possible to have our cake and eat it...

An exception may be if we only pay the perf penalty the first time something on the object is deref'd (process-global-ly speaking, I hope). But even then this might prove unfortunate when it comes to content processes.

(Moving to the JS engine as it sounds like the solution will be there or in XPCOM or in the Services implementation - not in devtools itself.)

testing (d) with native getter, where eager evaluation should abort when calling unknown native getter.
https://treeherder.mozilla.org/jobs?repo=try&revision=9bff790c88935e99a5c231b6ab99db334576f250

Possible performance effects:

• Properties are defined when Services object is created. There are 55 properties, which means 55 atoms, 55 getter functions, and 55 setter functions are created
  • if we prohibit overriding the services properties, setter functions can be omitted, but the override actually happens in testcase (AppInfo.sys.mjs)
• The first property access becomes getter call. the implementation itself isn't much different than the resolve hook. And actually this can avoid the perfect hash, by keeping the array index of the service in the getter function. So, the cost is only about the getter function call
• The subsequent property access is regular data property access, and no change from the current way
  • at least this patch won't affect hot code

I'll test the performance and memory consumption

(btw, the previous bug was bug 1805235, which was fixed by bug 1806598)

result (awsy and talos-other so far): https://treeherder.mozilla.org/perfherder/compare?originalProject=try&newProject=try&newRevision=9bff790c88935e99a5c231b6ab99db334576f250&framework=4&originalRevision=406daaf4a3dfdabeca35b30dc6a8675328240c24&page=1

There's slight increase in the memory consumption (+1.11% Base Content JS opt fission)

:standard8, since you are the author of the regressor, bug 1808173, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Cancelling NI since the regression was indirect. Also as this is not my team/area, I don't think it is right that I triage it.

(In reply to Tooru Fujisawa [:arai] from comment #8)

Possible performance effects:

• Properties are defined when Services object is created. There are 55 properties, which means 55 atoms, 55 getter functions, and 55 setter functions are created
  • if we prohibit overriding the services properties, setter functions can be omitted, but the override actually happens in testcase (AppInfo.sys.mjs)

Actually, just delazifying the property before the assignment should work, given the property becomes plain data property.
I'll test with modifying the testcases.

Only with setter, with extended slot (+0.81% Base Content JS opt fission)
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=406daaf4a3dfdabeca35b30dc6a8675328240c24&newProject=try&newRevision=f3a116475a0954db9b4d3db78fbe908798674ae7&framework=4&page=1&showOnlyComparable=1

Dedicate native functions instead of extended slot for storing the index (+0.84% Base Content JS opt fission)
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=406daaf4a3dfdabeca35b30dc6a8675328240c24&newProject=try&newRevision=cb16fd34ba5b8638df72c6def2643130f3098d89&framework=4&page=1&showOnlyComparable=1

still it comes with slight memory cost.

I'll test (b-2) with "side-effectful resolve" hook.

(b-2) with "side-effectful resolve" hook has no memory consumption regression:
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=406daaf4a3dfdabeca35b30dc6a8675328240c24&newProject=try&newRevision=5450a35a92935d4ba9a936486a480c265d848955&framework=4&page=1

and also it affects only the initial access of the Services properties, and it has very low cost when there's no debugger.
Given this solves the issue with (a) where there's no easy way to figure out whether the resolve hook has side-effect if the hook is called for all resolve hooks, I'm going to go with it.

In order to abort eager evaluation when executing a resolve hook with side-effect,
especially the Services' hook which imports ES module, which is not undo-able,
introduce a dedicate debugger hook for side-effectful resolve hook.

Backed out for causing dt failures in browser_console_eager_eval_resolve.js.

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser/browser_console_eager_eval_resolve.js | Test timed out -

findDebuggees is returning a dead wrapper, and it causes the failure.
At the point of findAllGlobals call, the global should be alive. So the possibility is that the global gets destroyed while evaluating the map or for-of, or the call operations.
Anyway, the global being dead there is not something really unexpected, and I think we should filter dead global out in eval-with-debugger.js.

https://searchfox.org/mozilla-central/rev/6eb9b2d8f23c03ff6e203502a476cbdb0358a807/devtools/server/actors/webconsole/eval-with-debugger.js#456-458

for (const global of targetActorDbg.findDebuggees()) {
  try {
    dbg.addDebuggee(global);

https://searchfox.org/mozilla-central/rev/6eb9b2d8f23c03ff6e203502a476cbdb0358a807/devtools/server/actors/utils/make-debugger.js#98-100

dbg.findDebuggees = function () {
  return findDebuggees(dbg);
};

https://searchfox.org/mozilla-central/rev/6eb9b2d8f23c03ff6e203502a476cbdb0358a807/devtools/server/actors/targets/parent-process.js#61-62

findDebuggees: dbg =>
  dbg.findAllGlobals().map(g => g.unsafeDereference()),

https://searchfox.org/mozilla-central/rev/6eb9b2d8f23c03ff6e203502a476cbdb0358a807/js/src/debugger/Debugger.cpp#6141-6142,6144,6147,6149,6164,6169-6172,6174,6181-6183,6186,6191

bool Debugger::CallData::findAllGlobals() {
  RootedObjectVector globals(cx);
...
  {
...
    JS::AutoCheckCannotGC nogc;
...
    for (RealmsIter r(cx->runtime()); !r.done(); r.next()) {
...
      GlobalObject* global = r->maybeGlobal();
...
      JS::ExposeObjectToActiveJS(global);
      if (!globals.append(global)) {
        return false;
      }
...
  }
...
  for (size_t i = 0; i < globals.length(); i++) {
    RootedValue globalValue(cx, ObjectValue(*globals[i]));
    if (!dbg->wrapDebuggeeValue(cx, &globalValue)) {
...
    if (!NewbornArrayPush(cx, result, globalValue)) {
...
  args.rval().setObject(*result);

https://hg.mozilla.org/mozilla-central/rev/2dc2a9103252