Test which JS modules and components are loaded during startup

RESOLVED FIXED in Firefox 55

Status

()

Firefox
General
P1
normal
RESOLVED FIXED
2 months ago
27 days ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-performance])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 months ago
We should review what xpcom components are registered to observe the app-startup notification, and delay loading those that don't need to run that early in startup.

A quick look shows at least these 3 components that only use app-startup to add another observer for something that happens at a later stage of startup:

http://searchfox.org/mozilla-central/rev/7aa21f3b531ddee90a353215bd86e97d6974e25b/services/sync/Weave.js#110 observes final-ui-startup

http://searchfox.org/mozilla-central/rev/6e1c138a06a80f2bb3167d9dac050cc0fd4fb39e/browser/components/feeds/WebContentConverter.js#843 observers browser-ui-startup-complete

http://searchfox.org/mozilla-central/rev/6e1c138a06a80f2bb3167d9dac050cc0fd4fb39e/dom/push/PushComponents.js#79 observes sessionstore-windows-restored

It seems that all of this could be more efficiently called directly from nsBrowserGlue.js

Updated

2 months ago
Flags: qe-verify?
Priority: -- → P2
(Assignee)

Updated

2 months ago
See Also: → bug 1358921
(Assignee)

Updated

2 months ago
Blocks: 1355956
(Assignee)

Updated

2 months ago
No longer blocks: 1348289

Updated

2 months ago
Depends on: 1364571
(Assignee)

Updated

a month ago
Assignee: nobody → florian

Updated

a month ago
Status: NEW → ASSIGNED
Iteration: --- → 55.6 - May 29
Priority: P2 → P1

Updated

a month ago
Flags: qe-verify? → qe-verify-

Updated

a month ago
Iteration: 55.6 - May 29 → 55.7 - Jun 12
(Assignee)

Comment 1

29 days ago
Created attachment 8872709 [details] [diff] [review]
add a test preventing us from loading scripts unintentionally during startup

Requesting review from mccr8 for the js/xpconnect changes, and from mconley for the browser/ changes.

I think I'll file individual bugs for each of the components that need to be initialized later, as I don't want to delay landing the test to prevent regressions.
Attachment #8872709 - Flags: review?(mconley)
Attachment #8872709 - Flags: review?(continuation)
(Assignee)

Comment 2

29 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ca9dac98ebd46d0000a09969a41f5252ff316ff is green except for an intentional failure I added in the test to easily locate the logs with the list of files.
Bobby, is it okay if I review the XPConnect changes here? It is just a couple of methods for added for testing, in the loader.
Flags: needinfo?(bobbyholley)
Sure thing! I just made you an XPConnect peer so that you don't have to ask anymore. Please obviously only review stuff you have a good understanding of, and probably avoid reviewing stuff in wrappers/. :-)
Flags: needinfo?(bobbyholley)
Comment on attachment 8872709 [details] [diff] [review]
add a test preventing us from loading scripts unintentionally during startup

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

r=me for the XPConnect changes, with the below issues fixed.

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +923,5 @@
>      *retval = !!mImports.Get(info.Key());
>      return NS_OK;
>  }
>  
> +NS_IMETHODIMP mozJSComponentLoader::LoadedModules(uint32_t *length,

For all of the arguments to these two methods, the * should be to the left, like:
char*** aModules.

@@ +927,5 @@
> +NS_IMETHODIMP mozJSComponentLoader::LoadedModules(uint32_t *length,
> +                                                  char * **aModules)
> +{
> +    char** modules =
> +        static_cast<char**>(moz_xmalloc(sizeof(char*) * mImports.Count()));

This is very C-ish. Here and below, this should be more like:
  char** modules = new char*[mImports.Count()];
Attachment #8872709 - Flags: review?(continuation) → review+
Comment on attachment 8872709 [details] [diff] [review]
add a test preventing us from loading scripts unintentionally during startup

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

This is great, thanks florian. I'm glad we're finally putting a stick in the ground here.

Just a few suggestions below. Let me know if you have any questions.

::: browser/base/content/test/performance/browser_startup.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +

We should add something here at the top describing what this test is doing, and what should be done if somebody makes a change that causes this to fail.

We could perhaps make the whitelist note on line 7-9 a little more dissuading, and suggest other places where things could init themselves (perhaps after first delayed startup, etc).

@@ +11,5 @@
> +    components: new Set([
> +      "nsBrowserGlue.js",
> +      "MainProcessSingleton.js",
> +
> +      // The following components shouldn't be initialized that early.

This comment seems to contradict the comment on 7-9. If we're planning on removing these down the road, perhaps we should mention that here.

@@ +21,5 @@
> +      "resource://gre/modules/AppConstants.jsm",
> +      "resource://gre/modules/XPCOMUtils.jsm",
> +      "resource://gre/modules/Services.jsm",
> +
> +      // Probably loaded too early, needs investigation.

Same as above - we likely plan on removing these.

@@ +36,5 @@
> +
> +  // We are at this phase after creating the first browser window.
> +  "after-final-ui-startup": {blacklist: {
> +    components: new Set([
> +      "nsSearchService.js"

This blacklist is likely to grow, I'd imagine. Perhaps let's put a trailing comma here to avoid updating the blame on each line when adding new entries.

@@ +41,5 @@
> +    ])
> +  }},
> +
> +  // We are at this phase after showing the first browser window.
> +  // Anything loaded at this phase or before delays first paint.

"Anything loaded at this phase or before delays first paint."

Wait... things firing _on_ after-first-paint delay first paint? Are we sure?

@@ +69,5 @@
> +
> +  SimpleTest.requestCompleteLog();
> +  let previous;
> +  for (let phase in data) {
> +    for (let scriptType in data[phase])

I'm not a huge fan of unbraced loops like this... seems like a foot-gun. But I'll leave it to your discretion.

@@ +71,5 @@
> +  let previous;
> +  for (let phase in data) {
> +    for (let scriptType in data[phase])
> +      for (let f of data[phase][scriptType])
> +        if (!previous || !data[previous][scriptType].includes(f))

Hm. From my understanding of this code, we're checking to see if the script was loaded one phase back. Since we're going across a number of phases during this test, should we be scanning back the full list instead of only one phase back?

Or am I misunderstanding what this set of loops does?

@@ +79,5 @@
> +
> +  for (let phase in startupPhases) {
> +    let loadedList = data[phase];
> +    if (startupPhases[phase].whitelist) {
> +      let whitelist = startupPhases[phase].whitelist;

Nit, perhaps we can simplify via:

let whitelist = startupPhases[phase].whitelist;
if (whitelist) {
  // ...
}

@@ +89,5 @@
> +          return false;
> +        });
> +        is(loadedList[scriptType].length, 0,
> +           `should have no unexpected ${scriptType} loaded during app-startup`);
> +        for (let script of loadedList[scriptType])

Same as above, re: unbraced loops.

::: browser/components/tests/startupRecorder.js
@@ +6,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +function startupRecorder() {

Can you please add a docstring above this function to describe the responsibilities of this component? Maybe also worth calling out the special idle-dispatch-after-session-restored item it adds to its recordings, and the mappings it does from topics to phase names.

::: js/xpconnect/idl/xpcIJSModuleLoader.idl
@@ +79,5 @@
>     *          otherwise
>     */
>    boolean isModuleLoaded(in AUTF8String aResourceURI);
> +
> +  // These 2 functions are for startup testing purpose.

s/purpose/purposes

Perhaps we should also mention that they're not expected to be used for production code.
Attachment #8872709 - Flags: review?(mconley) → review+
(Assignee)

Comment 7

29 days ago
(In reply to Mike Conley (:mconley) from comment #6)

> This is great, thanks florian. I'm glad we're finally putting a stick in the
> ground here.

Thanks!

> @@ +11,5 @@
> > +    components: new Set([
> > +      "nsBrowserGlue.js",
> > +      "MainProcessSingleton.js",
> > +
> > +      // The following components shouldn't be initialized that early.
> 
> This comment seems to contradict the comment on 7-9. If we're planning on
> removing these down the road, perhaps we should mention that here.

Yes, these are bugs. Ideally I would like to have a bug number in a comment on each line, but I'm not sure it's a good idea to block landing on having all these bugs filed.

> > +  // We are at this phase after showing the first browser window.
> > +  // Anything loaded at this phase or before delays first paint.
> 
> "Anything loaded at this phase or before delays first paint."
> 
> Wait... things firing _on_ after-first-paint delay first paint? Are we sure?

Yes, the recorder here is meant to be the first piece of JS that runs after first paint, so any script that's already loaded at this point has been loaded before first paint, and so delayed first paint. Suggestions welcome to make the name clearer.


> @@ +71,5 @@
> > +  let previous;
> > +  for (let phase in data) {
> > +    for (let scriptType in data[phase])
> > +      for (let f of data[phase][scriptType])
> > +        if (!previous || !data[previous][scriptType].includes(f))
> 
> Hm. From my understanding of this code, we're checking to see if the script
> was loaded one phase back. Since we're going across a number of phases
> during this test, should we be scanning back the full list instead of only
> one phase back?

The phases are ordered in the list, if a script wasn't loaded yet one phase back, it wasn't loaded in any of the previous phases.

> Or am I misunderstanding what this set of loops does?

This whole code block isn't actually 'useful' for the test, it just outputs logging that's helpful to decide what are the next bugs to file.


> Can you please add a docstring above this function to describe the
> responsibilities of this component? Maybe also worth calling out the special
> idle-dispatch-after-session-restored item it adds to its recordings, and the
> mappings it does from topics to phase names.

I'm afraid if we need to document it, it's because the mapping is unclear. I added it because the notifications I'm actually observing to record are (purposefully) mostly unknown, and I wanted something more familiar in the test code.

Suggestions welcome for better names to use in the mapping.
(Assignee)

Comment 8

28 days ago
Created attachment 8873098 [details] [diff] [review]
Patch v2

Addressed review comments.
(Assignee)

Updated

28 days ago
Attachment #8872709 - Attachment is obsolete: true
(Assignee)

Comment 9

28 days ago
Created attachment 8873147 [details] [diff] [review]
Patch v3

Fixed eslint error, and made the phase names human readable.
(Assignee)

Updated

28 days ago
Attachment #8873098 - Attachment is obsolete: true

Comment 10

28 days ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef0ddbc20a37
add a test preventing us from loading scripts unintentionally during startup, r=mconley,mccr8.
(Assignee)

Comment 11

28 days ago
Rephrasing the summary to match the patch that landed here. I'll file lots of follow-ups for individual scripts that are being loaded too early.
Summary: Move components away from app-startup → Test which JS modules and components are loaded during startup

Comment 12

28 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ef0ddbc20a37
Status: ASSIGNED → RESOLVED
Last Resolved: 28 days ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Updated

27 days ago
Depends on: 1369467
You need to log in before you can comment on or make changes to this bug.