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

ASSIGNED
Assigned to

Status

()

Firefox
SocialAPI: Providers
ASSIGNED
6 years ago
5 years ago

People

(Reporter: alindeac, Assigned: Florian Laplantif)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox18 affected, firefox19 affected, firefox20 affected)

Details

Attachments

(1 obsolete attachment)

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
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"?
Component: SocialAPI → SocialAPI: Providers
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
Component: SocialAPI: Providers → SocialAPI
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?
OS: Windows 8 → All
Hardware: x86_64 → All
Version: 18 Branch → Trunk
(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
(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).
(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.
(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.
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
tracking-firefox18: --- → ?
tracking-firefox19: --- → ?
tracking-firefox20: --- → ?
We shouldn't be showing the notification icons if the user isn't logged in. Fixing this shouldn't be too risky for 18.
(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
(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?
tracking-firefox18: ? → +
tracking-firefox19: ? → +
tracking-firefox20: ? → ---
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
(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?
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]
(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?
(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
Created attachment 692830 [details] [diff] [review]
Assume user not logged in after 2nd cookie request
Attachment #692830 - Flags: feedback?(jaws)
Attachment #692830 - Flags: feedback?(gavin.sharp)
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|.
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.
Assignee: nobody → florianl
Status: NEW → ASSIGNED
Component: SocialAPI → SocialAPI: Providers
Attachment #692830 - Attachment is obsolete: true
Attachment #692830 - Flags: feedback?(jaws)
Attachment #692830 - Flags: feedback?(gavin.sharp)
(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.
(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.
(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.
(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)
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.
(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.
tracking-firefox18: + → ---

Updated

5 years ago
tracking-firefox19: + → ---
You need to log in before you can comment on or make changes to this bug.