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

RESOLVED FIXED in Firefox 56

Status

()

Firefox
General
P1
normal
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-performance])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

a month ago
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.
(Assignee)

Comment 1

a month ago
Created attachment 8885304 [details] [diff] [review]
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).
Attachment #8885304 - Flags: feedback?(mconley)
(Assignee)

Comment 2

a month ago
Created attachment 8885855 [details] [diff] [review]
Patch v2
Attachment #8885855 - Flags: review?(mconley)
(Assignee)

Updated

a month ago
Attachment #8885304 - Attachment is obsolete: true
Attachment #8885304 - Flags: feedback?(mconley)
(Assignee)

Comment 3

a month ago
Created attachment 8885856 [details] [diff] [review]
adjust the browser_startup_image.js whitelist

I think this solves most browser_startup_image.js intermittents.
Attachment #8885856 - Flags: review?(jhofmann)
(Assignee)

Comment 4

a month ago
Created 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

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.
Attachment #8885859 - Flags: review?(mconley)
(Assignee)

Comment 5

a month ago
try push with the 3 patches applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f0b4811ca5f71087a02151cdf06f49d5ee3ef4e
(Assignee)

Updated

a month ago
Blocks: 1355956
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+

Updated

a month ago
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Flags: qe-verify-
Priority: -- → P1

Comment 9

a month ago
Pushed by florian@queze.net:
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

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/914e475abbf4
https://hg.mozilla.org/mozilla-central/rev/d3157e928734
https://hg.mozilla.org/mozilla-central/rev/72ce56d64a62
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Blocks: 1379359
Blocks: 1376259
Blocks: 1379068
Blocks: 1379360
You need to log in before you can comment on or make changes to this bug.