Closed Bug 1380011 Opened 7 years ago Closed 7 years ago

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


(Firefox :: General, enhancement, P1)




Firefox 56
56.3 - Jul 24
Tracking Status
firefox56 --- fixed


(Reporter: florian, Assigned: florian)


(Blocks 1 open bug)


(Whiteboard: [photon-performance])


(3 files, 1 obsolete file)

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.
Attached patch WIP (obsolete) — Splinter Review
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).
Attachment #8885304 - Flags: feedback?(mconley)
Attached patch Patch v2Splinter Review
Attachment #8885855 - Flags: review?(mconley)
Attachment #8885304 - Attachment is obsolete: true
Attachment #8885304 - Flags: feedback?(mconley)
I think this solves most browser_startup_image.js intermittents.
Attachment #8885856 - Flags: review?(jhofmann)
My last try push without this hack had lots of oranges:

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.
Attachment #8885859 - Flags: review?(mconley)
Whiteboard: [photon-performance]
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?
Attachment #8885855 - Flags: review?(mconley) → review+
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.
Attachment #8885859 - Flags: review?(mconley) → review+
Comment on attachment 8885856 [details] [diff] [review]
adjust the browser_startup_image.js whitelist

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

Thank you!
Attachment #8885856 - Flags: review?(jhofmann) → review+
Iteration: --- → 56.3 - Jul 24
Flags: qe-verify-
Priority: -- → P1
Pushed by
Make startupRecorder.js' 'before handling user event' phase more reliable, r=mconley.
adjust the browser_startup_image.js whitelist, r=johannh.
When e10s is disabled, treat the 'before handling user events' startup phase as 'before first paint' to avoid intermittent failures, r=mconley.
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.