Closed Bug 811243 Opened 12 years ago Closed 12 years ago

[App Error] Gecko error page still displays at certain conditions

Categories

(Firefox OS Graveyard :: Gaia::System, defect, P2)

Product:
Firefox OS Graveyard
Component:
Gaia::System
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(blocking-basecamp:+)

Status:
RESOLVED FIXED
Milestone:
B2G C2 (20nov-10dec)
Project Flags:
blocking-basecamp +

People

(Reporter: alive, Assigned: vingtetun)

References

Details

Attachments

(1 file)

Patch
8.17 KB, patch
alive
: review+
Details | Diff | Splinter Review 
STR:
1. Turn on airplane mode.
2. Open marketplace app.
3. Click try again.
4. Long press home button.

Expected:
2=> Do not show gecko error page in transitioning opening window.
3=> Do not show gecko error page when reloading.
4=> Do not show gecko error page in card view.

Actual:
Gecko error page is still there.

Note:
After discussion with Josh, to cover them with the dark firefox background may be an acceptable solution.
blocking-basecamp: ? → +
Priority: -- → P2
Let's try covering them with the dark Firefox background and see how well that works. The important thing is to consistently cover them.
Assignee: nobody → alive
I am working on this but I noticed that firstpaint event is also got from gecko error page.
And, after reload(), no firstpaint event anymore.

locationchange is got whenever a page is first time loading or second time.
But, when we get locationchange in the following step:
1. Confirm the device is on airplane mode.
2. Load an app which needs network.
3. Our own error page displays.
4. Turn off the airplane mode
5. Click the try again button.

6. I get loadstart->locationchange->......loading........->loadend, if I remove the dark firefox background when I get locationchange event, the gecko page is still there....disappears after ~1sec.
See Also: → 769189
Justin, I notice there are several locationchange event fired when the page is from error state to normal state. Could you please gave me some hints about what these locationchange means?
I see at least, two locationchange event are fired every time, but when I get the first one, the gecko error page is still here. It disappears after two or three seconds.
Flags: needinfo?(justin.lebar+bug)
> Actual:
> Gecko error page is still there.

Why are we trying to fix this in the platform?  Can't we fix the marketplace app so that it behaves properly when there is no network connection?

Is your goal to make it so that apps never show Gecko error pages?  I thought we explicitly decided against that.

> Justin, I notice there are several locationchange event fired when the page is from 
> error state to normal state. Could you please gave me some hints about what these 
> locationchange means?

I don't entirely understand your question -- it would help if you'd give me the locations that come with the locationchange event -- but in general, we'll fire a locationchange event when transitioning to a Gecko error page.  But if you go to http://thisisageckoerror.com and we show a Gecko error page, we fire a locationchange for http://thisisageckoerror.com.  (This is so the browser's URL bar shows the correct page; try it in Firefox.)

I don't think there's any way for you to distinguish between a "normal" locationchange and an "error" one right now, but I think we could add this easily.

Note that there's also mozbrowsererror, which will tell you when we're showing an error page.

But again, I don't understand why we're trying to solve this in the platform.  This sounds like a bug in the marketplace app to me.
Flags: needinfo?(justin.lebar+bug)
If the platform you mean if gecko, then we are going to fix anything in gecko I think.
What we want to achieve is, prevent the user to see the gecko error page. We have a custom error page in Gaia System app now with reload button. But when the user presses the reload(which calls frame.reload()), the custom error page is hidden, so the gecko error page reveals to the user. The custom error page is coming from the mozbrowsererror event.

For instance, if we turn off the network of the phone(by airplane mode enabling or something else).
And we open the marketplace app. It tries to connect to http://whatever.marketplace.com but failes, we got a mozbrowsererror event and then the system customized error dialog shows.

Step.A
Now disable the airplane mode to bring network up. Click the try again button(call iframe.reload()).
When the frame which is in error state is reloading, we get this events:
1. mozbrowserloadstart
2. mozbrowserlocationchange (event.detail=https://marketplace.allizom.org/telefonica/)
3. mozbrowserlocationchange (event.detail=https://marketplace.allizom.org/telefonica/)
..
n. mozbrowserloadend

Step.B
If we don't bring the network up and try to reload:
1. mozbrowserloadstart
2. mozbrowsererror
3. mozbrowserlocationchange <- matches your locationchange (transfer to error page changing)
4. mozbrowserloadend

I don't have idea when the gecko error page is start to render and when the error page is totally `brought out` during step A. That's my problem.
> What we want to achieve is, prevent the user to see the gecko error page.

Under what circumstances?  No app should ever show a Gecko error page?

> I don't have idea when the gecko error page is start to render and when the error page is 
> totally `brought out` during step A. That's my problem.

Right.  But getting you this information will require Gecko changes.

It's also non-trivial to get this information, because Gecko doesn't know when this transition happens, afaik.

If you guys are trying to completely replace Gecko error pages with your own, from Gaia, this is not the right way to do it, and you're seeing one example of exactly what's wrong with it here...
Marking for C2, given this meets the criteria of known P1/P2 blocking-basecamp+ bugs at the end of C1.
Target Milestone: --- → B2G C2 (20nov-10dec)
(In reply to Justin Lebar [:jlebar] from comment #6)
> It's also non-trivial to get this information, because Gecko doesn't know when this transition happens, afaik.
> If you guys are trying to completely replace Gecko error pages with your
> own, from Gaia, this is not the right way to do it, and you're seeing one
> example of exactly what's wrong with it here...

The resource is limited but there's one and only one lost key to this feature:
We need 'firstpaint' event after each iframe.reload()
Now the firstpaint event only happens once in the same iframe no matter how many times we reload the frame. This mechanism seems incorrect. 'Firstpaint' should be fired every time loading a new page.
So the gecko fix may be not that non-trival.

To clear, if we have this event,
When firstpaint is got by gaia, if the error event is happened after this loadstart event. We will know It's still in error page.
If there's no error event during this reload, we know that we are on the way to the `normal` page.
We should talk synchronously about this sometime; comment 6 was trying to explain why it's non-trivial to change the first-paint event the way you're asking.

It would be trivial to change the firstpaint event the way you describe, but I'm afraid it will not work perfectly (we'll miss some paint events).  I'm on vacation until next week, but I encourage you to experiment by modifying BrowserElementChild.js.  Patrick may be able to help you if you need help.
I wish you guys had cc'ed me on the bug where you decided to implement error pages as an overlay, because it's really not the right way to do it.  (The right thing to do is to replace Gecko's error page.)  I'm not a layout person, so I don't understand all the complexities of painting, but as I understand it, we're increasingly trying to move painting off the main thread, which makes it increasingly difficult to give you the precise "this page is no longer showing on screen" events that you need to make the overlays work.

Josh, can you please elaborate on the UX requirement here?  Do you want specifically the marketplace app not to display a Gecko error page when there's no network connection, or do you want a more general solution?
Flags: needinfo?(jcarpenter)
See also bug 808231; you might be able to use the nextpaint listener we're developing there and do something like

  * Notice that an error page is being shown (mozbrowsererror)
  * Add your Gaia error overlay
  * Call setVisible(false) on the gecko error page

Then, when refreshing the error page, you do

  * Add a nextpaint listener
  * setVisible(true)
  * refresh the frame
  * hide the Gaia error overlay.

But I can't guarantee that this will work like you want it to work.  That is, I can't guarantee that this will not show the Gecko error page.  Again, there's a right way to do this, and it's by either replacing Gecko's error pages or by detecting the offline condition in the Marketplace app.
Also this 'Offline' page will still be visible in the browser which is in favor of Justin solution. If you can confirm that this bug is mostly about Offline mode I can try to move the page in the platform.
(In reply to Vivien Nicolas (:vingtetun) from comment #12)
> Also this 'Offline' page will still be visible in the browser which is in
> favor of Justin solution. If you can confirm that this bug is mostly about
> Offline mode I can try to move the page in the platform.

I have noticed that before but I don't know if that's intentional.
Because the offline gecko message contains 'Firefox OS' wording.
c.c. Ben to see if he has plan/idea on this.

System app and browser app has their mozbrowsererror listener in their own. I can do same thing on both parts.

Last Friday Patrick made a patched gecko for me and it seems work fine. I could get mozbrowserfirstpaint after reload. Now I am fighting with the z-index issue :/
Attached patch Patch — — Splinter Review 
No z-index, firstpain issue. I also think that if in the future we want to replace the browser page to be more aligned this can be used as replacement to netError.xhtml.
Attachment #685531 - Flags: review?(alive)
Assignee: alive → 21
Comment on attachment 685531 [details] [diff] [review]
Patch

Review of attachment 685531 [details] [diff] [review]:
-----------------------------------------------------------------

Clever patch!

The text in the dialog seems little small but not a big issue. I wonder we should use building block here.

So we need this magic in popup manager(window.open) and browser app, too. I think.

::: apps/system/error.html
@@ +20,5 @@
> +
> +  <body>
> +    <div id="modal-dialog-error" class="visible" role="dialog">
> +      <div class="modal-dialog-message-container inner">
> +        <h3 data-l10n-id="error-title" id="title">Hmm, the app is having problems.</h3>

This is causing string flick every time after reload.
Try to remove it.

STR:
1. Turn on airplane mode.
2. Click reload button

Actual:
1. The string 'Hmm, the app is having problems.' flicks once and then shows 'Airplane mode is on'

I wonder this is timing issue but remove the text in HTML is an easiest way.

@@ +22,5 @@
> +    <div id="modal-dialog-error" class="visible" role="dialog">
> +      <div class="modal-dialog-message-container inner">
> +        <h3 data-l10n-id="error-title" id="title">Hmm, the app is having problems.</h3>
> +        <p>
> +         <span data-l10n-id="error-message" id="message">The app has encountered an error and is not loading properly.</span>

Also this.

@@ +26,5 @@
> +         <span data-l10n-id="error-message" id="message">The app has encountered an error and is not loading properly.</span>
> +        </p>
> +      </div>
> +      <menu data-items="2">
> +        <button id="close" data-l10n-id="close">Close</button>

Also flashes if non-English localization. Remove the text.

@@ +28,5 @@
> +      </div>
> +      <menu data-items="2">
> +        <button id="close" data-l10n-id="close">Close</button>
> +        <button id="reload" data-l10n-id="try-again">Try again</button>
> +      </menu>

Also flashes if non-English localization. Remove the text.
Attachment #685531 - Flags: review?(alive) → review+
I will file another bug for popup manager replacement. Cards view don't need one because the iframe content is already replaced.
Urgh, here we go again.

I don't know if this is all written down anywhere, but following the discussion in San Francisco my understanding of the requirements is the following:

*Browser App*
If a "fatal" mozbrowsererror event is fired and the browser app is in the foreground the crash screen should be displayed. No other mozbrowsererror events are handled by the browser and Gecko error messages should be shown. I understood someone was working on applying the new visual designs to these pages, is that the case?

*System App*
Josh doesn't want apps to ever show Gecko error messages, but instead show a limited set of generic messages saying that something went wrong, for all non-fatal mozbrowererror events received by the system app. The message displayed will differ slightly depending on whether there is an active Internet connection and whether the device is in airplane mode.

The current incomplete implementation in the system app displays an overlay over the app window when a non-fatal mozbrowsererror event is received. The disadvantage of this approach is that the Gecko error messages will be momentarily visible while the overlay loads. This was even worse when the error message was transparent because the Gecko error message was visible through the transparency!

These generic messages could be implemented on the platform, but this would require displaying different Gecko error messages for apps and browser tabs. It also raises the question of what type of error should be displayed in the homescreen wrapper which is half way between the two.
Note: With Vivien's patch we fulfil all of these requirements and display the same errors for apps and the homescreen wrapper.

We may not be using the latest visual design for either (Gaia) app errors or (Gecko) browser errors though, who has those?
(In reply to Alive Kuo [:alive] from comment #15)
> Comment on attachment 685531 [details] [diff] [review]
> Patch
> 
> Review of attachment 685531 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Clever patch!
> 
> The text in the dialog seems little small but not a big issue. I wonder we
> should use building block here.
> 
> So we need this magic in popup manager(window.open) and browser app, too. I
> think.
> 
> ::: apps/system/error.html

Hold down a sec, Alive told me about this and I highly doubt it will this will work, at least in the future. I don't know why a <iframe mozapp="app://non-system/manifest"> can load a web page at "app://system/error.html"; my understanding is that iframes should not have access to files in the other packages.

Unless we could confirm this is the desired behavior (and I am wrong), this shouldn't land.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #19)
> (In reply to Alive Kuo [:alive] from comment #15)
> > Comment on attachment 685531 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 685531 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Clever patch!
> > 
> > The text in the dialog seems little small but not a big issue. I wonder we
> > should use building block here.
> > 
> > So we need this magic in popup manager(window.open) and browser app, too. I
> > think.
> > 
> > ::: apps/system/error.html
> 
> Hold down a sec, Alive told me about this and I highly doubt it will this
> will work, at least in the future. I don't know why a <iframe
> mozapp="app://non-system/manifest"> can load a web page at
> "app://system/error.html"; my understanding is that iframes should not have
> access to files in the other packages.
> 

This is because the system application has the permission to resolve it to a real url.

In the long term I would like to see a way to override netError.xhtml from the system app. That would let us customize those errors pages for all the system from one page. I don't really want to move this error.html page to the platform right now because it depends on a lot of css and it would be painful if we need to duplicate it and land a patch on nightly/aurora/beta for each css changes that could happen.
Thanks for the explanation, Ben.

FWIW showing a different Gecko error page for the browser versus for apps wouldn't be infeasible, I think, because the Gecko error page knows the URL of the page with the error, and we can tell whether it's app content by the scheme (http(s):// versus app://).  But with Vivien's patch, it doesn't sound like this is needed.

I don't entirely follow this patch, but is the idea that instead of showing an error overlay we're redirecting the frame to point to an actual error page?  If so, I like that very much.
Flags: needinfo?(jcarpenter)
(In reply to Justin Lebar [:jlebar] from comment #21)
> I don't entirely follow this patch, but is the idea that instead of showing
> an error overlay we're redirecting the frame to point to an actual error
> page?  If so, I like that very much.

That's it.
(In reply to Ben Francis [:benfrancis] from comment #18)
> Note: With Vivien's patch we fulfil all of these requirements and display
> the same errors for apps and the homescreen wrapper.
> 
> We may not be using the latest visual design for either (Gaia) app errors or
> (Gecko) browser errors though, who has those?

I can't be sure about the Gaia app errors, but the Gecko errors are still displaying with their funky light-blue background and white rounded rectangle. I'll talk to the UXE guys about fixing that, since it's mostly CSS (albeit CSS in b2g chrome directories).
This patch is going to be out. I know Justin is not suggesting an overlay implementation,
but could we have any chance get the mozbrowserfirstpaint works after reloads described at comment 13? 

NextPaint just doesn't work for me.
At this point in the schedule, I don't think you're going to get a new browser event unless you write a patch and get it landed in the next few days.

If you can write a patch, I'll do my best to look at it quickly.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: