Closed Bug 1012223 Opened 8 years ago Closed 7 years ago

in-content preferences loading slowly

Categories

(Firefox :: Preferences, defect)

32 Branch
x86
macOS
defect
Not set
normal
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.1 - 26 Jan
Tracking Status
firefox36 --- wontfix
firefox37 --- fixed
firefox38 --- verified

People

(Reporter: alberts, Assigned: Gijs)

References

()

Details

Attachments

(4 files, 2 obsolete files)

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.
Blocks: 718011
I can confirm this behavior on Windows 7, HWA enabled.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached image Screenshot of profiler
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.
gps, do you think you could take a look at this?
Flags: needinfo?(gps)
Summary: in-content preferences loading slowly → in-content preferences loading slowly due to Sync
(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.
Flags: needinfo?(gps) → needinfo?(rnewman)
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.
Flags: needinfo?(rnewman)
Flags: needinfo?(mhammond)
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.
Attachment #8431284 - Flags: feedback?(jaws)
Flags: needinfo?(mhammond)
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.
Attachment #8431284 - Flags: feedback?(jaws)
Summary: in-content preferences loading slowly due to Sync → in-content preferences loading slowly
Flags: firefox-backlog+
Points: --- → 5
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."
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?
(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.
Taking this per discussion with Jared.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 37.3 - 12 Jan
Flags: qe-verify?
Flags: qe-verify? → qe-verify-
Iteration: 37.3 - 12 Jan → 38.1 - 26 Jan
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?
Attachment #8551403 - Flags: review?(jaws)
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. :-)
Attachment #8431284 - Attachment is obsolete: true
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.
Attachment #8551403 - Flags: review?(jaws) → review+
See Also: → 1124416
Ugh, tests. :-(
Attachment #8553005 - Flags: review?(jaws)
Attachment #8551403 - Attachment is obsolete: true
Attached patch InterdiffSplinter Review
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
Attachment #8553007 - Attachment is patch: true
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?
Attachment #8553007 - Flags: review+
(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).
Attachment #8553005 - Flags: review?(jaws) → review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/89cbd02b6589
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/89cbd02b6589
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
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.
Attachment #8553005 - Flags: approval-mozilla-aurora?
(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.
Depends on: 1132031
No longer depends on: 1132031
Depends on: 1132031
Whiteboard: Uplift w/ fix for bug 1132031 when applicable
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8553005 - Flags: approval-mozilla-aurora?
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
Flags: needinfo?(lmandel)
Attachment #8563566 - Flags: review+
Attachment #8563566 - Flags: approval-mozilla-aurora?
Whiteboard: Uplift w/ fix for bug 1132031 when applicable
Comment on attachment 8563566 [details] [diff] [review]
make opening about:preferences faster (includes fix for bug 1132031,

Aurora+ as per my comment 24.
Flags: needinfo?(lmandel)
Attachment #8563566 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1141055
Verified fixed on Mac OSX 10.9.5 using latest Aurora 38.0a2 (buildID: 20150317004004).
Status: RESOLVED → VERIFIED
Depends on: 1160076
You need to log in before you can comment on or make changes to this bug.