about:logins animated CSS spinner does not appear while loading large logins stores

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

Trunk
Firefox 44
Points:
---

Firefox Tracking Flags

(firefox44 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
Created attachment 8666235 [details]
MozReview Request: Bug 1208534 - Part 2: Fix test. r?mfinkle

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.
Attachment #8666235 - Flags: review?(ally)
Attachment #8666235 - Flags: review?(ally) → review+
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
Flags: needinfo?(nalexander)
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.
Flags: needinfo?(nalexander)
Duplicate of this bug: 1203720
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.
Attachment #8666235 - Attachment description: MozReview Request: Bug 1208534 - Ensure about:logins animated CSS spinner is painted before janky main-thread load. r?ally → MozReview Request: Bug 1208534 - Part 2: Fix test. r?mfinkle
Attachment #8666235 - Flags: review?(mark.finkle)
Comment on attachment 8666235 [details]
MozReview Request: Bug 1208534 - Part 2: Fix test. r?mfinkle

https://reviewboard.mozilla.org/r/20469/#review19047
Attachment #8666235 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/01ef0856fff7
https://hg.mozilla.org/mozilla-central/rev/49d88d6dfa50
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
(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!
Flags: needinfo?(teodora.vermesan)
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
Flags: needinfo?(teodora.vermesan)
(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
status-firefox44: fixed → verified

Comment 20

3 years ago
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.
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(nalexander)
You need to log in before you can comment on or make changes to this bug.