Add a test to prevent introducing new sync IPC during startup

RESOLVED FIXED in Firefox 69

Status

()

task
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Regressed 3 bugs)

unspecified
Firefox 69
Points:
---

Firefox Tracking Flags

(firefox69 fixed)

Details

Attachments

(1 attachment)

Similar to what I did in bug 1540135 for main thread I/O.

When running this on try, things seem pretty stable (ie. when running the same job several times, I got the exact same result).

I was surprised by how much sync IPC we do on Windows compared to other platforms. I wonder if this is well known or would deserve more investigation.

Two things that worry me:

  • on Windows, between the Windows 7 (32bit) and the Windows 10 (64bit) output, there were LOTS of differences, which forced me to put ignoreIfUnused on almost all the rules applying only to Windows. I suspect the actual reason for these differences isn't really Win7 vs Win10 or 32 vs 64 bits, but probably more related to which graphics code path we are picking. I think with a good understanding of that code, it would be possible to put accurate "condition"s on most of these rules instead of ignoreIfUnused... but I don't have the knowledge for this myself without doing a lot of research in the code.
  • Given that I know very little about the code doing these sync IPC calls, it would be hard for me to explain to people getting backed out due to this test how to write their code differently. I wonder if we need to seek support of someone who understands IPC better before landing.
Pushed by fqueze@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/012f377d8d82
Add a mochitest to avoid more sync IPC being introduced during startup, r=mconley.
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69

While I like the goal of this test, I think the implementation should have been a little more accepting - rather than allowing a particular sync IPC message "exactly" during a particular phase, it should have allowed it during the phase OR any later phase. That way if something is not totally deterministic and runs a little later than the phase it was annotated as being in (which is technically better for the user) it doesn't cause a failure.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)

if something is not totally deterministic and runs a little later than the phase it was annotated as being in (which is technically better for the user)

This seems like it highlights real bugs. If we have something async that triggers blocking calls (ie. sync IPC), but that thing was fine to run during the next phase, then it should always run during the next phase. But yes, I know startup order is currently quite non deterministic with async stuff mixed into the critical path toward showing a responsive UI to the user, which is scary.

(In reply to Florian Quèze [:florian] from comment #7)

If we have something async that triggers blocking calls (ie. sync IPC), but that thing was fine to run during the next phase, then it should always run during the next phase.

Yeah that's a fair point.

You need to log in before you can comment on or make changes to this bug.