Closed Bug 1014185 Opened 10 years ago Closed 8 years ago

Remove the about:customizing preloading hack

Categories

(Firefox :: Toolbars and Customization, defect)

x86_64
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 47
Tracking Status
firefox47 --- verified

People

(Reporter: mconley, Assigned: dao)

References

(Depends on 1 open bug)

Details

(Keywords: addon-compat, dev-doc-needed, user-doc-needed, Whiteboard: [MemShrink:P2])

Attachments

(1 file, 2 obsolete files)

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
OS: Windows 7 → All
Blocks: 975552
See Also: 975552
Do you have any ideas on an approach that would allow this? Hard to see us finding a solution here.
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
I think the cleanest way would be to add an about:blank tab and stop the loading immediately...
Blocks: 1047070
No longer blocks: 1047070
Whiteboard: [MemShrink:P2]
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #8720853 - Flags: review?(jaws)
Blocks: 1160162
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
Attachment #8720853 - Flags: review?(jaws) → review+
Attached patch patch v2 (obsolete) — Splinter Review
(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.
Attachment #8720853 - Attachment is obsolete: true
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.
https://hg.mozilla.org/mozilla-central/rev/c34fe673bb97
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1249587
backed this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7403652&repo=fx-team
Status: RESOLVED → REOPENED
Flags: needinfo?(dao)
Resolution: FIXED → ---
Looks like that test is bogus and needs to be rewritten. I'll disable it and file a new bug.
Flags: needinfo?(dao)
(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.
(In reply to :Gijs Kruitbosch from comment #14)
> Uncaught exception - at :0

Lovely.
See Also: → 1249698
(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.
(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...
Depends on: 1249878
Attached patch patch v3Splinter Review
(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
Attachment #8721055 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/48a7a7615da8
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
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...
Keywords: user-doc-needed
(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.
Depends on: 1252102
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
QA Whiteboard: [bugday-20160302]
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]
Status: RESOLVED → VERIFIED
Depends on: 1258344
Depends on: 1258675
Depends on: 1258678
Blocks: 1188800
No longer blocks: 1188800
Depends on: 1327178
See Also: → 1327210
Depends on: 1327210
Depends on: 1347514
Blocks: 957202
You need to log in before you can comment on or make changes to this bug.