1353174 - Turn ContentObservers.jsm into a process script

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Andrew McCreight [:mccr8]]

The comment at the top of ContentObserver.jsm says "This module is for small observers that we want to register once per content process, usually in order to forward content-based observer service notifications to the chrome process through message passing. Using a JSM avoids having them in content.js and thereby registering N observers for N open tabs, which is bad for perf."

If we turn this into a process script (maybe loaded from nsBrowserGlue.js like pdf.js does) then we can get the benefits of running only once per process without the overhead of one jsm per content process.

Comment 1

[Andrew McCreight [:mccr8]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6da979ca1393aa8e56c1ba149f6cf0ad1253fe1&filter-tier=1

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1353174, part 1 - Turn ContentObservers.jsm into a process script.

This retains the advantage of running only once per process, while
avoiding the per-process overhead of a jsm.

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

Comment 3

[Andrew McCreight [:mccr8]]

Hmm, that's not a good diff. I'll see if I can get it to actually show the move. Somehow.Hmm, so that git

Comment 4

[:Gijs (he/him)]

Comment on attachment 8854521 [details]
Bug 1353174, part 1 - Turn ContentObservers.jsm into a process script.

https://reviewboard.mozilla.org/r/126480/#review129010

::: browser/components/nsBrowserGlue.js:862
(Diff revision 1)
> +
> +    Services.ppmm.loadProcessScript("resource:///modules/ContentObservers.js", true);

I think this is too late, and I'm not sure why pdfjs (higher up in this method) waits this long, either. This is firing after the delayed startup of the first browser window finishes, but content.js is added to the content browsers here:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1154-1158

which is significantly earlier (onLoad rather than delayed startup).

Comment 5

[:Gijs (he/him)]

Comment on attachment 8854521 [details]
Bug 1353174, part 1 - Turn ContentObservers.jsm into a process script.

https://reviewboard.mozilla.org/r/126480/#review129010

::: browser/components/nsBrowserGlue.js:862
(Diff revision 1)
> +
> +    Services.ppmm.loadProcessScript("resource:///modules/ContentObservers.js", true);

I think this is too late, and I'm not sure why pdfjs (higher up in this method) waits this long, either. This is firing after the delayed startup of the first browser window finishes, but content.js is added to the content browsers here:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1154-1158

which is significantly earlier (onLoad rather than delayed startup).

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Bug 1353174, part 2 - Fix up ContentObservers.js.

This is split into a separate patch in an attempt to get a better
diff.

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

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8854521 [details]
Bug 1353174, part 1 - Turn ContentObservers.jsm into a process script.

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

Comment 8

[:Gijs (he/him)]

Comment on attachment 8854593 [details]
Bug 1353174, part 2 - Fix up ContentObservers.js.

https://reviewboard.mozilla.org/r/126550/#review129088

Comment 9

[:Gijs (he/him)]

Comment on attachment 8854521 [details]
Bug 1353174, part 1 - Turn ContentObservers.jsm into a process script.

https://reviewboard.mozilla.org/r/126480/#review129090

Comment 10

[Andrew McCreight [:mccr8]]

I didn't realize that browser.js is run once per browser window, though I suppose that makes sense.

By putting this code in MainProcessSingleton.js it seems to load it once per process, as expected, and it loads before browser.js runs, so hopefully that is early enough, but not too early.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05ba30ffa434
part 1 - Turn ContentObservers.jsm into a process script. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/fe34ea100aa3
part 2 - Fix up ContentObservers.js. r=Gijs

Comment 12

[:Gijs (he/him)]

I would have expected this to run into bug 1352982. I don't really understand why it didn't. Florian, do you?

Comment 13

[Florian Quèze [:florian]]

(In reply to :Gijs from comment #12)
> I would have expected this to run into bug 1352982. I don't really
> understand why it didn't. Florian, do you?

I don't really understand either. Is it possible that the browser_all_files_referenced.js test has not been run in any non-clobber os x build since this patch was pushed to autoland?

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/05ba30ffa434
https://hg.mozilla.org/mozilla-central/rev/fe34ea100aa3