821323 - The panels are empty if the logging in is not kept and Firefox is closed

Status: RESOLVED WONTFIX

(Firefox Graveyard :: SocialAPI: Providers, defect)

Description

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0
Build ID: 20121212073002

Steps to reproduce:
1. Launch Firefox and enable Social API
2. Log into your Facebook account without checking the box "Keep me logged in"
3. Log out from Facebook and close Firefox
4. Start Firefox again and log into your Facebook account
5. Close Firefox
6. Start Firefox again and click on the Social API buttons from the Navigation Toolbar
7. In the Social API sidebar -> click on the Log In button
8. In the Navigation Toolbar, click on the Friends, Messages and Notifications buttons.

Expected results:
The Friends, Messages and Notifications panels are opened and their content is displayed.

Actual results:
The panels are opened but are empty. 

Please see the screencast with the STR: 
http://screencast.com/t/L0njUhALQmJ

Comment 1

[u279076]

This looks like a connectivity issue with the provider. Can you please check to see if there are any errors in Error Console mentioning "facebook"?

Comment 2

[Shane Caraveo (:mixedpuppy)]

I can reproduce this problem, I don't think it is a facebook issue.  This seems to be a problem with our cacheing of the status icons then allowing the panels to be opened prior to login.  

1. we shouldn't allow the panels to open without login, we need to figure something out here
2. we should ensure we reset the panels after login

Comment 3

[u279076]

Yeah, it's pretty trivial to reproduce. The workaround seems to be turning on/off the Facebook Messenger from the Tools menu.

Shane, how trivial do you think this would be to fix? Can we get a fix in time for Firefox 18?

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #3)
> Yeah, it's pretty trivial to reproduce. The workaround seems to be turning
> on/off the Facebook Messenger from the Tools menu.

avoiding the problem is trivial, making good ux out of our current situation may not be.

> Shane, how trivial do you think this would be to fix? Can we get a fix in
> time for Firefox 18?

no

Comment 5

[u279076]

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> avoiding the problem is trivial

Can you please elaborate? The only way I can see to avoid this situation is by selecting "Keep me signed in" when logging in. While this action might seem trivial to some, it might not be reasonable to others (some people don't like having keep-alive sessions for privacy reasons).

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #5)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> > avoiding the problem is trivial

I think you need the full quote: "avoiding the problem is trivial, making good ux out of our current situation may not be."

We can prevent showing the panels if the user is not logged in, for those providers that have user login, but that is terrible ux (buttons that do nothing).  The question becomes what is the right ux in this situation.  IMO we should remove the icons if the user is not logged in.

> Can you please elaborate? The only way I can see to avoid this situation is
> by selecting "Keep me signed in" when logging in. While this action might
> seem trivial to some, it might not be reasonable to others (some people
> don't like having keep-alive sessions for privacy reasons).

I wasn't suggesting a solution from a user-perspective.

I dont recall reports of this issue before, so I don't feel it is critical enough to push on a fix for 18.  For 19 perhaps.

Comment 7

[u279076]

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> IMO we should remove the icons if the user is not logged in.

Yes, I agree that this would be the ideal UX.

> I dont recall reports of this issue before, so I don't feel 
> it is critical enough to push on a fix for 18.  For 19 perhaps.

My concern is that as Facebook continues to ramp up ADIs we'll be more prone to visibility of these issues. Poor UX, whether or not it's our fault, will reflect poorly on Firefox.

I'd much prefer getting this fixed as soon as we can. I understand if it can't be done in time for Firefox 18 given other priorities and the fact that this change likely isn't low-risk. If we can't push for 18, I'd be strongly in favour of 19.

I'll nom this for tracking all branches so we can get RelMan eyes on this. Note that I fully expect this to be wontfix for 18, possible for 19, and likely for 20.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

We shouldn't be showing the notification icons if the user isn't logged in. Fixing this shouldn't be too risky for 18.

Comment 9

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> I can reproduce this problem, I don't think it is a facebook issue.  This
> seems to be a problem with our cacheing of the status icons then allowing
> the panels to be opened prior to login.  

So what *should* happen is that the panels *are* allowed to open in the brief time between FF starting and the provider responding with profile information.  In this case, the panel would previously show a "not logged in" message, which isn't ideal but isn't horrible either.

But as soon as the provider supplied profile information - either with a logged-in user, or indicating no user is logged in, the panels should adjust correctly.  So the above should only be for a very short time before the provider initializes.

> 1. we shouldn't allow the panels to open without login, we need to figure
> something out here

As above, as soon as the provider tells us no user is logged in, the panels *should* be removed.

> 2. we should ensure we reset the panels after login

Ditto - if the provider tells us there is a logged in user, all should be sweet.

I suspect a regression in browser-social handling these states - the logic for this is hairy, non-obvious and probably doesn't have good test coverage

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jared Wein [:jaws] from comment #8)
> We shouldn't be showing the notification icons if the user isn't logged in.

This would break our theoretical support for providers that don't use the "Signed in" state, right? Or at least, break the use of the icon cache for them.

(In reply to Mark Hammond (:markh) from comment #9)
> So what *should* happen is that the panels *are* allowed to open in the
> brief time between FF starting and the provider responding with profile
> information.  In this case, the panel would previously show a "not logged
> in" message, which isn't ideal but isn't horrible either.

Sounds like what's happening is that FB's panel content doesn't handle this case, so it shows a blank panel instead?

Comment 11

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #10)
> (In reply to Mark Hammond (:markh) from comment #9)
> > So what *should* happen is that the panels *are* allowed to open in the
> > brief time between FF starting and the provider responding with profile
> > information.  In this case, the panel would previously show a "not logged
> > in" message, which isn't ideal but isn't horrible either.
> 
> Sounds like what's happening is that FB's panel content doesn't handle this
> case, so it shows a blank panel instead?

Possibly, although it's not clear to me that facebook hasn't fully initialized when this can be reproduced.

Another alternative is that facebook is never calling setProfile with null when not logged in - there was an old bug on file where they indicated they *would* do that once the "whitelist" was removed, but maybe that's not actually happening.

Comment 12

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

(In reply to Mark Hammond (:markh) from comment #11)
> Another alternative is that facebook is never calling setProfile with null
> when not logged in - there was an old bug on file where they indicated they
> *would* do that once the "whitelist" was removed, but maybe that's not
> actually happening.

I just verified this is the case.  The toolbar notification cache relies on the provider giving us an empty profile at startup if not logged in.

What shall we do this this?  Just move it to Social:Providers?

Comment 13

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

FWIW, if we felt this was somewhat urgent, we probably could work around it on our side.  Facebook always does a cookies-get message - if we are logged in, we immediately get a profile.  It then repeatedly (once per second or so) re-requests the cookies.

Thus, on the second cookies-get message, if the profile is still undefined we can infer the user is logged out.  It would be an ugly short-term hack, but probably an effective one.

[Personally I'm not sure it is urgent enough, but I'm throwing this out there anyway]

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mark Hammond (:markh) from comment #12)
> (In reply to Mark Hammond (:markh) from comment #11)
> > Another alternative is that facebook is never calling setProfile with null
> > when not logged in - there was an old bug on file where they indicated they
> > *would* do that once the "whitelist" was removed, but maybe that's not
> > actually happening.
> 
> I just verified this is the case.  The toolbar notification cache relies on
> the provider giving us an empty profile at startup if not logged in.

Oh okay, I thought we knew the user wasn't logged in but were keeping them around.

(In reply to Mark Hammond (:markh) from comment #13)
> FWIW, if we felt this was somewhat urgent, we probably could work around it
> on our side.  Facebook always does a cookies-get message - if we are logged
> in, we immediately get a profile.  It then repeatedly (once per second or
> so) re-requests the cookies.
> 
> Thus, on the second cookies-get message, if the profile is still undefined
> we can infer the user is logged out.  It would be an ugly short-term hack,
> but probably an effective one.

Maybe we can do this for 18 and 19?

Comment 15

[Simona Badau, Desktop QA]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #1)
> This looks like a connectivity issue with the provider. Can you please check
> to see if there are any errors in Error Console mentioning "facebook"?

I get the following errors in the Error Console (I followed the STR from the Description and I cleared the error console before clicking on the Social API buttons in step 6).
 
Timestamp: 12/14/2012 11:35:33 AM
Error: WorkerAPI: topic doesn't have a handler: 'worker.connected'
Source File: resource://gre/modules/WorkerAPI.jsm
Line: 41

Timestamp: 12/14/2012 11:35:34 AM
Error: Firefox can't establish a connection to the server at wss://orcart.facebook.com/websocket.
Source File: https://www.facebook.com/desktop/fbdesktop2/socialfox/fbworker.js.php
Line: 184

Timestamp: 12/14/2012 11:35:35 AM
Error: The connection to wss://orcart.facebook.com/websocket was interrupted while the page was loading.
Source File: https://www.facebook.com/desktop/fbdesktop2/socialfox/fbworker.js.php
Line: 184

Comment 16

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

Attachment: Assume user not logged in after 2nd cookie request

Comment 17

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

oops - fat-fingers!  Asking for feedback as I'm still not convinced we should take this route.  The patch does fix the problem, but at least 1 second *after* the problem would be solved if the provider did the right things.  Also note the patch to SocialService probably should be taken regardless - otherwise the provider must send a profile of |{}| rather than |null|.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Florian said on Friday that they could fix this on the Facebook side, so I don't think we need to make any changes to our API here.

Comment 19

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] from comment #18)
> Florian said on Friday that they could fix this on the Facebook side, so I
> don't think we need to make any changes to our API here.

That is good for a quick resolution, but we still need to handle this issue on our side, and watching cookies is not a general solution.  We need to fix the way we deal with caching the button and panels.

Comment 20

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #19)
> That is good for a quick resolution, but we still need to handle this issue
> on our side, and watching cookies is not a general solution.  We need to fix
> the way we deal with caching the button and panels.

I'm not sure what you mean here.  Can't we just declare that a provider not calling setProfile(null) on startup when the user isn't logged in is misbehaving?

As far as I can tell, the only other fix is to drop the cache.

Comment 21

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #20)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #19)
> > That is good for a quick resolution, but we still need to handle this issue
> > on our side, and watching cookies is not a general solution.  We need to fix
> > the way we deal with caching the button and panels.
> 
> I'm not sure what you mean here.  Can't we just declare that a provider not
> calling setProfile(null) on startup when the user isn't logged in is
> misbehaving?
> 
> As far as I can tell, the only other fix is to drop the cache.

or disable the buttons like other buttons in the ui, if something cant work, it is greyed out.

Comment 22

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> or disable the buttons like other buttons in the ui, if something cant work,
> it is greyed out.

There are 2 parts to this.  The main issue with this bug is that the buttons remain well after the provider knows we are logged out.  If we disable the buttons we would still end up with the buttons in the toolbar far longer than we want - we actually want the buttons removed in that case.

Assuming the above works correctly there should only be a very minimal time between the buttons appearing in the toolbar and not working correctly.  Making them disabled would solve that part - but I suspect this bug would not have been opened if things above worked correctly.  Disabling the buttons for this short period might be worthwhile, but we should consider if that kinda defeats the purpose of the cache (which was to make things *look* initialized early)

Comment 23

[Alex Keybl [:akeybl]]

We're getting very close to the release here. Do we actually expect to resolve in time for FF18? Beta 2 is already building, so the next opportunity to land would be beta 3.

Comment 24

[Shane Caraveo (:mixedpuppy)]

(In reply to Alex Keybl [:akeybl] from comment #23)
> We're getting very close to the release here. Do we actually expect to
> resolve in time for FF18? Beta 2 is already building, so the next
> opportunity to land would be beta 3.

I think there is too much going on right now, along with people on PTO, so I'm removing tracking for 18.

Comment 25

[BMO Automation]

SocialAPI was removed from Firefox 57 and is no longer available in any current release.