1197323 - Avatar does not show (but shows in the preferences)

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect, P1)

Description

[Sylvestre Ledru [:Sylvestre]]

Attachment: sc.png

I don't have the STR but on my firefox aurora (42 currently), the avatar shows in about preferences but not in the menu.

Removing the "Partager cette page" (share this page) item in the custom menu triggered a refresh of my avatar and it works

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: After the custom (OK behavior)

Comment 2

[Chris Karlof [:ckarlof]]

eoger, could take a look at this?

Comment 3

[Edwin Wong [:edwong]]

:zaach can repro - investigating a fix.

Comment 4

[Zachary Carter [:zaach]]

Attachment: prevent out-of-order resolution of profile info and avatar display

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8663948 [details] [diff] [review]
prevent out-of-order resolution of profile info and avatar display

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

I'm afraid I don't understand what this is fixing, and the test doesn't help me understand. Can you please explain what exactly is being fixed and see if we can make a test that accurately reflects it (or explain to me why the test change does, as I don't see it)

Happy to chat when I come online tomorrow if you prefer.

::: browser/components/preferences/in-content/sync.js
@@ +398,2 @@
>              let img = new Image();
> +            img.onerror = () => {

I don't understand why these changes are needed.

::: services/fxaccounts/FxAccountsProfile.jsm
@@ +118,5 @@
> +      this._cacheFetchPromise = Promise.resolve(this._cachedProfile)
> +        .then(profile => {
> +          this._cacheFetchPromise = null;
> +          return profile;
> +        }, err => {

I don't see how we could ever get here - doesn't that imply Promise.resolve() could return a rejected promise, which I don't think it ever would.

::: services/fxaccounts/tests/xpcshell/test_profile.js
@@ +108,5 @@
>  });
>  
> +add_test(function getCachedProfile_returns_same_promise_until_resolved() {
> +  let profile = CreateFxAccountsProfile();
> +  // a little pointless until bug 1157529 is fixed...

note this bug now *is* fixed :)

@@ +113,5 @@
> +  profile._cachedProfile = { avatar: "myurl" };
> +
> +  let promise1 = profile._getCachedProfile();
> +  let promise2 = profile._getCachedProfile();
> +  do_check_true(promise1 === promise2);

If I add this test but none of the other changes, this line is the only one that fails - and I don't see how them being an identical promise is important

Comment 6

[Zachary Carter [:zaach]]

(In reply to Mark Hammond [:markh] from comment #5)
> Comment on attachment 8663948 [details] [diff] [review]
> prevent out-of-order resolution of profile info and avatar display
> 
> Review of attachment 8663948 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm afraid I don't understand what this is fixing, and the test doesn't help
> me understand. Can you please explain what exactly is being fixed and see if
> we can make a test that accurately reflects it (or explain to me why the
> test change does, as I don't see it)

Sure, I'll add some context.

> 
> Happy to chat when I come online tomorrow if you prefer.
> 
> ::: browser/components/preferences/in-content/sync.js
> @@ +398,2 @@
> >              let img = new Image();
> > +            img.onerror = () => {
> 
> I don't understand why these changes are needed.

There's a concurrency issue here that was causing the bug I observed. STR:

1. Have an account with a profile image.
2. Go to accounts.firefox.com/settings and remove the profile image.
3. If you're unlucky, observe that the image is still visible in the hamburger menu or pref page, or both.

The issue is caused by us updating this UI in quick succession: first with whatever was in the profile cache and then again after we fetch the latest profile data in the background and find that it has changed. If the cached profile has an image, as in the STR, the img.onload callback will fire but possibly *after* we've displayed the latest profile. These changes make it so that we set the image src right away, preventing it from being set out of order.


> 
> ::: services/fxaccounts/FxAccountsProfile.jsm
> @@ +118,5 @@
> > +      this._cacheFetchPromise = Promise.resolve(this._cachedProfile)
> > +        .then(profile => {
> > +          this._cacheFetchPromise = null;
> > +          return profile;
> > +        }, err => {
> 
> I don't see how we could ever get here - doesn't that imply
> Promise.resolve() could return a rejected promise, which I don't think it
> ever would.
> 
> ::: services/fxaccounts/tests/xpcshell/test_profile.js
> @@ +108,5 @@
> >  });
> >  
> > +add_test(function getCachedProfile_returns_same_promise_until_resolved() {
> > +  let profile = CreateFxAccountsProfile();
> > +  // a little pointless until bug 1157529 is fixed...
> 
> note this bug now *is* fixed :)
> 
> @@ +113,5 @@
> > +  profile._cachedProfile = { avatar: "myurl" };
> > +
> > +  let promise1 = profile._getCachedProfile();
> > +  let promise2 = profile._getCachedProfile();
> > +  do_check_true(promise1 === promise2);
> 
> If I add this test but none of the other changes, this line is the only one
> that fails - and I don't see how them being an identical promise is important

This was to help give assurance that multiple calls to getProfile would resolve in serial. This might already be the case with how promises are scheduled, though...

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to Zachary Carter [:zaach] from comment #6)
> There's a concurrency issue here that was causing the bug I observed. STR:
> 
> 1. Have an account with a profile image.
> 2. Go to accounts.firefox.com/settings and remove the profile image.
> 3. If you're unlucky, observe that the image is still visible in the
> hamburger menu or pref page, or both.
> 
> The issue is caused by us updating this UI in quick succession: first with
> whatever was in the profile cache and then again after we fetch the latest
> profile data in the background and find that it has changed. If the cached
> profile has an image, as in the STR, the img.onload callback will fire but
> possibly *after* we've displayed the latest profile. These changes make it
> so that we set the image src right away, preventing it from being set out of
> order.

OK, that makes sense, thanks. I'm a little worried that this might end up causing a leak though - we expect the onload to fire in the normal case, but not onerror, so I'm worried the image will stay alive (eg, both the current handler and the new onerror handler have a closure over the outer scope where "img" is defined - I'm not sure how to verify that this is or isn't a problem though. And I'm not sure this is actually "more leaky" than the existing code would be - onload is probably just as leaky as onerror - but at least we could probably fix the onload version by removing the event handler once it fires.)

In the case of an error, what would we see if we removed that onerror and onload code?  It would seem even better to remove that complexity completely if that's possible.  (IIUC, the original motivation was to ensure we show nothing until we are certain it can be loaded, but that doesn't seem as relevant in this onerror style)

> This was to help give assurance that multiple calls to getProfile would
> resolve in serial. This might already be the case with how promises are
> scheduled, though...

Yeah, I don't think that is necessary.

Would it be possible for you to experiment with removing those handlers completely, and if that looks really bad, re-submit the patch with just those onerror changes?

Thanks

Comment 8

[Zachary Carter [:zaach]]

Attachment: Screen Shot 2015-09-24 at 11.28.15 AM.png

Here's Sync prefs with a failed avatar. The space for the avatar flattens.

Comment 9

[Zachary Carter [:zaach]]

Attachment: Screen Shot 2015-09-24 at 11.40.32 AM.png

In the hamburger menu the space appears blank.

Comment 10

[Zachary Carter [:zaach]]

(In reply to Mark Hammond [:markh] from comment #7)
> I'm a little worried that this might end up
> causing a leak though - we expect the onload to fire in the normal case, but
> not onerror, so I'm worried the image will stay alive (eg, both the current
> handler and the new onerror handler have a closure over the outer scope
> where "img" is defined - I'm not sure how to verify that this is or isn't a
> problem though. And I'm not sure this is actually "more leaky" than the
> existing code would be - onload is probably just as leaky as onerror - but
> at least we could probably fix the onload version by removing the event
> handler once it fires.)

I suppose we could have an onload handler that removes onerror as well as itself.


> Would it be possible for you to experiment with removing those handlers
> completely, and if that looks really bad, re-submit the patch with just
> those onerror changes?

Ryan: Should we show the default image when the avatar fails to load or leave it blank, as in the screenshots I've attached above?

Comment 11

[Ryan Feeley [:rfeeley]]

Default image definitely preferable.

Comment 12

[Zachary Carter [:zaach]]

Attachment: prevent profile images from being overwritten by an async onload callback

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8665676 [details] [diff] [review]
prevent profile images from being overwritten by an async onload callback

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

Thanks Zach - as discussed in our meeting, I couldn't demonstrate any problem leaving the onerror handler in place, so I think we should just remove the new onload handlers in your patch and leave the onerror handlers as they are in the patch.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5091c2699839

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5091c2699839

Comment 16

[Mihai Boldan, Desktop QA [:mboldan]]

I was not able to reproduce this issue following the STR from Comment 6 on Windows 10 x86, Ubuntu 14.04 x64 and Mac OS X 10.10.5 using Firefox 42.0a2 (2015-08-21).
I've also tested on Firefox 46.0a1 (2016-01-14), Firefox 45.0a2 (2016-01-14) and Firefox 44.0b9 and I can confirm that the avatar is correctly updated in both hamburger menu and pref page.
Are there any other STR in order to reproduce this issue, or is there a specific OS where is reproducible?