Note: There are a few cases of duplicates in user autocompletion which are being worked on.

proper ui on transport failures

VERIFIED FIXED

Status

()

Firefox
SocialAPI
--
blocker
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: mixedpuppy, Assigned: Felipe)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17+ verified)

Details

(Whiteboard: [Fx17])

Attachments

(12 attachments, 7 obsolete attachments)

1.02 MB, image/png
Details
25.23 KB, image/png
Details
1.31 KB, patch
mixedpuppy
: review+
jaws
: checkin+
Details | Diff | Splinter Review
19.24 KB, patch
Details | Diff | Splinter Review
227.83 KB, image/png
Details
3.87 KB, patch
Details | Diff | Splinter Review
20.14 KB, patch
mixedpuppy
: review+
Details | Diff | Splinter Review
1.09 KB, patch
jaws
: review+
Details | Diff | Splinter Review
12.90 KB, patch
jaws
: review+
Details | Diff | Splinter Review
31.43 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
139.94 KB, image/png
Details
177.31 KB, image/png
Details
(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 733414
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.
Severity: normal → blocker
OS: Mac OS X → All
Hardware: x86 → All

Updated

5 years ago
Blocks: 766607
(Reporter)

Updated

5 years ago
Whiteboard: [ms1]

Updated

5 years ago
Whiteboard: [ms1] → [Fx16]
(Reporter)

Updated

5 years ago
Assignee: nobody → mixedpuppy
(Reporter)

Comment 2

5 years ago
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.
(Reporter)

Comment 3

5 years ago
Created attachment 638855 [details]
load error in sidebar
Created attachment 640484 [details]
Mockup: How we could reveal a problem scenario in three parts of a social interface: A share/recommend button, a toolbar button, and a sidebar

(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.
(Reporter)

Updated

5 years ago
Depends on: 775336
(Reporter)

Comment 5

5 years ago
Created attachment 643672 [details] [diff] [review]
errorhandling.patch

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.
Attachment #643672 - Flags: feedback?(mhammond)
Attachment #643672 - Flags: feedback?(gavin.sharp)
(Reporter)

Comment 6

5 years ago
Created attachment 643674 [details]
errormenu.png
(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.
Attachment #643672 - Flags: feedback?(mhammond) → feedback-
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?
(Reporter)

Comment 11

5 years ago
(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.
Whiteboard: [Fx16] → [Fx16][strings:1?]
(Reporter)

Comment 12

5 years ago
gavin will be looking at this now
Assignee: mixedpuppy → gavin.sharp
Attachment #643672 - Flags: feedback?(gavin.sharp)
Created attachment 645601 [details] [diff] [review]
just add the strings

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.
Attachment #638855 - Attachment is obsolete: true
Attachment #643672 - Attachment is obsolete: true
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"
Attachment #645601 - Attachment is obsolete: true
Attachment #646718 - Flags: review?(mixedpuppy)
(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).
Created attachment 647072 [details] [diff] [review]
strings

turns out I attached the wrong patch anyhow... This addresses comment 15 too.
Attachment #646718 - Attachment is obsolete: true
Attachment #646718 - Flags: review?(mixedpuppy)
Attachment #647072 - Flags: review?(mixedpuppy)
(Reporter)

Updated

5 years ago
Attachment #647072 - Flags: review?(mixedpuppy) → review+
(Reporter)

Updated

5 years ago
Whiteboard: [Fx16][strings:1?] → [Fx17][strings:1?]
(Assignee)

Updated

5 years ago
Assignee: gavin.sharp → felipc
Status: NEW → ASSIGNED
(Assignee)

Comment 17

5 years ago
Strings landed in
https://hg.mozilla.org/integration/mozilla-inbound/rev/770b96704cde
Whiteboard: [Fx17][strings:1?] → [Fx17][strings:1?][leave open]
https://hg.mozilla.org/mozilla-central/rev/770b96704cde
(Assignee)

Comment 19

5 years ago
Created attachment 655908 [details] [diff] [review]
WIP - error page look and feel

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

Comment 20

5 years ago
Created attachment 655909 [details]
Screenshot

Screenshot of the socialerror page on the sidebar, based on Boriss' mockup
tracking-firefox17: --- → +
(Assignee)

Comment 21

5 years ago
Created attachment 656702 [details] [diff] [review]
Part 2 - SocialService.reload (v1)

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

Comment 22

5 years ago
Created attachment 656727 [details] [diff] [review]
Patch - Part 1 - about:socialerror

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.
Attachment #656727 - Flags: review?(mixedpuppy)
(Reporter)

Comment 23

5 years ago
Comment on attachment 656727 [details] [diff] [review]
Patch - Part 1 - about:socialerror

everything look fine, how an I test it?
Attachment #656727 - Flags: review?(mixedpuppy) → review+
(Reporter)

Comment 24

5 years ago
(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.
(Assignee)

Comment 25

5 years ago
(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)
(Assignee)

Updated

5 years ago
Depends on: 788920
(Assignee)

Comment 26

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d0471602053
https://hg.mozilla.org/mozilla-central/rev/2d0471602053
Attachment #647072 - Flags: checkin+
(Assignee)

Comment 28

5 years ago
Created attachment 664745 [details] [diff] [review]
Part 3 - set error page on sidebar and flyouts

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
Attachment #664745 - Flags: review?(gavin.sharp)
(Assignee)

Updated

5 years ago
Attachment #664745 - Attachment is patch: true
Blocks: 789707
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?
Attachment #664745 - Flags: review?(gavin.sharp) → feedback+
(Assignee)

Comment 30

5 years ago
(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
(Assignee)

Comment 31

5 years ago
Created attachment 666840 [details] [diff] [review]
Part 3 - set error on sidebar, flyout and notification panels

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
Attachment #664745 - Attachment is obsolete: true
Attachment #666840 - Flags: review?(gavin.sharp)
(Assignee)

Comment 32

5 years ago
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
(Assignee)

Comment 33

5 years ago
Created attachment 667593 [details] [diff] [review]
Part 4 - dont try to attach mozSocial to error pages

Just an extra patch to suppress throwing the exception of wrong origin when an error page is displayed
Attachment #667593 - Flags: review?(jaws)
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
(Assignee)

Comment 35

5 years ago
Created attachment 667656 [details] [diff] [review]
Part 4 - dont try to attach mozSocial to error pages

sure
Attachment #667593 - Attachment is obsolete: true
Attachment #667593 - Flags: review?(jaws)
Attachment #667656 - Flags: review?(jaws)
Attachment #667656 - Flags: review?(jaws) → review+
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!
Attachment #666840 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 37

5 years ago
(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).
(Assignee)

Comment 38

5 years ago
Created attachment 668642 [details] [diff] [review]
Patch

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
Attachment #666840 - Attachment is obsolete: true
Attachment #668642 - Flags: review?(jaws)
Blocks: 796218
Comment on attachment 668642 [details] [diff] [review]
Patch

r+ on the changes shown in the interdiff.
Attachment #668642 - Flags: review?(jaws) → review+
(Assignee)

Comment 40

5 years ago
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
(Assignee)

Comment 41

5 years ago
Created attachment 668709 [details] [diff] [review]
Roll-up for aurora

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
Attachment #668709 - Flags: review+
Attachment #668709 - Flags: approval-mozilla-aurora?
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
Attachment #668709 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Fx17][strings:1?][leave open] → [Fx17]
(Assignee)

Comment 48

5 years ago
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.
status-firefox17: --- → fixed

Comment 49

5 years ago
Note that the motown icon won't display if the browser is not 'online', causing an empty toolbarbutton.
Keywords: verifyme
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.
(Assignee)

Comment 53

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

Comment 54

5 years ago
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
(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.

Updated

5 years ago
QA Contact: ioana.budnar

Comment 56

5 years ago
Created attachment 681025 [details]
The Server Not Found error in the Social sidebar

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.

Comment 57

5 years ago
(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

Comment 58

5 years ago
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?

Comment 59

5 years ago
(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).

Comment 61

5 years ago
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.
status-firefox17: fixed → verified

Updated

5 years ago
Blocks: 812108

Updated

5 years ago
Blocks: 812112
No longer blocks: 812112

Updated

5 years ago
Depends on: 815000
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.