Closed Bug 1479241 Opened 3 years ago Closed 3 years ago

Don't eagerly load AboutPages.jsm in content processes

Categories

(Firefox :: Normandy Client, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: [overhead:10k])

Attachments

(1 file)

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.
Summary: Don't eagerly load AboutStudies.jsm in content processes → Don't eagerly load AboutPages.jsm in content processes
Whiteboard: [overhead:10k]
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?
Attachment #8995764 - Flags: review?(mcooper)
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 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.
Component: General → Normandy Client
Product: Shield → Firefox
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"
Attachment #8995764 - Flags: review?(mcooper) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f150e62dcbbdc14bec93db0705470a5d9e71737e
Bug 1479241: Don't eagerly load AboutPages.jsm in content processes. r=mythmon
https://hg.mozilla.org/mozilla-central/rev/f150e62dcbbd
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Backed out for causing a big performance regression on OS X and as request by igoldan. 

https://hg.mozilla.org/mozilla-central/rev/fe6020e5c9d901a40fa2e7ea2f1ab2a36bf6d856
Status: RESOLVED → REOPENED
Flags: needinfo?(kmaglione+bmo)
Resolution: FIXED → ---
Target Milestone: Firefox 63 → ---
https://hg.mozilla.org/integration/mozilla-inbound/rev/74910e67768c6e829d946bcd7f873c997a799156
Bug 1479241: Don't eagerly load AboutPages.jsm in content processes. r=mythmon
https://hg.mozilla.org/mozilla-central/rev/74910e67768c
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.