in the case of 404, ssl cert failures, captive portals, etc etc we need to have the worker/sidebar/etc we need to provide the user a nice ui and a way to recover from that.

Note that in the case of the problem being found in the worker, we almost certainly want to terminate the worker immediately and consider the provider "disabled" until an explicit retry request from this UI or until next start. This is slightly tricky as we will not detect the problem until after the worker has been created (ie, it will be after the createWorker() call has returned). If the problem is in some other component (such as the sidebar or a chat window), then we have the option of leaving the worker and provider enabled but just blocking that particular action.

proposal, looking to Boriss for feedback: the problem is existing error page mechanisms (e.g. netError.xhtml) don't work for us, since we have a hidden frame and a sidebar width that is too small. screenshot to be attached as example. on cert/404/etc errors, we: - hide sidebar/recommend button/etc - unload worker - remove status icons - use a default provider icon (note blank button in screenshot) - update the user-profile section of the provider menu with a "failed to load" message, and a button or something to retry. - upon retry, we reload the worker and sidebar, rinse repeat.

(In reply to Mark Hammond (:markh) from comment #1) > we almost > certainly want to terminate the worker immediately and consider the provider > "disabled" until an explicit retry request from this UI or until next start. Sounds like there's at least four states the UI could be in after one of these failures: 1. Just needs a reload - we can do this automatically without bothering the user 2. Needs a manual "refresh" from user - we need a button prompting them to do so 3. User needs to restart - we can tell the user this but should not force them to restart if they are busy 4. Something has failed and there’s nothing we know the user could do to fix the problem Mark, do you have a sense of which of these (or another) is the most common case for hitting these errors? The important part from the user angle is if action is needed from them, such as a refresh or restart. Attached are mockups that show how we could reveal a problem scenario in three parts of the UI: A share/recommend button, a toolbar button, and a sidebar.

initial patch for handling load/cert failures in both frameworker and sidebar. does not yet handle a load failure of the provider icon (though it uses an error icon if fw or sidebar fail). This patch rides on top of a few others, so this is just for early feedback until others land. We use one single ui element for errors, the toolbar button. The share and sidebar ui components are hidden on error. I'll also attach an image of the error case.

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #4) > Created attachment 640484 [details] > Mark, do you have a sense of which of these (or another) is the most common > case for hitting these errors? The important part from the user angle is if > action is needed from them, such as a refresh or restart. I can't think of an error that would require a restart to resolve. For errors that would require a "refresh", I can think of 2 main scenarios: * A simple network failure - eg, resuming a laptop before the wireless has reconnected, or a burp by your ISP. * A captive portal or similar, where a refresh will fix it but *only* after some external action has been taken (eg, by logging into the portal via a normal window). Another class of error is SSL validation failure which will only be resolved by either the site owner fixing the cert, or the user explicitly adding an exception for the site. So I think SSL cert issues should be explicitly called out.

and I also meant to say: I don't think saying "an error has occurred" without at least offering some way to see the details of the error is particularly useful...

Comment on attachment 643672 [details] [diff] [review] errorhandling.patch + // just reuse this notification for now + Services.obs.notifyObservers(null, "social:frameworker-error", origin); Should we just formalize this and call the notification something like "social:provider-load-error/-success"? Seems better than introducing a potentially misleading name (ie, may lead people to believe it really *is* specific to the frameworker). Further, isn't there a potential race condition - the sidebar might send a "social:frameworker-error" just before the frameworker sends a "social:frameworker-loaded", in which case the error state would be lost? + broadcaster = document.getElementById("social-statusarea-errmsg"); + broadcaster.setAttribute("value", label); 'broadcaster' doesn't seem to be declared as a variable, and might not be an appropriate name anyway - the element isn't a <broadcaster> + Ci.nsIWebProgress.NOTIFY_SECURITY | + Ci.nsIWebProgress.NOTIFY_STATE_WINDOW It looks like we don't need these state flags as we don't take any action on them. + STATE_LOADING: 1, That state is never used. Also, the state appears to only be used by browser-social.js. I wonder if it isn't better to just have browser-social see the 'social:frameworker-error' notifications and track that state locally rather than having socialProvider track state it doesn't use itself - IOW, drop the observation from SocialService.jsm as it isn't actually used.) But this still leaves us with the potential race condition when one provider component fails before another component succeeds - solving that may well mean having the provider manage the state is the best option. And I guess we need some resolution to the chat with Boriss (which IIUC, is that we will not auto-close the sidebar)... In general it looks good to me, but f- mainly due to the potential race condition and apparently unnecessary state management in the provider itself.

Also, in the case of any component failing, we probably want to nuke the other components? eg, if the sidebar reports an error, we probably want to nuke the frameworker completely. So it sounds like we could drop the "loading" state and just have an "error" state that is sticky. That then implies some new method to "unstick" that error state and attempt a reload of *all* components?

(In reply to Mark Hammond (:markh) from comment #10) > Also, in the case of any component failing, we probably want to nuke the > other components? eg, if the sidebar reports an error, we probably want to > nuke the frameworker completely. > > So it sounds like we could drop the "loading" state and just have an "error" > state that is sticky. That then implies some new method to "unstick" that > error state and attempt a reload of *all* components? errors result in unload of frameworker and the reload function reloads everything.

gavin will be looking at this now

It's clear that we're going to need some more time to sort out the exact UI we want. I think we're pretty reasonably confident that the string from the mockup will be sufficient, though, so we can just add that string now in order to settle all the string landings.

This just adds all the strings that I think we'll need: message: "$brandShortName is unable to connect with $provider right now." buttons: "Try Again", "OK", and "Close this Sidebar"

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #14) > Created attachment 646718 [details] [diff] [review] > strings > > This just adds all the strings that I think we'll need: > message: "$brandShortName is unable to connect with $provider right now." > buttons: "Try Again", "OK", and "Close this Sidebar" Small string tweak - we need to capitalize "this" in "Close this Sidebar" for consistency (see "Edit This Bookmark" in star bookmark menu).

turns out I attached the wrong patch anyhow... This addresses comment 15 too.

Strings landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/770b96704cde

Implemented the error page as about:socialerror and the strings usage on that page. Still needs to make the buttons functional.

Screenshot of the socialerror page on the sidebar, based on Boriss' mockup

First stab at SocialService.reload needed for reloading everything upon an error. I still need to see if this works properly

Part 1 - about:socialerror Made some CSS tweaks since last time, implemented the buttons and some simple query string parsing code to be able to easily add more cases later. I think this part is ready for review. I added two modes, reload the provider and reload the sidebar only, with the default being reload the provider. I think the sidebar-only might not be used at the end but I'd like to keep it there to have the chance of trying it out and experimenting. If a smaller reload is fine for some cases we might want to use it rather than being extra aggressive about it.

Comment on attachment 656727 [details] [diff] [review] Patch - Part 1 - about:socialerror everything look fine, how an I test it?

(In reply to :Felipe Gomes from comment #22) > I added two modes, reload the provider and reload the sidebar only, with the > default being reload the provider. I think the sidebar-only might not be > used at the end but I'd like to keep it there to have the chance of trying > it out and experimenting. If a smaller reload is fine for some cases we > might want to use it rather than being extra aggressive about it. I'm not certain we would want to uplift that to 17 if it wont be used.

(In reply to Shane Caraveo (:mixedpuppy) from comment #23) > Comment on attachment 656727 [details] [diff] [review] > Patch - Part 1 - about:socialerror > > everything look fine, how an I test it? you can just build with this patch and open about:socialerror, or better yet use DOM inspector or something to set the sidebar URL to about:socialerror (or about:socialerror?mode=onlyRefreshSidebar)

Often times the content of the panels are cached and the scripts/main page structure loads from the cache even on a network problem. In this cases it's up to the providers to handle error displaying when they realize the XHR (or whatever method they get their content) is not working. For off-cache network failures though this patch display the proper error page for flyouts and the sidebar

Comment on attachment 664745 [details] [diff] [review] Part 3 - set error page on sidebar and flyouts Add a comment explaining why setUpProgressListener uses clientTop. There's a commented-out getcomputedStyle in there that should be removed :) Can the flyout's setUpProgressListener just be called (or inlined) in _createFrame, rather than in open()? Should onStateChange use aRequest.requestSucceeded rather than checking responseStatus "manually"? I guess you want to handle 404 differently, but maybe in that case you could check !requestSucceeded || reponseStatus == 404? What's the plan for the notification panels?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #29) > Add a comment explaining why setUpProgressListener uses clientTop. sure > There's a commented-out getcomputedStyle in there that should be removed :) oh, ok > Can the flyout's setUpProgressListener just be called (or inlined) in > _createFrame, rather than in open()? I tried this and unfortunately it only worked after the panel has been explicitly shown; it didn't work before openPopup is called. If you want I can try some more combinations to verify > > Should onStateChange use aRequest.requestSucceeded rather than checking > responseStatus "manually"? I guess you want to handle 404 differently, but > maybe in that case you could check !requestSucceeded || reponseStatus == 404? No, I can't use requestSucceeded but it's not because of 404. The problem is that requestSucceeded treat redirects (anything in the 3xx range) as non-success. And here I just want to treat client side (4xx) and server side (5xx) errors as failures. > > What's the plan for the notification panels? I'm gonna include it in a follow-up patch (maybe in this same bug). I think we can use the same UI for them, with the try again button displaying (since panels are pre-loaded on startup, if there's a transport error the user would need to click try again). We could be smarter and try to reload the notification panels after an interval, but that could potentially cause overload to a provider's server (specially if the server is already broken and failing to load the panel). so this requires more thought

Actually I just included the notifications panels as well in this patch and did some clean-up and simplifications in the code. I was also gonna include bug 796218 here but I'll do it separately for easier tracking

Assume the line > while (let frame = createdFrames.pop()) { changed to: > for (let frame of createdFrames) { as the former doesn't work and I forgot to hg qref

Just an extra patch to suppress throwing the exception of wrong origin when an error page is displayed

Comment on attachment 667593 [details] [diff] [review] Part 4 - dont try to attach mozSocial to error pages Review of attachment 667593 [details] [diff] [review]: ----------------------------------------------------------------- Can you move this check to injectController? https://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/MozSocialAPI.jsm#40

sure

Comment on attachment 666840 [details] [diff] [review] Part 3 - set error on sidebar, flyout and notification panels >diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js >+ setUpProgressListener: function SF_setUpProgressListener() { >+ if (!this._progressListenerSet) { >+ this._progressListenerSet = true; >+ // Force a layout flush by calling .clientTop so >+ // that the docShell of this frame is created >+ this.panel.firstChild.clientTop; I wonder whether you can avoid this early flush by doing this work onpopupshown (after which you're pretty much garanteed to have a docshell since the popup will have been flushed). Only concern is that that might be too late to catch the error... I guess this is fine for now. >@@ -535,27 +560,30 @@ var SocialToolbar = { >+ while (let frame = createdFrames.pop()) { >+ if (frame.docShell) { Does this check fail? If it is, then we need to prevent that somehow, don't we, rather than just failing to install the listener? > var SocialSidebar = { >+ sbrowser.webProgress.addProgressListener(new SocialErrorListener("sidebar"), >+ Ci.nsIWebProgress.NOTIFY_STATE_REQUEST | >+ Ci.nsIWebProgress.NOTIFY_LOCATION); Hrm, the social sidebar is hidden by default too, I wonder why this works. I guess this is a <xul:browser> rather than an iframe, so it's possible the XBL causes this JS references to construct frames/attach XBL, and so that makes this work? >+SocialErrorListener.prototype = { >+ QueryInterface: function (aIID) { nit: could use XPCOMUtils.generateQI... though actually, is this needed at all? >+ onStateChange: function SPL_onStateChange(aWebProgress, aRequest, aState, aStatus) { >+ // Calling cancel() will raise some OnStateChange notifications by itself, >+ // so avoid doing that more than once >+ if (failure && aStatus != Components.results.NS_BINDING_ABORTED) { Do we need to worry about cases where aStatus is NS_BINDING_ABORTED but it wasn't triggered by our Cancel()? I don't know if that's possible/likely, but we'd want to show the error UI in that case presumably - ideally we would use a custom nsresult, but that's not really possible. This is probably fine as-is though, I imagine that's an edge case. >+ onLocationChange: function SPL_onLocationChange(aWebProgress, aRequest, aLocation, aFlags) { >+ let failure = aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE; >+ if (failure) { >+ aRequest.cancel(Components.results.NS_BINDING_ABORTED); Do we really need to cancel() this case? What is still loading here, the error page itself? I guess that's fine. >+ setErrorMessage: function(aWebProgress) { >+ case "notification-panel": >+ SocialToolbar.setPanelErrorMessage(aWebProgress.chromeEventHandler); How does this work? I don't see a chromeEventHandler attribute on nsIWebProgress, which is the only thing you're garanteed to get from onStateChange. And nsIWebProgress seems to only be implemented by nsDocLoader, which doesn't implement anything with a chromeEventHandler getter AFAICT. r- for this last issue, but let me know if I'm missing something!

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #36) > >+ setErrorMessage: function(aWebProgress) { > > >+ case "notification-panel": > >+ SocialToolbar.setPanelErrorMessage(aWebProgress.chromeEventHandler); > > How does this work? I don't see a chromeEventHandler attribute on > nsIWebProgress, which is the only thing you're garanteed to get from > onStateChange. And nsIWebProgress seems to only be implemented by > nsDocLoader, which doesn't implement anything with a chromeEventHandler > getter AFAICT. > > r- for this last issue, but let me know if I'm missing something! The aWebProgress received in these progress listeners are actually nsDocShell objects (which implements nsDocLoader and hence nsIWebProgress). So yeah that's how it work. Do you want me to QI it to make that explicit? > >+ setUpProgressListener: function SF_setUpProgressListener() { > >+ if (!this._progressListenerSet) { > >+ this._progressListenerSet = true; > >+ // Force a layout flush by calling .clientTop so > >+ // that the docShell of this frame is created > >+ this.panel.firstChild.clientTop; > > I wonder whether you can avoid this early flush by doing this work > onpopupshown (after which you're pretty much garanteed to have a docshell > since the popup will have been flushed). Only concern is that that might be > too late to catch the error... I guess this is fine for now. I will experiment with that soon but I thought the same, it might be too late to attach it > > >@@ -535,27 +560,30 @@ var SocialToolbar = { > > >+ while (let frame = createdFrames.pop()) { > >+ if (frame.docShell) { > > Does this check fail? If it is, then we need to prevent that somehow, don't > we, rather than just failing to install the listener? I never saw it failing but I just added it there for safety. If it happens, failing to attach the listener seemed better than throwing an exception and then failing to finish initialization (as this function is called from SocialToolbar.init). Since I never saw it happening I didn't add any handling for that case. > > > var SocialSidebar = { > > >+ sbrowser.webProgress.addProgressListener(new SocialErrorListener("sidebar"), > >+ Ci.nsIWebProgress.NOTIFY_STATE_REQUEST | > >+ Ci.nsIWebProgress.NOTIFY_LOCATION); > > Hrm, the social sidebar is hidden by default too, I wonder why this works. I > guess this is a <xul:browser> rather than an iframe, so it's possible the > XBL causes this JS references to construct frames/attach XBL, and so that > makes this work? Yeah I'm pretty sure xul:browsers have something in their constructor that causes everything to attach early. tabbrowser.xml even creates xul:browsers and attach the listeners in the same function (no executeSoon to wait for the event loop or anything like that) > > >+SocialErrorListener.prototype = { > >+ QueryInterface: function (aIID) { > > nit: could use XPCOMUtils.generateQI... though actually, is this needed at > all? Yeah it's needed, AddProgressListener doesn't accept the object if nsISupportsWeakRef is not declared: http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsDocLoader.cpp#962 I'll switch to use generateQI > > >+ onStateChange: function SPL_onStateChange(aWebProgress, aRequest, aState, aStatus) { > > >+ // Calling cancel() will raise some OnStateChange notifications by itself, > >+ // so avoid doing that more than once > >+ if (failure && aStatus != Components.results.NS_BINDING_ABORTED) { > > Do we need to worry about cases where aStatus is NS_BINDING_ABORTED but it > wasn't triggered by our Cancel()? I don't know if that's possible/likely, > but we'd want to show the error UI in that case presumably - ideally we > would use a custom nsresult, but that's not really possible. This is > probably fine as-is though, I imagine that's an edge case. Yeah I don't think it's a big concern for now, more of an edge case > > >+ onLocationChange: function SPL_onLocationChange(aWebProgress, aRequest, aLocation, aFlags) { > >+ let failure = aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE; > >+ if (failure) { > >+ aRequest.cancel(Components.results.NS_BINDING_ABORTED); > > Do we really need to cancel() this case? What is still loading here, the > error page itself? I guess that's fine. > IIRC it helped a bit to avoid some more OnStateChange messages being thrown for the old page before switching to the new one (specially since this case requires the setTimeout).

Jared, can you take a look at the latest changes I made to this patch? Gavin said that the only issue with the previous one was the question about aWebProgress.chromeEventHandler which I've made the QI explicit in this patch. Also changed to use XPCOMUtils.generateQI. This is an interdiff to make it simpler: http://pastebin.mozilla.org/1858375

Comment on attachment 668642 [details] [diff] [review] Patch r+ on the changes shown in the interdiff.

Gavin, in order to land this quickly, as you mentioned that the r- was only for the last issue, I've landed the patch with a fix for that as reviewed by Jared above. If you think that was not enough to cover your comment I can back it out or follow-up. https://hg.mozilla.org/integration/mozilla-inbound/rev/6a8bb5834090 https://hg.mozilla.org/integration/mozilla-inbound/rev/0ecee5121681

Roll-up patch for aurora with all the patches from this bug together. It also includes the patches for bug 798437 and bug 796218, which are each one-liners that should land together with this bug, so it makes it simpler to do it all together. [Approval Request Comment] Bug caused by (feature/regressing bug #): new feature User impact if declined: No specific error pages for the social sidebar and flyouts Testing completed (on m-c, etc.): ran through tryserver and just landed on inbound Risk to taking this patch (and alternatives if risky): small risk on social, unlikely to break something outside of social String or UUID changes made by this patch: none

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9b9678df7672 because something in the push was hitting "browser_frameworker.js | sub-test testEarlyClose failed: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.nukeSandbox]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: resource://gre/modules/FrameWorker.jsm :: terminate :: line 239" data: no]" and "browser_frameworker.js | check that websockets worked - Got FAILED calling WebSocket constructor: TypeError: WebSocket is not a constructor, expected ok" and I didn't know which (though I presume bug 790201), and didn't know interconnectedness among them.

relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e2479870b9

relanded other part: https://hg.mozilla.org/integration/mozilla-inbound/rev/04870d2657db

https://hg.mozilla.org/mozilla-central/rev/d8e2479870b9 https://hg.mozilla.org/mozilla-central/rev/04870d2657db

Comment on attachment 668709 [details] [diff] [review] Roll-up for aurora >diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js >+SocialErrorListener.prototype = { >+ QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener, >+ Ci.nsISupportsWeakReference, >+ Ci.nsISupports]), nit: don't need to specify nsISupports for generateQI

This is FIXED now, right?

https://hg.mozilla.org/releases/mozilla-aurora/rev/b69ca3697450 (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #47) > This is FIXED now, right? yeah. also removed nsISupports from generateQI on the patch for aurora.

Note that the motown icon won't display if the browser is not 'online', causing an empty toolbarbutton.

What are the conditions which might cause this UI to be triggered so I can verify fixed?

- starting Firefox up with social enabled and no internet connection (maybe after clearing your cache) - when behind a captive portal (wifi access point that redirects requests to the provider) - when the provider is down and is returning HTTP errors (e.g. 404 or 500)

The "no internet connection" scenario is easy to test but the others are not so easy I think. Could we fake this through the social.manifest.<provider> pref? For example, if I used a URL which had blocking permissions set (500) or a URL to a non-existent provider? I could probably do this with Shane's local demo provider.

Anthony: yeah setting the e.g. sidebarURL property in the manifest to a non-existing URL should work

Firefox 17.0 beta 5 - Build ID 20121106195758 Firefox couldn't connect to Facebook Messenger all day long so we had the chance to test this fix on multiple OSs. Most of the times, the about:socialerror page is displayed as described in the above comments and the buttons on it seem to work fine. I did run into an intermittent problem though: after clicking Try Again a few times (3 or more), without waiting for more than 1-2 seconds between the clicks, the sidebar gets empty. - Please see my attachment

(In reply to Ioana Budnar [QA] from comment #54) > Created attachment 679118 [details] > Empty SocialAPI sidebar > > Firefox 17.0 beta 5 - Build ID 20121106195758 > > Firefox couldn't connect to Facebook Messenger all day long so we had the > chance to test this fix on multiple OSs. Most of the times, the > about:socialerror page is displayed as described in the above comments and > the buttons on it seem to work fine. > > I did run into an intermittent problem though: after clicking Try Again a > few times (3 or more), without waiting for more than 1-2 seconds between the > clicks, the sidebar gets empty. - Please see my attachment Thanks for testing this out Ioana. I see in the bottom of the attachment there is part of the Facebook content, so it appears that occasional loads of the Facebook sidebar partially succeeded.

Firefox 17.0 beta 5 - Build ID 20121106195758 The fix is not working right in other cases either: 1. Enable the Social sidebar in Firefox and log into Facebook. 2. Close Firefox. Close your internet connection. 3. Restart Firefox. 4. Click "Try again" on the sidebar. Click it again. (Leave the internet connection closed) The "Server not found" error page is displayed in the sidebar. The sidebar should have preserved its UI until the error is gone.

(In reply to :Felipe Gomes from comment #53) > Anthony: yeah setting the e.g. sidebarURL property in the manifest to a > non-existing URL should work When I changed the "sidebarURL" link in social.manifest.facebook, the sidebar displayed the right UI (unable to connect message with "try again" and "close this sidebar" buttons). When I changed the "origin" link in social.manifest.facebook, the sidebar displayed the loading animation all the time. An error message should be displayed in this case again - if not from the beginning at least after trying to load for 1-2 minutes. In both cases, I changed the link with one for a non-existent page, all other links but the one changed had their default value, and I restarted Firefox after the change. Firefox 17.0 beta 5 - Build ID 20121106195758

Felipe, would you prefer me to reopen this bug and fix the issues in comments 54, 56 and 57, or mark this bug as verified and log new smaller ones for those issues?

(In reply to Ioana Budnar [QA] from comment #58) > Felipe, > would you prefer me to reopen this bug and fix the issues in comments 54, 56 > and 57, This sounds a bit fishy: I wan only offering to reopen the bug, you'd have to fix it...

Thanks for looking into this - please file a new bug for new issues. Comment 56 sounds like a problem. Comment 57 sounds like a side effect of the testing method - changing the origin URI is not a valid test for this patch, and would cause the failure you describe for other reasons (origin URI not matching sidebar URI).

Marking this bug as verified per the above comments. I will most probably only file the follow up bugs tomorrow, but I will add this bug as a dependency so anyone interested can get to them easily.