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)
Toolkit
Startup and Profile System
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: florian, Assigned: florian)
References
Details
(Whiteboard: [reserve-photon-performance])
Attachments
(2 files, 1 obsolete file)
13.08 KB,
patch
|
Felipe
:
review+
|
Details | Diff | Splinter Review |
4.72 KB,
patch
|
Felipe
:
review+
qdot
:
review+
|
Details | Diff | Splinter Review |
This would be very helpful to debug intermittent browser_startup.js failures.
Assignee | ||
Comment 1•7 years ago
|
||
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 | ||
Comment 2•7 years ago
|
||
Comment on attachment 8906002 [details] [diff] [review] Patch Adding another request for the js/xpconnect/ part.
Attachment #8906002 -
Flags: review?(continuation)
Updated•7 years ago
|
Attachment #8906002 -
Flags: review?(felipc) → review+
Comment 3•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
Attachment #8906002 -
Attachment is obsolete: true
Assignee | ||
Comment 5•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8906648 -
Flags: review?(felipc) → review+
Updated•7 years ago
|
Attachment #8906662 -
Flags: review?(felipc) → review+
Updated•7 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 57.3 - Sep 19
Flags: qe-verify-
Priority: -- → P1
Whiteboard: [reserve-photon-performance]
Comment 6•7 years ago
|
||
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.
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7030bd97757e https://hg.mozilla.org/mozilla-central/rev/7bf13864b0ab
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.
Description
•