Closed Bug 998118 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3T+, b2g-v1.3 wontfix, b2g-v1.3T verified, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S1 (9may)
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3 --- wontfix
b2g-v1.3T --- verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: nhirata, Assigned: daleharvey)

References

Details

(Whiteboard: [tarako-bug-bash-1.3T],[systemsfe])

Attachments

(3 files)

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?
Whiteboard: [tarako-bug-bash-1.3T]
Noming as a consideration for disabling browser tabs
blocking-b2g: --- → 1.3T?
ni? Marvin and Gregor
Flags: needinfo?(mkhoo)
Flags: needinfo?(anygregor)
Can we have a logcat?
Flags: needinfo?(anygregor)
Adding qawanted to get a logcat
Keywords: qawanted
(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.
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
Flags: needinfo?(firefoxos-ux-bugzilla)
this bug looks to be similar to Bug 998152 and also bug 991547
but yes the triage team feels that comment 6 makes sense
Assignee: nobody → dale
Attached file log.txt
Attaching logcat.
Keywords: qawanted
QA Contact: jharvey
Hi Dale, does this bug get resolved by Bug 998152 and bug 991547? thanks
Flags: needinfo?(dale)
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
Flags: needinfo?(dale)
ni? swilkes
Flags: needinfo?(swilkes)
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.
Flags: needinfo?(swilkes)
Flags: needinfo?(firefoxos-ux-bugzilla)
Flags: needinfo?(fdjabri)
blocking-b2g: 1.3T? → 1.3T+
Whiteboard: [tarako-bug-bash-1.3T] → [tarako-bug-bash-1.3T],[systemsfe]
Note: going to browser settings and then hitting done without doing anything else will also show the sad face icon.
Flags: needinfo?(dale)
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.
Flags: needinfo?(mkhoo)
Attachment #8414886 - Flags: review?(bfrancis)
Flags: needinfo?(dale)
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.
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
Flags: needinfo?(fdjabri)
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
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 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]:
Attachment #8414886 - Flags: approval-gaia-v1.4?
Comment on attachment 8414886 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18810

Will need to land on Dolphin branch
Attachment #8414886 - Flags: approval-gaia-v1.4?
Whiteboard: [tarako-bug-bash-1.3T],[systemsfe] → [tarako-bug-bash-1.3T],[systemsfe][land Dolphin]
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?
Flags: needinfo?(pcheng)
(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.
Flags: needinfo?(pcheng)
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.
Flags: needinfo?(dale)
Attached patch Patch for v1.4Splinter Review
There was a minor conflict with a file that had been deleted, this patch should be good for 1.4
Flags: needinfo?(dale)
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
Attachment #8448376 - Flags: approval-gaia-v1.4?(lmandel)
Comment on attachment 8448376 [details] [diff] [review]
Patch for v1.4

Approved for 1.4.
Attachment #8448376 - Flags: approval-gaia-v1.4?(lmandel) → approval-gaia-v1.4+
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.
Whiteboard: [tarako-bug-bash-1.3T],[systemsfe][land Dolphin] → [tarako-bug-bash-1.3T],[systemsfe]
You need to log in before you can comment on or make changes to this bug.