998118 - [Tarako] launching a simple geolocation page in another tab will show the sad face in the tabs

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Browser, defect)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

1. go to http://people.mozilla.com/~nhirata/html_tp
2. zoom a little and pan to geo_testpage.html
3. long tap on the link and open in new page
4. pan and open drawer

Expected: no sad face
Actual: sad face in the drawer for the geolocation page.

Note: sometimes it gives the "this is embarassing" message esp after you hit the home button and go back to it or lock the screen

Gaia      a8d2d399f2939f4845abaa0df57abab241a2c782
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/d97dad54cb61
BuildID   20140417004002
Version   28.1
ro.build.version.incremental=eng.cltbld.20140417.115200
ro.build.date=Thu Apr 17 11:52:06 EDT 2014
Tarako

Note: do we want to disable extra tabs and the context menu to open in new tab?

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Noming as a consideration for disabling browser tabs

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

ni? Marvin and Gregor

Comment 3

[Gregor Wagner [:gwagner]]

Can we have a logcat?

Comment 4

[Jason Smith [:jsmith]]

Adding qawanted to get a logcat

Comment 5

[bhavana bajaj [:bajaj]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #1)
> Noming as a consideration for disabling browser tabs

I don't think that's possible because we will have hyperlinks on any standard webpage and we will need newtabs created based on the target.

Comment 6

[Dale Harvey (:daleharvey)]

So when we first implemented sad faces on the tab screen I vaguely remember arguing that they shouldnt exist, on memory constrained devices its expected for us to evict old tabs, on the tarako we are pretty much limited to 1 tab, its indicating an error where the application is behaving as expected, when you switch to the sad face tab the page will load.

I think the fix for this should be to not display crash icons on the tab screen, the page will be loaded when the user switches to the tab if it isnt already loaded, this is consistent with other mobile browsers

Comment 7

[Joe Cheng [:jcheng] (please needinfo)]

this bug looks to be similar to Bug 998152 and also bug 991547
but yes the triage team feels that comment 6 makes sense

Comment 8

[Harvey]

Attachment: log.txt

Attaching logcat.

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

Hi Dale, does this bug get resolved by Bug 998152 and bug 991547? thanks

Comment 10

[Dale Harvey (:daleharvey)]

Nope, the tab view will still show the tab opened in the background to be a sad face if its oom'd, it will be reloaded when the user visits it which has always happened, the only 'fix' here would be to not show the sad face for background tabs that crash which I needinfo's firefoxos-ux for , its a small change to the code

Comment 11

[Joe Cheng [:jcheng] (please needinfo)]

ni? swilkes

Comment 12

[Stephany Wilkes]

Flagging Francis for a recommendation on what to do with tabs here. Francis, please re-flag if you're not the correct person for this in this context.

Comment 13

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Note: going to browser settings and then hitting done without doing anything else will also show the sad face icon.

Comment 14

[Marvin Khoo [:Marvin_Khoo]]

We should remove the sad face.

we can remain foreground page (1 tab) run in active, for other tabs, it reload once user select it.

Comment 15

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/18810

Comment 16

[Dale Harvey (:daleharvey)]

If a foreground tab crashes it will be displayed to the user, however if background tabs crash then there is no notification given and when navigating to the tab the url shall be reloaded, this is consistent with other mobile browsers behaviour.

Comment 17

[Ben Francis [:benfrancis]]

Comment on attachment 8414886 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18810

Nice :)

Comment 18

[Ben Francis [:benfrancis]]

Merged into master https://github.com/mozilla-b2g/gaia/commit/ec6a340e133da57f0857ed71276b10a920ab53b9

Requires uplift to 1.3T.

Do we want this in 1.4?

Comment 19

[Dale Harvey (:daleharvey)]

Uplifted to 1.3t

https://github.com/mozilla-b2g/gaia/commit/d26a776beae0070b0032248a2ce482bbe6321a6d

Jason -'d the last similiar patch to 1.4, all for closing out our releases, but if we have a 1.4+ that conflicts because of this then we should definitely request approval

Comment 20

[Montoya Clemmons (MClemmons)]

Verified following STR from Comment 0, if background tabs crash then there is no notification given and when navigating to the tab the url is reloaded to the appropriate screen. No sad face appears on the main screen. 

Environmental Variables: 
Device: Tarako 1.3T MOZ 
BuildID: 20140429014002 
Gaia: b5adc5a943d3abbd6ab070a47c847f2c24891cc5 
Gecko: e9890f5d4709 
Version: 28.1
Firmware Version: sp6821a-gonk4.0-4-29

Comment 21

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8414886 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18810

Hi, we'll need this patch uplifted for 1.4 Dolphin release.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Initial implementation
[User impact] if declined: Crashing tabs on
[Testing completed]: Verified
[Risk to taking this patch] (and alternatives if risky): As far as I can see from the code this is only a visual change in the way we show background tabs, should be low risk.
[String changes made]:

Comment 22

[Preeti Raghunath(:Preeti)]

Comment on attachment 8414886 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18810

Will need to land on Dolphin branch

Comment 23

[Wayne Chang [:wchang]]

We should find out if this impacts Dolphin which has twice the memory or more before deciding if this is really needed on 1.4.

Peipei, Can you please check?

Comment 24

[Peipei Cheng (needinfo if you need my action)]

(In reply to Wayne Chang [:wchang] from comment #23)
> We should find out if this impacts Dolphin which has twice the memory or
> more before deciding if this is really needed on 1.4.
> 
> Peipei, Can you please check?

It's still very easy to reproduce. This could happen if the page user open is too large. Per comment 10, the tab view will still show the tab opened in the background to be a sad face if its oom'd. 

For example:
1. Open google.com
2. Search for "mozilla"
3. Long tap on several search results and open hyperlinks in new taps. (I tried 5 tabs)
4. Check Tab list
   --> 4 of the 5 tabs shows as a sad face.

Comment 25

[Ivan Tsay (:ITsay)]

Hi Dale,

Is the patch good for v1.4 uplift without re-base? Based on comment 24, it seems the issue is still very easy to be reproduced on dolphin. I'd like to request for 1.4 uplift if the patch is good for v1.4. Otherwise, I'll need your help to prepare a v.1.4 patch for the uplift. Thanks.

Comment 26

[Dale Harvey (:daleharvey)]

Attachment: Patch for v1.4

There was a minor conflict with a file that had been deleted, this patch should be good for 1.4

Comment 27

[Ivan Tsay (:ITsay)]

Comment on attachment 8448376 [details] [diff] [review]
Patch for v1.4

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: sad face shows up easily in Geolocation page when there are multiple tabs
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Low, just remove sad face
[String changes made]: N/A

Comment 28

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8448376 [details] [diff] [review]
Patch for v1.4

Approved for 1.4.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/5f5a05e454960872d6d7766f322e1ce837de7458

Sorry, I forgot to set the correct author when committing :(. I can backout and reland if it's important to you.