Closed Bug 905297 Opened 8 years ago Closed 8 years ago

frameworker errorpages prevents panel errorpages from showing up

Categories

(Firefox Graveyard :: SocialAPI, defect)

23 Branch
x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

Attachments

(1 file)

Our handling of the error page for our panels is prevented if the frameworker also had an error.  The result is that the panels will get the default error page, which is not formatted for the size of the panels.  

I've seen this issue very rarely in real use and was unable to track down an STR.  However, with the changes in bug 891218 (remote frameworker) I was able to reliably reproduce the issue with the existing tests.  This patch ensures that the correct error page is always shown for our panels.
Attached patch fix error pagesSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=2be7ca1d1d99
Assignee: nobody → mixedpuppy
Attachment #790361 - Flags: review?(mhammond)
Blocks: 891218
The original reason for this is that if the frameworker has a problem, it's probably a good guess that the panel pages won't work properly, so a provider restart is likely needed (rather than just a page refresh).

I think the best solution would be to style the frameworker error properly for the panels
(In reply to :Felipe Gomes from comment #2)
> The original reason for this is that if the frameworker has a problem, it's
> probably a good guess that the panel pages won't work properly, so a
> provider restart is likely needed (rather than just a page refresh).
> 
> I think the best solution would be to style the frameworker error properly
> for the panels


The problem is not the styling of the frameworker error, it is that the panels dont get the social errorpage if the frameworker had an error, they get the standard error page used in browser tabs.
Blocks: 891221
No longer blocks: 891221
(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> (In reply to :Felipe Gomes from comment #2)
> > The original reason for this is that if the frameworker has a problem, it's
> > probably a good guess that the panel pages won't work properly, so a
> > provider restart is likely needed (rather than just a page refresh).
> > 
> > I think the best solution would be to style the frameworker error properly
> > for the panels
> 
> 
> The problem is not the styling of the frameworker error, it is that the
> panels dont get the social errorpage if the frameworker had an error, they
> get the standard error page used in browser tabs.

The problem is that even if the correct error page was shown, we don't want to display a "refresh" button in that page if the frameworker is down.  Any thoughts on how to keep that in place?  Styling sounds the correct way.  If the timing is out such that the sidebar reports an error before the frameworker, we should still be able to restyle the existing error page for the sidebar such that "reload" is no longer presented as an option?
(In reply to Mark Hammond (:markh) from comment #4)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> > (In reply to :Felipe Gomes from comment #2)
> > > The original reason for this is that if the frameworker has a problem, it's
> > > probably a good guess that the panel pages won't work properly, so a
> > > provider restart is likely needed (rather than just a page refresh).
> > > 
> > > I think the best solution would be to style the frameworker error properly
> > > for the panels
> > 
> > 
> > The problem is not the styling of the frameworker error, it is that the
> > panels dont get the social errorpage if the frameworker had an error, they
> > get the standard error page used in browser tabs.
> 
> The problem is that even if the correct error page was shown, we don't want
> to display a "refresh" button in that page if the frameworker is down.  Any
> thoughts on how to keep that in place?  Styling sounds the correct way.  If
> the timing is out such that the sidebar reports an error before the
> frameworker, we should still be able to restyle the existing error page for
> the sidebar such that "reload" is no longer presented as an option?


this patch does not update the error type if it is already set, I suppose we should change that to not updating it if it is already the frameworker error.  That way, the frameworker error would take precedence if it happened second (the sidebar should then show the frameworker error page).
(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> (In reply to Mark Hammond (:markh) from comment #4)

> > The problem is that even if the correct error page was shown, we don't want
> > to display a "refresh" button in that page if the frameworker is down.

It will be dependent on implementation.  Nothing other than notifications truly requires the frameworker.    e.g. FB share is currently working though their worker is not.
(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> It will be dependent on implementation.  Nothing other than notifications
> truly requires the frameworker.    e.g. FB share is currently working though
> their worker is not.

hmm.  Given providers in the future might not have a frameworker at all, and even ones that do might have a UI that doesn't use it (as you mentioned re facebook), I guess the fact a refresh button might appear when it doesn't now isn't a big problem (and the alternative seems impossible).

Felipe, what do you think?
Flags: needinfo?(felipc)
Yeah, I think it's alright.  If it's possible to do what Shane mentioned (don't change the error if it's already marked a frameworker-error) it's better, but if that's not compatible with some use case, no big deal
Flags: needinfo?(felipc)
(In reply to :Felipe Gomes from comment #8)
> Yeah, I think it's alright.  If it's possible to do what Shane mentioned
> (don't change the error if it's already marked a frameworker-error) it's
> better, but if that's not compatible with some use case, no big deal

I mis-spoke slightly in my earlier comment. The patch already works I suggested, that a frameworker error always sets provider.errorState (in Frameworker.jsm).  With this patch, if the errorState is already set to frameworker-error, then we do not update the state but we callback so our error page is shown rather than the standard error page.  Our error page looks at errorState to see what UI to present.
Attachment #790361 - Flags: review?(mhammond) → review+
https://hg.mozilla.org/mozilla-central/rev/8eb2471839df
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Can someone please guide me on how to test this? Under which circumstances will we show which error pages?
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #12)
> Can someone please guide me on how to test this? Under which circumstances
> will we show which error pages?

Manual testing of this specific case will be hard, as it depends on the order in which things fail.  Eg, setting the browser to offline and loading providers will cause *an* error page, but timing outside of our control will dictate which one.

I think that testing with either offline mode or the network cable disconnected, and ensuring *some* social-specific error page appears is sufficient.
This was intermittently produced via mochitests (clean build), and an odd reproduction that involved running a local node.js server and having patches (yet to land) that mark and I are working on.

I think that for general purposes, this is in testsuite.
Facebook 
 * Offline mode shows a "You're not connected" overlay in the sidebar, though the toolbar still shows me signed in. When I go back online I have to reload the sidebar but it still shows me as "not connected". I have to disable and re-enable Facebook Messenger to get it to come back.
 * Disconnect network shows "Nightly is unable to connect [try again][close sidebar]" UI in the sidebar
 * Reconnecting network and clicking [try again] restores the Facebook sidebar content and toolbar elements as expected

MSN Now
 * Offline mode has no effect, there is no indication that we are offline
 * Disconnect network shows "Nightly is unable to connect [try again][close sidebar]" UI in the sidebar
 * Reconnecting network and clicking [try again] restores the MSN Now sidebar content and toolbar elements as expected

Cliqz
 * Offline mode shows their login UI in the sidebar
 * Disconnect network shows "Nightly is unable to connect [try again][close sidebar]" UI in the sidebar
 * Reconnecting network and clicking [try again] restores the Cliqz sidebar content but the top line of the toolbar button menu has no branding and shows "Unknown" 


It would seem that with my limited testing this is working as expected. Please let me know if we need more specific testing or if the issues I pointed out above warrant follow up bug reports.
Thanks Anthony,
  The only issue I see here is:

> Cliqz
...
>  * Reconnecting network and clicking [try again] restores the Cliqz sidebar
> content but the top line of the toolbar button menu has no branding and
> shows "Unknown" 

That's probably worth a followup, even if it turns out to just be a provider issue.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.