1778696 - WatchdogMain seems to have a bogus manager->ForAllActiveContexts loop condition

Status: NEW

(Core :: XPConnect, defect)

Description

[Jens Stutte [:jstutte]]

<snip from a discussion in D150598>
(jstutte) I am puzzled by this false together with the previous true. IIUC ForAllActiveContexts breaks its loop on false, so if the first context in the list was not running for too long, we won't check the other contextes? I'd expect if ever to be it the other way round, such that we ask for one interrupt at a time but check all contextes.

(nika) It looks like this dates back to when ForAllActiveContexts was introduced alongside this use of it in https://bugzilla.mozilla.org/show_bug.cgi?id=1376507 (https://hg.mozilla.org/mozilla-central/rev/7e5fec0ca1ca960d24daece0c62aa5634f540222). I'm guessing it might've been a mistake which was missed during review, and hasn't come up since as it hasn't been relevant. That being said, I don't think we'll ever have more than one context. The code was added in 2017 as part of the Quantum DOM scheduling work, which proposed having multiple main threads, which took turns running cooperatively. This would've meant that we could have multiple active contexts, but was never implemented.

(asuth) It does seem like it's a logic error that was meant to be an optimization, yeah. I see no discussion of this logic when it was added in https://bugzilla.mozilla.org/show_bug.cgi?id=1376507 and it seems like it hasn't been modified since. That said, I think there's probably only 1 of these because I assume the context was for the Quantum DOM stuff where we'd basically have multiple main threads that multiplex themselves.
</snip>

While it seems harmless we might want to prevent future readers from headaches here.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:kmag, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Michael Holloway]

I'm interested in taking this on. Is the fix essentially just to revert this change?

Comment 3

[Jens Stutte [:jstutte]]

(In reply to Michael Holloway from comment #2)

I'm interested in taking this on. Is the fix essentially just to revert this change?

Hi! Sorry for the late response, EOY holidays... It sounds quite right from Nika's part of comment 0 that we do not need the list at all these days, so (carefully) reverting that patch seems to be a good start.

Comment 4

[Michael Holloway]

Attachment: Bug 1778696 - Remove support for multiple XPCJSContexts r?jstutte

This is a manual revert of 7e5fec0ca1ca960d24daece0c62aa5634f540222,
which added support for multiple XPCJSContexts to WatchdogManager.
This was added to support proposed Quantum DOM scheduling work
involving multiple main threads, a proposal which was never
implemented.

I added or left in place a few small changes from the previous code:

• The AutoLockWatchdog class is still declared as final.

• I left in place a couple of null checks that weren't there
  previously.

• I added brackets to all if statements for maintainability and
  consistency with the current code style.

Comment 5

[Michael Holloway]

Looks like that's got some build errors. Will amend.

Comment 6

[Michael Holloway]

Thank you for the review, Jens. I'll update the change per your comments.

One question: I'd like to be able to use the static analyzer locally after making a change rather than relying on CI to validate my changes, but running ./mach static-analysis check --outgoing as the build reports suggest does not produce any meaningful output. (Here is an example of the full output I see locally.) Do I need some additional configuration or command line arguments here?

Comment 7

[Jens Stutte [:jstutte]]

(In reply to Michael Holloway from comment #6)

Thank you for the review, Jens. I'll update the change per your comments.

One question: I'd like to be able to use the static analyzer locally after making a change rather than relying on CI to validate my changes, but running ./mach static-analysis check --outgoing as the build reports suggest does not produce any meaningful output. (Here is an example of the full output I see locally.) Do I need some additional configuration or command line arguments here?

Sorry, I do not really know the answer, I usually rely on the hints in phabricator. From there it seems you solved the issue. But you might ask on the #build channel on https://chat.mozilla.org.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.