Albert Scheiner [:alberts] Reporter
	Description • 10 years ago

At the moment when opening the preferences in a tab for the first time after the browser started I see a blank page for about 0.5-1s. It is a bit irritating as there is no loading indicator and I actually would expect a page to be right there when the tab opens.
The blank page is still there when I open the preferences a second time, but so briefly that it doesn't affect the experience IMO.

	Pavel Cvrcek [:JasnaPaka]
	Comment 1 • 10 years ago

I can confirm this behavior on Windows 7, HWA enabled.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 2 • 10 years ago

It looks like this hang may be related to Sync. In the windowed preferences, the Sync pane wasn't shown by default, but with the new preferences we load up all of the panes but keep them hidden until the category is switched to.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 3 • 10 years ago

gps, do you think you could take a look at this?

	Gregory Szorc [:gps]
	Comment 4 • 10 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> gps, do you think you could take a look at this?

Sorry, but I can't. My only priority right now is the Firefox update hotfix. rnewman should be able to triage.

	Richard Newman [:rnewman]
	Comment 5 • 10 years ago

I can't really reproduce this on my machine (there's a flash of white *every time* I open in-content prefs), perhaps because I don't have a Firefox Account.

Mark, can you repro?

I do see gSyncPanel.init in a profile capture for the first open, which will usually call _init, which calls updateWeavePrefs, which not only reads a bunch of prefs and loads a bunch of code, but for non-legacy-Sync users does fun stuff like:

      fxAccounts.getSignedInUser().then(data => {

I could certainly imagine this being too heavyweight to occur inline with building UI, but building the Sync setup UI involves doing a lot of work, so….

My guess is that there are two ways forward here:

* Don't init a prefs panel that we aren't waiting to show.
* Add some kind of two-phase init.

	Mark Hammond [:markh] [:mhammond]
	Comment 6 • 10 years ago

This patch seems to reduce the impact by initializing sync on a timeout - however, it doesn't remove it entirely - and I don't think it can be totally removed as Sync is always going to spin some nested event loops.  Bug 1007448 might help, but that's not landing for a while.

Jaws' profile looks a little strange - it appears a sync was actually running, which is surprising and I can't repro that via the profiler locally.

It would be great if someone can verify if this significantly improves the situation.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 7 • 10 years ago

Comment on attachment 8431284 [details] [diff] [review]
0001-Bug-1012223-initialize-Sync-on-a-timeout-to-avoid-in.patch

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

(In reply to Richard Newman [:rnewman] from comment #5)
> My guess is that there are two ways forward here:
> 
> * Don't init a prefs panel that we aren't waiting to show.
> * Add some kind of two-phase init.

I think we should go with a two-step initialization here as a first pass, since I can't reliably reproduce that sync delay either.

When opening the in-content preferences, we show the General pane by default, except for cases where Advanced is opened by default (bug 741047) or other panes. We can init the pane that is being shown by default and then init the other panes in the next tick. We'll need all panes to be initialized for implementing preference searching (bug 1017053), but we should prioritize getting something visible and interactive in front of people first.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 8 • 10 years ago

From in-content preference triage today, "As part of fixing this bug, we should include some telemetry to keep track of load times so we can see that this is fixed."

	Hernán Rodriguez Colmeiro (:peregrino)
	Comment 9 • 10 years ago

So, as far as I can see bug 1017053 suggests all the preferences get mixed in a single page. Is that the direction chosen? Any ideas on how it'll impact on initialization time if all the panels have to be enabled at the same time?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 10 • 10 years ago

(In reply to Hernán Rodriguez Colmeiro (:peregrino) from comment #9)
> So, as far as I can see bug 1017053 suggests all the preferences get mixed
> in a single page. Is that the direction chosen? Any ideas on how it'll
> impact on initialization time if all the panels have to be enabled at the
> same time?

They already are all being loaded in to a single page, just set to display:none if they are not the requested view. This is 100% the source of the problem for them loading slowly.

	:Gijs (he/him) Assignee
	Comment 11 • 9 years ago

Taking this per discussion with Jared.

	:Gijs (he/him) Assignee
	Comment 12 • 9 years ago

This saves us a good 150+ms on the first open my very-fast-ssd-retina machine, and I expect still better on slower machines, because the applications and content panes do sloooooow things on initialization (like enumerating fonts and plugins). Initializing everything lazily seems to help a lot. Jared, does this look right to you?

	:Gijs (he/him) Assignee
	Comment 13 • 9 years ago

Note that IME there's still a sizable delay (300ms between hitting the keyboard shortcut (on OS X, anyway) and chrome being notified the prefs have loaded). However, profiling it, it all seems to be stuck in layout/graphics doing painting and so on. I'm not sure that/how we could do better, except perhaps by moving it all to HTML, which is slightly out of scope for this bug. :-)

	Jared Wein [:jaws] (please needinfo? me)
	Comment 14 • 9 years ago

Comment on attachment 8551403 [details] [diff] [review]
make opening about:preferences faster,

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

Overall this looks good. Have you ran it through the automated tests? There might be a test or two that keys off of the "Initialized" event and starts tweaking things before they have been unhidden (not sure).

Hopefully this won't break any add-ons, but I think we'll have to just play the wait-and-see approach.

::: browser/components/preferences/in-content/content.js
@@ +122,5 @@
>                     element  : "defaultFontSize",
>                     fonttype : null }];
>      var preferences = document.getElementById("contentPreferences");
> +    // Ensure preferences are "visible" to ensure bindings work.
> +    preferences.hidden = false;

Please file a follow-up to investigate moving this out of init.

@@ +124,5 @@
>      var preferences = document.getElementById("contentPreferences");
> +    // Ensure preferences are "visible" to ensure bindings work.
> +    preferences.hidden = false;
> +    // Force flush:
> +    var x = preferences.clientHeight;

The `var x = ` should be unnecessary. We don't use it elsewhere when we force a layout flush http://mxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_bug854082.html?force=1#30

::: browser/components/preferences/in-content/sync.xul
@@ +1,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +<!-- Sync panel -->

Might as well just kill this line while we're here. The file is already pretty self-explanatory.

	:Gijs (he/him) Assignee
	Comment 15 • 9 years ago

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=9966850853cf

	:Gijs (he/him) Assignee
	Comment 16 • 9 years ago

Ugh, tests. :-(

	:Gijs (he/him) Assignee
	Comment 17 • 9 years ago

I took the liberty of cleaning up one of these tests to reduce the duplication a bit.

Renewed trypush:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=aac5711b2127

	Jared Wein [:jaws] (please needinfo? me)
	Comment 18 • 9 years ago

Comment on attachment 8553007 [details] [diff] [review]
Interdiff

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

r=me with test_pane_visibility updated.

::: browser/components/preferences/in-content/tests/browser_bug410900.js
@@ -22,5 @@
>                getService(Ci.nsIHandlerService);
>    hserv.store(info);
>  
> -  function observer(win, topic, data) {
> -    if (topic != "app-handler-pane-loaded")

Out of curiosity, did this approach stop working with the change to delay-loading? I can't get any results back from Addons MXR (stalls on "searching...") so I'm not sure if add-ons will break if this has stopped working.

::: browser/components/preferences/in-content/tests/privacypane_tests_perwindow.js
@@ +8,5 @@
>      let browser = aEvent.originalTarget.linkedBrowser;
>      browser.addEventListener("Initialized", function(aEvent) {
>        browser.removeEventListener("Initialized", arguments.callee, true);
>        is(browser.contentWindow.location.href, "about:preferences", "Checking if the preferences tab was opened");
> +      browser.contentWindow.gotoPref("panePrivacy");

Well, this is both sad and surprising.

Can we update test_pane_visibility to actually check that the elements are visible using the is_element_visible function?

	:Gijs (he/him) Assignee
	Comment 19 • 9 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> Comment on attachment 8553007 [details] [diff] [review]
> Interdiff
> 
> Review of attachment 8553007 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with test_pane_visibility updated.
> 
> ::: browser/components/preferences/in-content/tests/browser_bug410900.js
> @@ -22,5 @@
> >                getService(Ci.nsIHandlerService);
> >    hserv.store(info);
> >  
> > -  function observer(win, topic, data) {
> > -    if (topic != "app-handler-pane-loaded")
> 
> Out of curiosity, did this approach stop working with the change to
> delay-loading? I can't get any results back from Addons MXR (stalls on
> "searching...") so I'm not sure if add-ons will break if this has stopped
> working.

Yes, it assumes that that notice gets sent after the preferences load, even if they load on another pane. That is no longer true. Apart from lying about it, I don't think there's much we can do about it.

I just redid the addons search, and get 0 results, so it looks like we're in the clear (excluding dark xul etc. but we can't fix that - I expect the switch to in-content rather than windowed stuff will break more things, anyway).

> :::
> browser/components/preferences/in-content/tests/privacypane_tests_perwindow.
> js
> @@ +8,5 @@
> >      let browser = aEvent.originalTarget.linkedBrowser;
> >      browser.addEventListener("Initialized", function(aEvent) {
> >        browser.removeEventListener("Initialized", arguments.callee, true);
> >        is(browser.contentWindow.location.href, "about:preferences", "Checking if the preferences tab was opened");
> > +      browser.contentWindow.gotoPref("panePrivacy");
> 
> Well, this is both sad and surprising.

Yup. All these checks while the pane wasn't even opened - seems to have worked well for those tests so far! :-\

> Can we update test_pane_visibility to actually check that the elements are
> visible using the is_element_visible function?

I'll give it a shot. Assuming that works and passes locally, I'll land (as the latest try run was very green).

	:Gijs (he/him) Assignee
	Comment 20 • 9 years ago

remote:   https://hg.mozilla.org/integration/fx-team/rev/89cbd02b6589

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/89cbd02b6589

	:Gijs (he/him) Assignee
	Comment 22 • 9 years ago

Comment on attachment 8553005 [details] [diff] [review]
make opening about:preferences faster,

Approval Request Comment
[Feature/regressing bug #]: in-content-prefs/sessionstore
[User impact if declined]: people on aurora/early-beta will run into bug 1034296, and in-content-prefs might seem slow
[Describe test coverage new/current, TreeHerder]: has some implicated test coverage, but no specific test coverage for "this should be fast" (or for bug 1034296, which would be difficult to test for because it requires restarts)
[Risks and why]: low; we're not planning on shipping in-content-prefs in 37, so this doesn't strictly *need* uplift from that perspective and won't endanger shipping 37... but considering the seriousness (mostly wrt add-on compat) of the change, and the fact that it fixes a user-visible weird bug, I'd like to uplift it, essentially to ensure we get an extra late aurora + early beta's worth of test coverage on the change.
[String/UUID change made/needed]: no.

	:Gijs (he/him) Assignee
	Comment 23 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #22)
> [Risks and why]: low; we're not planning on shipping in-content-prefs in 37,
> so this doesn't strictly *need* uplift from that perspective and won't
> endanger shipping 37... but considering the seriousness (mostly wrt add-on
> compat) of the change, and the fact that it fixes a user-visible weird bug,
> I'd like to uplift it, essentially to ensure we get an extra late aurora +
> early beta's worth of test coverage on the change.

In an attempt to make this less confusing (not sure it'll help...): normally uplifting increases shipping (ie release branch) risk because the number of weeks * audience of testing is reduced, and you have less time to fix issues that show up.

Because in-content-prefs is slated for a 38 release, and we'll turn it off in late 37 betas, uplifting here in fact means we get *more* baking (preceding the 38 release) without changing the risk situation on the 37 release (or 37 late beta). IOW, I believe that uplifting this (counterintuitively) is a win in terms of baking/risks, and one we should take also to make life better for our aurora/beta audience.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 24 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #23)
> Because in-content-prefs is slated for a 38 release, and we'll turn it off
> in late 37 betas, uplifting here in fact means we get *more* baking
> (preceding the 38 release) without changing the risk situation on the 37
> release (or 37 late beta). IOW, I believe that uplifting this
> (counterintuitively) is a win in terms of baking/risks, and one we should
> take also to make life better for our aurora/beta audience.

I agree with this reasoning. As long as we're going to present in-content prefs to the Beta audience, we should treat it as though it is shipping and get it into a good state. The patch on this bug, while large, contains a good chunk of test changes and doesn't have anything too scary. It is also completely isolated to in-content prefs so should have no impact on the release once this feature is disabled after Beta 4. We will take this change assuming that the fix for bug 1132031 is ready (looks like the patch hit fx-team today) before the merge to Beta.

	:Gijs (he/him) Assignee
	Comment 26 • 9 years ago

Comment on attachment 8563566 [details] [diff] [review]
make opening about:preferences faster (includes fix for bug 1132031,

Carrying over r+ from florian+jaws

Approval Request Comment: see comment 22, comment 23, comment 24

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 27 • 9 years ago

Comment on attachment 8563566 [details] [diff] [review]
make opening about:preferences faster (includes fix for bug 1132031,

Aurora+ as per my comment 24.

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/d03da8430432

	Camelia Badau [:cbadau], Desktop QA
	Comment 29 • 9 years ago

Verified fixed on Mac OSX 10.9.5 using latest Aurora 38.0a2 (buildID: 20150317004004).