Closed Bug 1398198 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Startup and Profile System, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Iteration:
57.3 - Sep 19
Tracking Status
firefox57 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

(Whiteboard: [reserve-photon-performance])

Attachments

(2 files, 1 obsolete file)

This would be very helpful to debug intermittent browser_startup.js failures.
Attached patch Patch (obsolete) — Splinter Review
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)
Blocks: 1358798
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+
Attached patch Patch v2Splinter Review
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)
Attachment #8906002 - Attachment is obsolete: true
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+
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+
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.
https://hg.mozilla.org/mozilla-central/rev/7030bd97757e
https://hg.mozilla.org/mozilla-central/rev/7bf13864b0ab
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
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.