Teodora reported this in https://bugzilla.mozilla.org/show_bug.cgi?id=1204510#c23.

I've investigated this, and we're seeing main thread jank, and unexplained GFX corruption. What's happening is that in response to "load" (on the window), we're: 1) updating the DOM to show the spinner; 2) loading the logins with a main-thread janking synchronous load; 3) updating the DOM to hide the spinner. This is all happening on the same JS tick, so we never see the spinner; instead we see a single update (to the loaded state) after the load has completed. The solution is to ensure that 2) happens the tick after 1). That's tricky 'cuz this code is synchronous; but it shows Promise and is not hard to address. Patch incoming.

Bug 1208534 - Ensure about:logins animated CSS spinner is painted before janky main-thread load. r?ally Right now, in response to "load" (on the window), we're: 1) updating the DOM to show the spinner; 2) loading the logins with a main-thread janking synchronous load; 3) updating the DOM to hide the spinner. This is all on the main-thread, so we only see a layout and paint after 3). Thus no interstitial is ever visible, and the logins list pops in after a long delay. This patch ensures that 2) occurs at least one layout after 1). This allows a paint to occur with the interstitial visible. Since the animated CSS spinner is carefully designed to hit the off-main-thread animation pipeline, it animates smoothly even though the main-thread janking synchronous load blocks JavaScript progress. There is a small race window between the promises resolving and the _logins member being accessed by the filter. It's not clear that this was ever well guarded, so I haven't tried to mitigate.

Comment on attachment 8666235 [details] MozReview Request: Bug 1208534 - Part 2: Fix test. r?mfinkle https://reviewboard.mozilla.org/r/20469/#review18389 This was very difficult to wrap my head around, but I have thought about it for a couple of days and I don't see a good way to make this simpler. I think its within reason for uplift as well. ::: mobile/android/chrome/content/aboutLogins.js:53 (Diff revision 1) > - _getLogins: function() { > + // Load the logins list, displaying interstitial UI while loading. I'd actually like to name the interstitial UI in the comment, so future readers who aren't me knows exactly where to look for it. Its 'logins-list-loading-body' ::: mobile/android/chrome/content/aboutLogins.js:55 (Diff revision 1) > + // be careful when modifying it! uber nit: capital B to start the sentence. ::: mobile/android/chrome/content/aboutLogins.js:71 (Diff revision 1) > - logins = Services.logins.getAllLogins(); > + let logins = Services.logins.getAllLogins(); Please add back the bit of comment about Masterpassword from the catch block. If masterpassword has not been entered, getAllLogins() will throw. Disentangling MP related failures isn't easy so let's leave that breadcrumb for future engineers. ::: mobile/android/chrome/content/aboutLogins.js:133 (Diff revision 1) > + debug("Failed to _reloadList: " + e.toString()); I'd add "aboutLogins" to the debug message. debug messages I've noticed have a tendency to get lost in the noise unless they point the finger at a feature in an obvious way.

https://reviewboard.mozilla.org/r/20469/#review18389 > I'd actually like to name the interstitial UI in the comment, so future readers who aren't me knows exactly where to look for it. Its 'logins-list-loading-body' Done. > uber nit: capital B to start the sentence. This is a not a complete sentence; it's a clause after a semi-colon. > Please add back the bit of comment about Masterpassword from the catch block. If masterpassword has not been entered, getAllLogins() will throw. Disentangling MP related failures isn't easy so let's leave that breadcrumb for future engineers. Done. > I'd add "aboutLogins" to the debug message. debug messages I've noticed have a tendency to get lost in the noise unless they point the finger at a feature in an obvious way. It's there in the log tag: var debug = Cu.import("resource://gre/modules/AndroidLog.jsm", {}).AndroidLog.d.bind(null, "AboutLogins");

Teodora: could we get some focused testing here? You should see the spinner on first load, and see the spinner after an item delete or edit. The spinner should animate smoothly even while the UI is unresponsive.

This broke mochitest-chrome tests like https://treeherder.mozilla.org/logviewer.html#?job_id=4873907&repo=fx-team Backed out in: https://hg.mozilla.org/integration/fx-team/rev/fd551ed64296

With a fresh profile, on latest Nightly (2015-09-30), if I go to the synced tabs and tap "Get Started", the spinner is displayed and after that the creation of Firefox account. On about:logins, I don't see any spinner on first load or after deleting an item.

(In reply to Teodora Vermesan (:TeoVermesan) from comment #8) > With a fresh profile, on latest Nightly (2015-09-30), if I go to the synced > tabs and tap "Get Started", the spinner is displayed and after that the > creation of Firefox account. > On about:logins, I don't see any spinner on first load or after deleting an > item. Yeah, I got backed out. I'll re-flag after it sticks.

Comment on attachment 8666235 [details] MozReview Request: Bug 1208534 - Part 2: Fix test. r?mfinkle Bug 1208534 - Part 2: Fix test. r?mfinkle It's quite challenging to both wait for "load", and wait for something to happen in the DOM, since the DOM isn't prepared until after "load" has fired. This test therefore has a small race window: it is possible that we could wait for the mutation only after the logins have been loaded and the 'logins-list' DOM element is inserted. The logging should be good enough to identify this case; and in practice, this is very unlikely. Since I was here, I converted this to use SpawnTask.js.

Comment on attachment 8666235 [details] MozReview Request: Bug 1208534 - Part 2: Fix test. r?mfinkle https://reviewboard.mozilla.org/r/20469/#review19047

https://hg.mozilla.org/integration/fx-team/rev/01ef0856fff755c1bb2b3674382822cf1f5d2323 Bug 1208534 - Part 1: Ensure about:logins animated CSS spinner is painted before janky main-thread load. r=ally https://hg.mozilla.org/integration/fx-team/rev/49d88d6dfa50e56d03096204783a844b970335aa Bug 1208534 - Part 2: Fix test. r=mfinkle

https://hg.mozilla.org/mozilla-central/rev/01ef0856fff7 https://hg.mozilla.org/mozilla-central/rev/49d88d6dfa50

(In reply to Nick Alexander :nalexander from comment #9) > (In reply to Teodora Vermesan (:TeoVermesan) from comment #8) > > With a fresh profile, on latest Nightly (2015-09-30), if I go to the synced > > tabs and tap "Get Started", the spinner is displayed and after that the > > creation of Firefox account. > > On about:logins, I don't see any spinner on first load or after deleting an > > item. > > Yeah, I got backed out. I'll re-flag after it sticks. Teo: I've relanded, should make it into tonight's Nightly. Thanks for testing again!

Tested with latest Nightly: going to about:logins, the spinner is displayed and after that the saved logins are shown. Also, after deleting an item, the spinner is displayed: https://www.youtube.com/watch?v=BIJaVCWSe5U&feature=youtu.be

(In reply to Teodora Vermesan (:TeoVermesan) from comment #17) > Tested with latest Nightly: going to about:logins, the spinner is displayed > and after that the saved logins are shown. Also, after deleting an item, the > spinner is displayed: > https://www.youtube.com/watch?v=BIJaVCWSe5U&feature=youtu.be Thanks! The spinner after delete is intentional. (It should have done that before, but didn't for jank reasons.)

Based on comment 17 I will mark status-firefox44 to verified

Nick, do you think it's safe to uplift this? We're shipping about:logins for the first time in 42, and it may be too late to uplift there, but we could at least uplift this to 43.

(In reply to :Margaret Leibovic from comment #20) > Nick, do you think it's safe to uplift this? We're shipping about:logins for > the first time in 42, and it may be too late to uplift there, but we could > at least uplift this to 43. We can uplift all the pieces together safely. I have some uplifts in the FxA web area, so I'll prepare a patch queue. Leaving NI for that.