1358798 - Test which JS modules and components are loaded during startup

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P1)

Description

[Florian Quèze [:florian]]

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

Comment 1

[Florian Quèze [:florian]]

Attachment: 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.

Comment 2

[Florian Quèze [:florian]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 4

[Bobby Holley (:bholley)]

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/. :-)

Comment 5

[Andrew McCreight [:mccr8]]

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()];

Comment 6

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 7

[Florian Quèze [:florian]]

(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.

Comment 8

[Florian Quèze [:florian]]

Attachment: Patch v2

Addressed review comments.

Comment 9

[Florian Quèze [:florian]]

Attachment: Patch v3

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef0ddbc20a37
add a test preventing us from loading scripts unintentionally during startup, r=mconley,mccr8.

Comment 11

[Florian Quèze [:florian]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef0ddbc20a37