browser_startup.js should show the stack when a JS file was loaded earlier than expected

RESOLVED FIXED in Firefox 57

Status

()

Toolkit
Startup and Profile System
P1
normal
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

unspecified
mozilla57
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [reserve-photon-performance])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 months ago
This would be very helpful to debug intermittent browser_startup.js failures.
(Assignee)

Comment 1

4 months ago
Created attachment 8906002 [details] [diff] [review]
Patch

The patch makes the module loader record JS stacks when importing a module or loading a component, so that our startup test can output the stacks.

This code is ifdef'ed to only exist for nightly and debug builds, to avoid using more memory for release builds, and the actual recording is controlled by a pref to avoid slowing down startup unless we are running our startup test. The pref is set only for our performance test folder. I verified that it's set early during startup, and so I can use it to make startupRecorder.js return early to remove the startup penalty it had for Nightly users.

This will likely need a separate review for the js/xpconnect part, but let's see if you like it first :-).
Attachment #8906002 - Flags: review?(felipc)
(Assignee)

Updated

4 months ago
Blocks: 1358798
(Assignee)

Comment 2

4 months ago
Comment on attachment 8906002 [details] [diff] [review]
Patch

Adding another request for the js/xpconnect/ part.
Attachment #8906002 - Flags: review?(continuation)
Attachment #8906002 - Flags: review?(felipc) → review+
Comment on attachment 8906002 [details] [diff] [review]
Patch

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

Nice.

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +898,5 @@
> +    nsresult rv = info.EnsureKey();
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    ModuleEntry* mod;
> +    mImports.Get(info.Key(), &mod);

This line looks like a duplicate of the next line.
Attachment #8906002 - Flags: review?(continuation) → review+
(Assignee)

Comment 4

4 months ago
Created attachment 8906648 [details] [diff] [review]
Patch v2

Turns out try wasn't so happy about the previous version of the patch. Changes made here:
- clear the importStack string explicitly in the Clear method, otherwise the string is leaked on shutdown (making try unhappy on debug builds). This is needed because the ModuleEntry instances in mModules are 'intentionally leaked' (according to http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/js/xpconnect/loader/mozJSComponentLoader.h#186)
- when returning early in startupRecorder.js, call this._resolve(), so that marionette can start (http://searchfox.org/mozilla-central/rev/d29c832536839b60a9ee5b512eeb934274b879b0/testing/marionette/components/marionette.js#298 waits for this promise).
Attachment #8906648 - Flags: review?(felipc)
(Assignee)

Updated

4 months ago
Attachment #8906002 - Attachment is obsolete: true
(Assignee)

Comment 5

4 months ago
Created attachment 8906662 [details] [diff] [review]
Patch for browser_startup_content.js

With only the previous patch, content processes fail to start because the "browser.startup.record" pref gets read the first time a JS component is loaded in the content process, and the pref isn't initialized yet. I could have just disabled the stack collection for content processes, but I figured it was better to also print stacks in browser_startup_content.js.

r? felipe for the test change, and billm for the addition to ContentPrefs::gInitPrefs.

Try now looks happy: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5da5c385432451933124fb18a2536ec4d95abadb&filter-tier=1
Attachment #8906662 - Flags: review?(wmccloskey)
Attachment #8906662 - Flags: review?(felipc)
Attachment #8906648 - Flags: review?(felipc) → review+
Attachment #8906662 - Flags: review?(felipc) → review+

Updated

4 months ago
Status: NEW → ASSIGNED
Iteration: --- → 57.3 - Sep 19
Flags: qe-verify-
Priority: -- → P1
Whiteboard: [reserve-photon-performance]
Comment on attachment 8906662 [details] [diff] [review]
Patch for browser_startup_content.js

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

Providing DOM peer r+ for ContentPrefs.
Attachment #8906662 - Flags: review+

Comment 7

4 months ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7030bd97757e
browser_startup.js should show the stack when a JS file was loaded earlier than expected, r=felipe,mccr8.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bf13864b0ab
browser_startup_content.js should show the stack when a JS file was loaded earlier than expected, r=felipe,qdot.

Comment 8

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7030bd97757e
https://hg.mozilla.org/mozilla-central/rev/7bf13864b0ab
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8906662 [details] [diff] [review]
Patch for browser_startup_content.js

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

Sorry, was on PTO.
Attachment #8906662 - Flags: review?(wmccloskey)
You need to log in before you can comment on or make changes to this bug.