899222 - Make about:home work in e10s

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: about-home

Our goal for most about: pages is to load them non-remotely. However, about:home is different because it loads content from the network. Therefore we'd like to make sure it's sandboxed.

This patch adds code to communicate between the chrome and content process whenever the user clicks on the buttons for opening the downloads manager and the like. It also sends the snippets down to the content process and informs it when the search engine has changed.

The one tricky part was dealing with search engine changes. The chrome process needs to listen for changes to the search engine and send them to any tabs that have about:home open. When one of those tabs closes or navigates away, we need to stop listening.

Ideally, the content process should be able to register a "pagehide" handler and tell the chrome process to stop listening. However, when everything runs in the same process (as we do now in Firefox) the "pagehide" handler can be too late to send any messages. If the tab is being closed, the messageManager for the content script has already been disconnected when "pagehide" fires. I worked around this by listening for "TabClose" in the chrome process. I'm welcome to other suggestions though.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: remove old about:home

This patch removes the old about:home.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 782692 [details] [diff] [review]
about-home

Review of attachment 782692 [details] [diff] [review]:
-----------------------------------------------------------------

Just as a drive-by comment: I think all the browser.js additions should be moved into a separate browser-aboutHome.js file and included at the top.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Bill McCloskey (:billm) from comment #0)
> The one tricky part was dealing with search engine changes. The chrome
> process needs to listen for changes to the search engine and send them to
> any tabs that have about:home open. When one of those tabs closes or
> navigates away, we need to stop listening.

Why do we need to stop listening? Why does the chrome process need to keep track of about:home being loaded at all rather than sending a global message for search engine changes?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'll make those changes. There isn't really a good reason not to send the search engine changes to every tab.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: about-home v2

I made the changes suggested by Tim and Dão.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: elim-old-about-home v2

This is just a rebase.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 783981 [details] [diff] [review]
about-home v2

Review of attachment 783981 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but I think we have a good opportunity here to reduce this feature's footprint instead of increasing it, by making the code going into browser-aboutHome.js a .jsm instead. It will be a little cumbersome to call the window's functions, but you can use the message.target in the receiveMessage function to discover which browser (and therefore also which window) received the message. And then you can use the global message manager to register the listeners only once instead of per-window.

Even the code in content.js could be made a .jsm similar to LoginManagerContent in that same file

::: browser/base/content/browser.js
@@ +754,5 @@
>      Services.obs.addObserver(gPluginHandler.pluginCrashed, "plugin-crashed", false);
>  
>      window.addEventListener("AppCommand", HandleAppCommandEvent, true);
>  
> +    AboutHomePage.init();

This is very early in the browser window startup process. Did you find any use case where initializing this was necessary here, or can this be pushed for later? It'd be great if this could go in delayedStartup, but I'm guessing by that time Session Restore might have already loaded about:home. But worth a try.

If not, maybe it can at least be grouped with the "Misc. inits." group, although that matters less.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 782694 [details] [diff] [review]
remove old about:home

the two review requested ended up in the same patch (remove old about-home) instead of one of each patch

Comment 9

[Marco Bonardo [:mak]]

(In reply to :Felipe Gomes from comment #7)
>  by making the code going into
> browser-aboutHome.js a .jsm instead.

*cough* AboutHomeUtils.jsm *cough*

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: about-home v3

This renames AboutHomeUtils.jsm to AboutHome.jsm and moves the parent-side logic to that file.

I considered moving the stuff in the content script there as well. It would be a lot of work, though, because it uses globals like |content| in many places. We could store references to the content windows in the JSM, but then we'd have to worry about clearing them when the window goes away. Also, we already have a cache of content scripts, so the script bytecode is shared between windows anyway. I don't see any other advantages to moving this code to a JSM.

I looked at when the session restore code fires and when the delayed startup code fires. Session restore happens once the session data has been read from disk. Delayed startup happens after the first paint. I don't know of any reason why the first paint would happen before the disk I/O, so it seems safer not to put this code in delayed startup.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 786049 [details] [diff] [review]
about-home v3

Review of attachment 786049 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +1338,5 @@
>  
>      CombinedStopReload.uninit();
>  
> +    AboutHome.uninit();
> +

this is incorrect now because this will be called when any window closes. Uninit should be unecessary as AboutHome will have the same lifetime as the process

::: browser/modules/AboutHome.jsm
@@ +109,5 @@
> +    "AboutHome:RequestUpdate",
> +    "AboutHome:Search",
> +  ],
> +
> +  init: function() {

init needs to prevent against being called more than once (which will happen when a second window opens)

@@ +119,5 @@
> +
> +    Services.obs.addObserver(this, "browser-search-engine-modified", false);
> +  },
> +
> +  uninit: function() {

It should be possible to remove uninit, unless doing so will cause memory leak reports, in which case you can listen for shutdown and uninit there

Comment 12

[Dão Gottwald [:dao]]

(In reply to :Felipe Gomes from comment #11)
> > +  init: function() {
> 
> init needs to prevent against being called more than once (which will happen
> when a second window opens)

No, you should call it from nsBrowserGlue.js rather than browser.js.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: about-home v4

Here's a new patch that uses nsBrowserGlue.js. I left the uninit code in. However, I did two try pushes:

with uninit: https://tbpl.mozilla.org/?tree=Try&rev=c4e54bba2193
without: https://tbpl.mozilla.org/?tree=Try&rev=fd08b10cb302

If the without one comes back clean, I'll remove the uninit code.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 786433 [details] [diff] [review]
about-home v4

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js

>+XPCOMUtils.defineLazyModuleGetter(this, "AboutHome",
>+  "resource:///modules/AboutHome.jsm");

This appears to be unused.

>+let AboutHomeListener = {

>+  QueryInterface: function QueryInterface(aIID) {
>+    if (aIID.equals(Ci.nsIWebProgressListener) ||
>+        aIID.equals(Ci.nsISupportsWeakReference) ||
>+        aIID.equals(Ci.nsISupports)) {
>+        return this;
>+    }

'return this;' is indented too far.

Comment 15

[:Felipe Gomes (needinfo for replies!)]

(In reply to Dão Gottwald [:dao] from comment #14)
> Comment on attachment 786433 [details] [diff] [review]
> about-home v4
> 
> >--- a/browser/base/content/browser.js
> >+++ b/browser/base/content/browser.js
> 
> >+XPCOMUtils.defineLazyModuleGetter(this, "AboutHome",
> >+  "resource:///modules/AboutHome.jsm");
> 
> This appears to be unused.
> 

good call, AboutHome is unused, but browser.js uses AboutHomeUtils, so instead of removing you'll need to change the param to AboutHomeUtils

Comment 16

[Dão Gottwald [:dao]]

(In reply to :Felipe Gomes from comment #15)
> good call, AboutHome is unused, but browser.js uses AboutHomeUtils, so
> instead of removing you'll need to change the param to AboutHomeUtils

Attachment 783982 [details] [diff] removes all of that.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

oh, that's right

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a165a59f5316

Thanks everyone!

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This seems to have regressed Tp5. Does anyone know what that measures and why it might be affected?

Regression: Mozilla-Inbound - Tp5 Optimized - MacOSX 10.7 - 3.45% increase
--------------------------------------------------------------------------
    Previous: avg 280.804 stddev 1.363 of 12 runs up to revision b5319d82251e
    New     : avg 290.496 stddev 1.915 of 12 runs since revision a165a59f5316
    Change  : +9.692 (3.45% / z=7.111)
    Graph   : http://mzl.la/13jgByg

Comment 20

[:Gavin Sharp [email: [e-mail]]]

It measures page load time, and the progress listener you added now runs frequently during all page loads (in addition to the existing one in browser.js). That's probably the cause. Not sure what to do about it. Are there lighter-weight ways of doing this that are isolated to about:home loads?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #20)
> It measures page load time, and the progress listener you added now runs
> frequently during all page loads (in addition to the existing one in
> browser.js). That's probably the cause. Not sure what to do about it. Are
> there lighter-weight ways of doing this that are isolated to about:home
> loads?

I put a dump() call inside the onStateChange handler and it doesn't seem to trigger that often. However, I can't think of any other reason why the test would get slower. I did a try push with the web progress listener disabled so we can see if that's any faster.

If the listener is at fault, then we could probably use the existing TabProgressListener in browser.js to tell the page's content script that it's loading about:home. At least that way we're not adding an extra listener.

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm going to reopen this to deal with the Tp5 regression.

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: faster-about-home

The web progress listener in the content script was what made Tp5 slower. This patch eliminates it. Instead, about:home dispatches a custom event when it's loaded that is handled by the content script. I think this should be faster because it doesn't have any per-page overhead except when loading about:home. I'm still waiting for new Tp5 results.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I also backed out the original patch.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdce12c4f699

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 787852 [details] [diff] [review]
faster-about-home

>+  var event = new CustomEvent('AboutHomeLoad', {bubbles:true});

nit: please use double quotes.

>+    let self = this;
>+    chromeGlobal.addEventListener('AboutHomeLoad', function(e) { self.onPageLoad(); }, false, true);
>   },

Can you make AboutHomeListener implement handleEvent and pass 'this' to addEventListener instead of the closure?

Comment 27

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 787852 [details] [diff] [review]
faster-about-home

Review of attachment 787852 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/content.js
@@ +54,5 @@
>  }
>  
>  let AboutHomeListener = {
> +  init: function(chromeGlobal) {
> +    addMessageListener("AboutHome:Update", this);

I think you could now only add this listener for an about:home page, right? by calling it from onPageLoad

r+ with this and dao's suggestions

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Merge of backout to m-c.
https://hg.mozilla.org/mozilla-central/rev/cdce12c4f699

Comment 29

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Ugh, I pushed this after forgetting to apply the review requests.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b1cf3dd025e
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a62bbd033a2

I'll land it again once I've tested them.

Comment 30

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f68b32be429

As a side note, the reason for bug 903208 was bug 888972.

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0f68b32be429