1552524 - Add a test to prevent introducing new sync IPC during startup

Status: RESOLVED FIXED

(Firefox :: General, task)

Description

[Florian Quèze [:florian]]

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

Comment 1

[Florian Quèze [:florian]]

Attachment: Bug 1552524 - Add a mochitest to avoid more sync IPC being introduced during startup, r=mconley.

Comment 2

[Florian Quèze [:florian]]

Green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c35fee102713ca906162d4447fc4624144fd4d19

Comment 3

[Florian Quèze [:florian]]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/012f377d8d82
Add a mochitest to avoid more sync IPC being introduced during startup, r=mconley.

Comment 5

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/012f377d8d82

Comment 6

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 7

[Florian Quèze [:florian]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(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.