1479241 - Don't eagerly load AboutPages.jsm in content processes

Status: RESOLVED FIXED

(Firefox :: Normandy Client, enhancement)

Description

[Kris Maglione [:kmag]]

AboutStudies.jsm is only needed in a content process if and when that process tries to load about:studies. Loading it before then is unnecessary and wasteful.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1479241: Don't eagerly load AboutPages.jsm in content processes.

Review commit: https://reviewboard.mozilla.org/r/260132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260132/

Comment 2

[Florian Quèze [:florian]]

There's a whitelist entry to remove at https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/browser/base/content/test/performance/browser_startup_content.js#83

Comment 3

[Michael Cooper [:mythmon]]

Comment on attachment 8995764 [details]
Bug 1479241: Don't eagerly load AboutPages.jsm in content processes.

https://reviewboard.mozilla.org/r/260132/#review267354

Thanks for this! I've been wanting to update this code since Normandy stopped being a system add-on. I *think* this work (possibly along with your other patch) covers bug 1440056. Does that sound right to you?

Also from that bug, I noticed bug 1440056 which says the tests for these pages are disabled. I don't think there is anything you need to do here, but worth being aware of.

::: browser/installer/package-manifest.in:372
(Diff revision 1)
>  ; [Extensions]
>  @RESPATH@/components/extensions-toolkit.manifest
>  @RESPATH@/components/extension-process-script.js
>  @RESPATH@/browser/components/extensions-browser.manifest
>  
> +; [Shield]

Can this be "Normandy" instead of "Shield"?

::: mobile/android/installer/package-manifest.in:268
(Diff revision 1)
>  ; [Extensions]
>  @BINPATH@/components/extensions-toolkit.manifest
>  @BINPATH@/components/extensions-mobile.manifest
>  @BINPATH@/components/extension-process-script.js
>  
> +; [Shield]

I don't think these are needed here. Normandy isn't used at all on Android.

::: toolkit/components/normandy/content/shield-content-process.js:20
(Diff revision 1)
> -class ShieldChildListener {
> -  onStartup() {
> +function AboutStudies() {
> +  return AboutStudies.prototype;
> -    Services.cpmm.addMessageListener("Shield:ShuttingDown", this, true);
> -    AboutPages.aboutStudies.register();
> -  }
> +}
> +AboutStudies.prototype = AboutPages.aboutStudies;

These seems like a weird pattern. Is there a reason AboutStudies needs to be a class like object? Could it be a simple function instead of having a prototype? Also I don't see where NSGetFactory is used. Overall I think a comment explaining what is going on here would be useful.

::: toolkit/components/normandy/moz.build:10
(Diff revision 1)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  with Files('**'):
>      BUG_COMPONENT = ('Firefox', 'Normandy Client')
>  
>  JAR_MANIFESTS += ['jar.mn']

jar.mn previously dealt with the content directory. Is that still needed, or do your build system changes obsolete that?

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8995764 [details]
Bug 1479241: Don't eagerly load AboutPages.jsm in content processes.

https://reviewboard.mozilla.org/r/260132/#review267354

This might cover bug 1440056, depending on whether you still want to use AboutRedirector. But I think you're still going to need most of the same code to handle the events/messages from the about pages, anyway, so it might make more sense to just keep everything on one place.

I didn't realize the tests for the pages were disabled, but I tested manually anyway, and everything still seems to work as expected.

> I don't think these are needed here. Normandy isn't used at all on Android.

Ah, interesting.

> These seems like a weird pattern. Is there a reason AboutStudies needs to be a class like object? Could it be a simple function instead of having a prototype? Also I don't see where NSGetFactory is used. Overall I think a comment explaining what is going on here would be useful.

Unfortunately, generateNSGetFactory only supports constructor functions, and looks up properties like classID directly on their prototypes, so we need a wrapper.

Ideally, I'd have AboutStudies just subclass AboutPage rather than create an instance of it, but that's probably better left to another bug.

NSGetFactory is used by the XPConnect component loading infrastructure. It's a pretty common pattern, so we don't usually comment on it, but I can if you prefer.

> jar.mn previously dealt with the content directory. Is that still needed, or do your build system changes obsolete that?

Our component loading stuff is kind of weird, so component scripts and their manifests need to be packaged specially (for now).

The jar.mn is still needed for the rest of the files in this directory, though.

Comment 5

[Michael Cooper [:mythmon]]

Comment on attachment 8995764 [details]
Bug 1479241: Don't eagerly load AboutPages.jsm in content processes.

https://reviewboard.mozilla.org/r/260132/#review267354

Ok. Thanks for summarizing the differences. It almost sounds like with these patches, there isn't much of a reason to persue 1440056 further?

> Unfortunately, generateNSGetFactory only supports constructor functions, and looks up properties like classID directly on their prototypes, so we need a wrapper.
> 
> Ideally, I'd have AboutStudies just subclass AboutPage rather than create an instance of it, but that's probably better left to another bug.
> 
> NSGetFactory is used by the XPConnect component loading infrastructure. It's a pretty common pattern, so we don't usually comment on it, but I can if you prefer.

I think it makes sense to comment about `generateNSGetFactory`'s restrictions, but since `NSGetFactory` is common, no need to comment about that.

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8995764 [details]
Bug 1479241: Don't eagerly load AboutPages.jsm in content processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260132/diff/1-2/

Comment 7

[Michael Cooper [:mythmon]]

Comment on attachment 8995764 [details]
Bug 1479241: Don't eagerly load AboutPages.jsm in content processes.

https://reviewboard.mozilla.org/r/260132/#review267916

Looks good, thanks for the changes.

::: toolkit/components/normandy/content/shield-content-process.js:18
(Diff revisions 1 - 2)
>  
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  ChromeUtils.import("resource://normandy-content/AboutPages.jsm");
>  
> +// generateNSGetFactory only knows how to register factory classes, with
> +// classID properties on their prototype, and a cnstructor that returns

Typo: "cnstructor"

Comment 8

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f150e62dcbbdc14bec93db0705470a5d9e71737e
Bug 1479241: Don't eagerly load AboutPages.jsm in content processes. r=mythmon

Comment 9

[Narcis Beleuzu [:NarcisB]]

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

Comment 10

[Cosmin Sabou [:CosminS]]

Backed out for causing a big performance regression on OS X and as request by igoldan. 

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

Comment 11

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74910e67768c6e829d946bcd7f873c997a799156
Bug 1479241: Don't eagerly load AboutPages.jsm in content processes. r=mythmon

Comment 12

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/74910e67768c