Closed
Bug 905297
Opened 11 years ago
Closed 11 years ago
frameworker errorpages prevents panel errorpages from showing up
Categories
(Firefox Graveyard :: SocialAPI, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: mixedpuppy, Assigned: mixedpuppy)
References
Details
Attachments
(1 file)
(deleted),
patch
|
markh
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee: nobody → mixedpuppy
Attachment #790361 -
Flags: review?(mhammond)
Comment 2•11 years ago
|
||
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
Assignee | ||
Comment 3•11 years ago
|
||
(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.
Comment 4•11 years ago
|
||
(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?
Assignee | ||
Comment 5•11 years ago
|
||
(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).
Assignee | ||
Comment 6•11 years ago
|
||
(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.
Comment 7•11 years ago
|
||
(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)
Comment 8•11 years ago
|
||
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)
Assignee | ||
Comment 9•11 years ago
|
||
(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.
Updated•11 years ago
|
Attachment #790361 -
Flags: review?(mhammond) → review+
Assignee | ||
Comment 10•11 years ago
|
||
Comment 11•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Comment 12•11 years ago
|
||
Can someone please guide me on how to test this? Under which circumstances will we show which error pages?
Comment 13•11 years ago
|
||
(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.
Assignee | ||
Comment 14•11 years ago
|
||
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.
Comment 15•11 years ago
|
||
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.
Comment 16•11 years ago
|
||
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.
Updated•6 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•