1474155 - Lazify more of browser-content.js and content.js

Status: RESOLVED FIXED

(Toolkit :: General, enhancement, P1)

Description

[Kris Maglione [:kmag]]

There's still quite a lot in browser-content.js and content.js that will never need to be loaded in most iframe documents, and/or will not need to be loaded until the documents are interacted with.

There will need to be several more bugs like this... but let's just keep chipping away.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1474155: Part 1 - Move PopupBlocking to a separate JSM.

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

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1474155: Part 2 - Move AutoCompletePopup to a separate JSM.

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

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1474155: Part 3 - Move WebChannel message listeners to a separate JSM.

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

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1474155: Part 4 - Move ClickEventHandler to a separate JSM.

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

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1474155: Part 5 - Null out frame script singleton init methods after they're called.

Believe it or not, the memory these references hold alive is significantl
Nulling them out saves 5-10K per process.

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

Comment 6

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

Comment on attachment 8990569 [details]
Bug 1474155: Part 1 - Move PopupBlocking to a separate JSM.

https://reviewboard.mozilla.org/r/255632/#review262860

Looks good. Thanks for copying PopupBlocking.jsm from browser-content.js to save the history!

::: toolkit/modules/PopupBlocking.jsm:10
(Diff revision 1)
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");

Unless I'm mistaken, I don't think either of these are used.

Comment 7

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

Comment on attachment 8990570 [details]
Bug 1474155: Part 2 - Move AutoCompletePopup to a separate JSM.

https://reviewboard.mozilla.org/r/255634/#review262866

Thanks!

::: toolkit/modules/AutoCompletePopupContent.jsm:10
(Diff revision 1)
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");

Neither of these seem to be used here either.

Comment 8

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

Comment on attachment 8990571 [details]
Bug 1474155: Part 3 - Move WebChannel message listeners to a separate JSM.

https://reviewboard.mozilla.org/r/255636/#review262872

r-'ing because I don't think the WebChannelMessageToContent thing is working (at least by inspection). Or am I misunderstanding something here?

::: toolkit/modules/WebChannelContent.jsm:14
(Diff revision 1)
> +
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  
> -ChromeUtils.defineModuleGetter(this, "AutoCompletePopup",
> -  "resource://gre/modules/AutoCompletePopupContent.jsm");
> +function getMessageManager(event) {
> +  let window = ChromeUtils.getGlobalForObject(event.target);

TIL about getGlobalForObject. Handy!

::: toolkit/modules/WebChannelContent.jsm:15
(Diff revision 1)
> -ChromeUtils.defineModuleGetter(this, "AutoScrollController",
> -  "resource://gre/modules/AutoScrollController.jsm");
> +  return window.document.docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +  .getInterface(Ci.nsIContentFrameMessageManager);

From what I've seen, I think this formatting is more idiomatic for long incantations like this:

```js

return window.document
             .docShell
             .QueryInterface(Ci.nsIInterfaceRequestor)
             .getInterface(Ci.nsIContentFrameMessageManager);
             
```

::: toolkit/modules/WebChannelContent.jsm:34
(Diff revision 1)
>  
> -  init() {
> -    addEventListener("WebChannelMessageToChrome", e => {
> -      this._onMessageToChrome(e);
> -    }, true, true);
> +  handleEvent(event) {
> +    switch (event.type) {
> +      case "WebChannelMessageToChrome":
> +        return this._onMessageToChrome(event);
> +      case "WebChannelMessageToContent":

I think I'm missing something here... WebChannelMessageToContent is a message from the message manager, and is only an event, I think, for the page that's being communicated to...

So this should be in a receiveMessage unless there's some message<->event conversion voodoo going on that I'm not seeing.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8990571 [details]
Bug 1474155: Part 3 - Move WebChannel message listeners to a separate JSM.

https://reviewboard.mozilla.org/r/255636/#review262872

> I think I'm missing something here... WebChannelMessageToContent is a message from the message manager, and is only an event, I think, for the page that's being communicated to...
> 
> So this should be in a receiveMessage unless there's some message<->event conversion voodoo going on that I'm not seeing.

Oh. Yes. I knew something seemed wrong there... Whoever had the idea to call that parameter `e` ...

Comment 10

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

Comment on attachment 8990572 [details]
Bug 1474155: Part 4 - Move ClickEventHandler to a separate JSM.

https://reviewboard.mozilla.org/r/255638/#review262878

This looks okay - but I think you'll want to change the ClickEventHandler to a lazy proxy.

::: browser/base/content/content.js:37
(Diff revision 1)
>  
>  XPCOMUtils.defineLazyProxy(this, "contextMenu", () => {
>    return new ContextMenu(global);
>  });
>  
> +XPCOMUtils.defineLazyGetter(this, "ClickEventHandler", () => {

Did this mean to be a lazy proxy? Because I believe the adding as a system event listener is going to cause this JSM to load.

Comment 11

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

Comment on attachment 8990573 [details]
Bug 1474155: Part 5 - Null out frame script singleton init methods after they're called.

https://reviewboard.mozilla.org/r/255640/#review262838

As mentioned in IRC, this is a pretty surprising discovery. This feels like the sort of thing we can easily backslide on because it's not immediately obvious. I'll make sure to call it out at the next bi-weekly Desktop meeting, but we should probably see if there's a way we can either do this automatically, or add some linting to enforce this by machine.

::: commit-message-40252:3
(Diff revision 1)
> +Bug 1474155: Part 5 - Null out frame script singleton init methods after they're called. r?mconley
> +
> +Believe it or not, the memory these references hold alive is significantl

Nit - typo: "significant1" -> "significant!"

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8990572 [details]
Bug 1474155: Part 4 - Move ClickEventHandler to a separate JSM.

https://reviewboard.mozilla.org/r/255638/#review262878

> Did this mean to be a lazy proxy? Because I believe the adding as a system event listener is going to cause this JSM to load.

Yeah. Good catch.

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8990573 [details]
Bug 1474155: Part 5 - Null out frame script singleton init methods after they're called.

https://reviewboard.mozilla.org/r/255640/#review262838

> Nit - typo: "significant1" -> "significant!"

It's an ell, not a one :) It was meant to be a '.'

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8990573 [details]
Bug 1474155: Part 5 - Null out frame script singleton init methods after they're called.

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

Comment 15

[Kris Maglione [:kmag]]

I actually tracked down and ran the WebChannel tests this time :) That was the only thing I didn't manage to properly test the first time around.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Kris Maglione [:kmag] from comment #13)
> Comment on attachment 8990573 [details]
> Bug 1474155: Part 5 - Null out frame script singleton init methods after
> they're called.

Can you explain why we would null out the init functions? Is it because we expect that they would only get called once? If so, are there other methods that would exhibit this same behavior? Could we do something to annotate these as self-destructing methods? It also seems like we could write a test that tries to call these self-destructing methods two times and expects no exception in the first call and a TypeError on the second call.

Comment 17

[Kris Maglione [:kmag]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> (In reply to Kris Maglione [:kmag] from comment #13)
> > Comment on attachment 8990573 [details]
> > Bug 1474155: Part 5 - Null out frame script singleton init methods after
> > they're called.
> 
> Can you explain why we would null out the init functions? Is it because we
> expect that they would only get called once? If so, are there other methods
> that would exhibit this same behavior? Could we do something to annotate
> these as self-destructing methods? It also seems like we could write a test
> that tries to call these self-destructing methods two times and expects no
> exception in the first call and a TypeError on the second call.

It's because we know they're only called once, and keeping a reference to them means they continue to consume memory.

I'm not sure what the point of a test would be.

Comment 18

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

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> Could we do something to annotate
> these as self-destructing methods? It also seems like we could write a test
> that tries to call these self-destructing methods two times and expects no
> exception in the first call and a TypeError on the second call.

Perhaps this is something we can do with linting. These types of functions tend to be named init and uninit, so assuming they're only ever supposed to be called once per instance, we might be able to add some ESLint foo to check for deletions somewhere in the function (and we opt-out the odd cases where init() or uninit() might be called multiple times).

Unless I'm very mistaken, I suspect we won't be able to get ESLint to detect the single-use-function case automatically without leaning on the naming conventions. :/

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Kris Maglione [:kmag] from comment #17)
> I'm not sure what the point of a test would be.

The purpose of the test would be to ensure that the `this.init = null` line doesn't get removed without knowledge of the side-effects.

Comment 20

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

Something kats suggested is to use something like JS coverage to determine in our debug builds when methods on objects in framescripts are called once and only once.

Comment 21

[Kris Maglione [:kmag]]

(In reply to Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th)) from comment #20)
> Something kats suggested is to use something like JS coverage to determine
> in our debug builds when methods on objects in framescripts are called once
> and only once.

That might be a good idea long term.

In the short term, it would probably be enough to just add an ESLint rule to check that any init() methods in singleton objects have `this.init = null` as their last line.

Well, actually, in the long term I'm hoping to get rid of frame scripts entirely, so...

Comment 22

[Kris Maglione [:kmag]]

Oh, did I screw up pushing my updated patches?

Comment 23

[Kris Maglione [:kmag]]

Attachment: Bug 1474155: Part 1 - Move PopupBlocking to a separate JSM.

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

Comment 24

[Kris Maglione [:kmag]]

Attachment: Bug 1474155: Part 2 - Move AutoCompletePopup to a separate JSM.

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

Comment 25

[Kris Maglione [:kmag]]

Attachment: Bug 1474155: Part 4 - Move ClickEventHandler to a separate JSM.

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

Comment 26

[Kris Maglione [:kmag]]

Attachment: Bug 1474155: Part 3 - Move WebChannel message listeners to a separate JSM.

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

Comment 27

[Kris Maglione [:kmag]]

Comment on attachment 8990573 [details]
Bug 1474155: Part 5 - Null out frame script singleton init methods after they're called.

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

Comment 28

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36055fa716f90e4be18b1e4f6e589e3223d307f3
Bug 1474155: Part 1 - Move PopupBlocking to a separate JSM. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/a87e6be9d9dfeb33761351329187ee461da344f7
Bug 1474155: Part 2 - Move AutoCompletePopup to a separate JSM. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/145701c9e7f765ed00c18318047d652131c3baf4
Bug 1474155: Part 4 - Move ClickEventHandler to a separate JSM. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/9175b19b1b4933aeb879323f059f4be1efadba28
Bug 1474155: Part 5 - Null out frame script singleton init methods after they're called. r=mconley

Comment 29

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

(Cleared review? flags on things that already landed)

Comment 30

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

Comment on attachment 8992543 [details]
Bug 1474155: Part 3 - Move WebChannel message listeners to a separate JSM.

https://reviewboard.mozilla.org/r/257410/#review264504

Looks good, thanks!

::: toolkit/modules/WebChannelContent.jsm:81
(Diff revision 1)
> -// This should be kept in sync with /browser/base/content.js.
> +  // This should be kept in sync with /browser/base/content.js.
> -// Add message listener for "WebChannelMessageToContent" messages from chrome scripts.
> +  // Add message listener for "WebChannelMessageToContent" messages from chrome scripts.

Nit: I don't think these comments make much sense anymore. Probably more confusing than anything, and can be cut.

Comment 31

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/36055fa716f9
https://hg.mozilla.org/mozilla-central/rev/a87e6be9d9df
https://hg.mozilla.org/mozilla-central/rev/145701c9e7f7
https://hg.mozilla.org/mozilla-central/rev/9175b19b1b49

Comment 32

[Ionuț Goldan [:igoldan]]

AWSY perf wins from either this bug or bug 1474131:

== Change summary for alert #14386 (as of Tue, 17 Jul 2018 05:24:57 GMT) ==

Improvements:

  3%  Base Content Resident Unique Memory windows10-64 pgo stylo     16,838,656.00 -> 16,281,941.33
  3%  Base Content Resident Unique Memory windows10-64 opt stylo     16,883,285.33 -> 16,342,698.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14386

Comment 33

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d3fd5f71734c96286e05e9d3ab0b461e97d55e3
Bug 1474155: Part 3 - Move WebChannel message listeners to a separate JSM. r=mconley

Comment 34

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/2d3fd5f71734