1380011 - Make startupRecorder.js' 'before handling user event' phase more reliable

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P1)

Description

[Florian Quèze [:florian]]

Several times I've wanted to block some scripts from startup and tried to blacklist them at the 'before handling user events' phase of startup, but try disagreed and I only blacklisted before first paint.

The most recent occurrence is bug 1379788, where RecentWindow.jsm isn't loaded during startup on my local development machine, but on try it's loaded before the main thread becomes idle, because there's a SessionSaver timer that fires.

The current solution of calling requestIdleCallback once from startupRecorder to determine when we are fully done with startup is also not going to race with all the startup stuff we are delaying using idle callbacks.

Proposed solution:
- record the "before handling user events" phase after just a dispatchToMainThread (instead of the current idleDispatchToMainThread). That seems to make more sense, as it's indeed the first point where a user event will be dispatched.
This lets us reliably blacklist from this phase anything that we expect to be loaded at the end of startup using idle callbacks.

- record another "before becoming idle" phase, that's recorded after several (eg 10) nested idle callback fired. This lets us blacklist everything that is now supposed to be lazy.

Waiting for several periods of idle time before capturing the final startup state may also help with browser_startup_image.js intermittents.

Comment 1

[Florian Quèze [:florian]]

Attachment: WIP

Only requesting feedback for now because I need to do try runs to see what part of the current "before first paint" blacklist can now be moved to "before handling user events". This is otherwise ready for review (if it looks good and you feel comfortable, you can even r+, knowing I'll adjust the blacklists before landing).

Comment 2

[Florian Quèze [:florian]]

Attachment: Patch v2

Comment 3

[Florian Quèze [:florian]]

Attachment: adjust the browser_startup_image.js whitelist

I think this solves most browser_startup_image.js intermittents.

Comment 4

[Florian Quèze [:florian]]

Attachment: When e10s is disabled, treat the 'before handling user events' startup phase as 'before first paint' to avoid intermittent failures

My last try push without this hack had lots of oranges: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eacecdc5666af171644001b22be74d8685468b2&selectedJob=113650793

There are least 2 intermittent issues that occur only when e10s is disabled:
- RecentWindow.jsm gets loaded as a consequence of the SessionSaver timer firing before we are fully done with startup. I think it's triggered by a content page loading in the <browser>, can causing session store to want to save it later (hence the timer). This session store timer sometimes firing before we are done restoring windows seems nasty, but it never seems to happen with e10s, likely due to the delay in receiving the messages from the content processes (or the delays in starting the content processes).
- BookmarkHTMLUtils.jsm gets intermittently loaded during startup as a result of the places database being initialized (likely by a content page getting loaded). And sometimes (depending on IO timing in nsBrowserGlue.js' _initPlaces method), this even has time to load a whole lot of places stuff that's blacklisted in the test. Again, this doesn't happen with e10s.

Given that non-e10s won't be supported much longer, I don't think it's worth investing time in fixing these issues now, but I was reluctant to disable the whole test for non-e10s. I think this hack is a reasonable compromise.

Comment 5

[Florian Quèze [:florian]]

try push with the 3 patches applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f0b4811ca5f71087a02151cdf06f49d5ee3ef4e

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8885855 [details] [diff] [review]
Patch v2

Review of attachment 8885855 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Happy to see those blacklist entries move forward too!

::: browser/components/tests/startupRecorder.js
@@ +89,3 @@
>          this.record.bind(this, "before handling user events"));
>  
> +      (function waitForIdle(callback, count = 10) {

Can you please add some commentary here about the magic number of 10?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8885859 [details] [diff] [review]
When e10s is disabled, treat the 'before handling user events' startup phase as 'before first paint' to avoid intermittent failures

Review of attachment 8885859 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds good to me. I agree that investigating this is not really worth it for the non-e10s case.

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8885856 [details] [diff] [review]
adjust the browser_startup_image.js whitelist

Review of attachment 8885856 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/914e475abbf4
Make startupRecorder.js' 'before handling user event' phase more reliable, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3157e928734
adjust the browser_startup_image.js whitelist, r=johannh.
https://hg.mozilla.org/integration/mozilla-inbound/rev/72ce56d64a62
When e10s is disabled, treat the 'before handling user events' startup phase as 'before first paint' to avoid intermittent failures, r=mconley.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/914e475abbf4
https://hg.mozilla.org/mozilla-central/rev/d3157e928734
https://hg.mozilla.org/mozilla-central/rev/72ce56d64a62