1014185 - Remove the about:customizing preloading hack

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

As part of our effort to make the transition to customize smoother, we preload about:customizing[1] so that the cost of instantiating a docshell / browser doesn't jankify the transition.

We preload about:customizing when the user opens the menu panel.

That's a pretty bodacious hack, and means that we've got a docshell / browser just sitting in the background for the most part doing nothing.

We should try to remove this hack to help conserve memory, and to generally simplify things.

[1]: See bug 975552

Comment 1

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

Do you have any ideas on an approach that would allow this? Hard to see us finding a solution here.

Comment 2

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

(In reply to :Gavin Sharp (email [e-mail]) from comment #1)
> Do you have any ideas on an approach that would allow this? Hard to see us
> finding a solution here.

I know there were some ideas like making it possible for tabs to be browserless, or to share a browser, but that sounds pretty fragile, and might break some deep assumptions the rest of the codebase (and add-on ecosystem) make about tabbrowser.

I think we need to find a way of not needing the browser in the tab to actually _load_ about:customizing, and still receive the notification that the user is attempting to browse there to initialize the transition. I think having the <browser> element be created is fine. It's the actual loading of the about:customizing document that is unnecessary.

Comment 3

[Dão Gottwald [:dao]]

I think the cleanest way would be to add an about:blank tab and stop the loading immediately...

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch

Comment 6

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

Comment on attachment 8720853 [details] [diff] [review]
patch

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

We may see some changes to CART due to this, how much of a regression are we willing to accept?

::: browser/components/build/nsModule.cpp
@@ -106,5 @@
>      { NS_ABOUT_MODULE_CONTRACTID_PREFIX "accounts", &kNS_BROWSER_ABOUT_REDIRECTOR_CID },
>  #ifdef MOZ_SERVICES_HEALTHREPORT
>      { NS_ABOUT_MODULE_CONTRACTID_PREFIX "healthreport", &kNS_BROWSER_ABOUT_REDIRECTOR_CID },
>  #endif
> -    { NS_ABOUT_MODULE_CONTRACTID_PREFIX "customizing", &kNS_BROWSER_ABOUT_REDIRECTOR_CID },

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/browser_bug1163570.js still has a reference to about:customizing

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch v2

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> We may see some changes to CART due to this, how much of a regression are we
> willing to accept?

Well, not much... I can't give you a specific number. I'll land this and see how it goes.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c34fe673bb97

Comment 9

[Dão Gottwald [:dao]]

So far it looks like there's a CART improvement with !e10s and a regression with e10s. I think forcing the blank tab to be non-remote would probably turn that regression around.

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Carsten Book [:Tomcat]]

backed this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7403652&repo=fx-team

Comment 12

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/69ec3dc408a2

Comment 13

[Dão Gottwald [:dao]]

Looks like that test is bogus and needs to be rewritten. I'll disable it and file a new bug.

Comment 14

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #13)
> Looks like that test is bogus and needs to be rewritten. I'll disable it and
> file a new bug.

There were also some other failures that look related though, like:

 73 INFO TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_876944_customize_mode_create_destroy.js | Uncaught exception - at :0 - Error: operation not possible on dead CPOW 

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=26771bc857f8&selectedJob=7407922

and a few other CUI tests in that run with CPOW errors.

Comment 15

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #14)
> Uncaught exception - at :0

Lovely.

Comment 16

[William Lachance (:wlach)]

As stated, this had some negative performance implications on the cart numbers before it was backed out:

https://treeherder.allizom.org/perf.html#/alerts?id=198

More detailed breakdown on winxp:

https://treeherder.allizom.org/perf.html#/comparesubtest?originalProject=fx-team&originalRevision=f9e4a56281d8&newProject=fx-team&newRevision=c34fe673bb97&originalSignature=e0de5c766545056e21f79d23c94bd212f2426249&newSignature=e0de5c766545056e21f79d23c94bd212f2426249

Comment 17

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to Dão Gottwald [:dao] from comment #13)
> > Looks like that test is bogus and needs to be rewritten. I'll disable it and
> > file a new bug.
> 
> There were also some other failures that look related though, like:
> 
>  73 INFO TEST-UNEXPECTED-FAIL |
> browser/components/customizableui/test/
> browser_876944_customize_mode_create_destroy.js | Uncaught exception - at :0
> - Error: operation not possible on dead CPOW 
> 
> https://treeherder.mozilla.org/#/jobs?repo=fx-
> team&revision=26771bc857f8&selectedJob=7407922
> 
> and a few other CUI tests in that run with CPOW errors.

I suspect the patch in bug 1249587 might fix this.

Comment 18

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #17)
> > and a few other CUI tests in that run with CPOW errors.
> 
> I suspect the patch in bug 1249587 might fix this.

It didn't. Maybe the problem is endCustomizing in browser/components/customizableui/test/head.js...

Comment 19

[Dão Gottwald [:dao]]

Attachment: patch v3

(In reply to Dão Gottwald [:dao] from comment #18)
> Maybe the problem is endCustomizing in
> browser/components/customizableui/test/head.js...

Yeah, fixing endCustomizing seems to have helped. https://treeherder.mozilla.org/#/jobs?repo=try&revision=06859665a156&selectedJob=16990382

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/48a7a7615da8

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/48a7a7615da8

Comment 22

[Dão Gottwald [:dao]]

https://support.mozilla.org/en-US/kb/customize-firefox-controls-buttons-and-toolbars says "Tip: Another option to open Customize firefox tab is to type in Location bar about:customizing." We need to remove that note. Not that it was ever useful to end users in the first place...

Comment 23

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #22)
> https://support.mozilla.org/en-US/kb/customize-firefox-controls-buttons-and-
> toolbars says "Tip: Another option to open Customize firefox tab is to type
> in Location bar about:customizing." We need to remove that note. Not that it
> was ever useful to end users in the first place...

I submitted a revision.

There are also several hits in the add-on MXR: https://mxr.mozilla.org/addons/search?string=about%3Acustomizing&find=.js , so I'm adding devdoc-needed (we should put this in the "Firefox 47 for developers" page) and addon-compat.

Comment 24

[Khalid Syfullah Zaman [:khalid32]]

I've reproduced this bug on Firefox Nightly 32.0a1(2014-05-21) on Ubuntu 14.04 LTS, 32-bit!

The bug's fix is now verified on Latest Firefox Nightly 47.0a1!

Build ID: 20160303030253
User Agent: Mozilla/5.0 (X11; Linux i686; rv:47.0) Gecko/20100101 Firefox/47.0

Comment 25

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 32.0a1 (Build ID: 20140521030200) on 
windows 8.1, 64-bit with the instructions from comment 0.

Verified as fixed with Firefox Nightly 47.0a1 (Build ID: 20160307030208)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0

As this bug is also verified on Linux (Comment 24), I am marking this as verified!

[bugday-20160302]