1131268 - [Browser] Top Sites thumbnails showing settings screens

Status: VERIFIED FIXED

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

Description

[Yeojin Chung [:YeojinC]]

Attachment: logcat_20150209_WrongThumbnail.txt

Description:
When the user tries to go to a website without a data or Wi-Fi connection, the internet connection error screen is displayed. When the user selects "Check settings" and connect to a Wi-Fi network or data connection and return to Browser, a thumbnail shows the settings view under TOP SITES.

Pre-requisite: Have no data or Wi-Fi connected.

Repro Steps:
1) Update a Flame to 20150209010211.
2) Open Browser, and select one of the default top sites (ex. "Mozilla").
3) Select "Check settings", and connect to a Wi-Fi netowrk or cell data.
4) Press the home button, and open Browser.
5) Observe the thumbnails under TOP SITES.

Actual:
The settings view appears under one of the top sites with the URL selected from step 2.

Expected:
The settings view does not appear under top sites.

Environmental Variables:
Device: Flame 3.0 (319mb, full flash)
Build ID: 20150209010211
Gaia: 0d7b35f23402c4cb29bca6b98280fec48a196dec
Gecko: 3436787a82d0
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Repro frequency: 3/3
See attached: screenshot, video clip, logcat
http://youtu.be/bZQd9nxXeXk

Comment 1

[Yeojin Chung [:YeojinC]]

This issue also reproduces on Flame 2.2.

Result: Settings view is shown under TOP SITES.

Device: Flame 2.2 (319mb, full flash)
Build ID: 20150209002504
Gaia: e827781324cbde91d2434b388f5dead3303a85ee
Gecko: 0552759956d3
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

-------------------------------------------
This issue does NOT reproduce on Flame 2.1.

Result: Settings view is NOT shown under TOP SITES. The internet connection error screen view is shown instead.

Device: Flame 2.1 (319mb, full flash)
Build ID: 20150209001212
Gaia: 4a14bb118d55f3d15293c2ff55b7f29f9b0bfcdb
Gecko: 6cbe28d0bb8c
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 2

[Yeojin Chung [:YeojinC]]

Attachment: TopSiteThumbnails.png

Comment 3

[KTucker [:KTucker][Inactive 3/4/2016]]

Hermes, can you weigh in on the severity of this issue?

Comment 4

[Hermes Cheng (inactive after July 27, 2015)]

(In reply to KTucker [:KTucker] from comment #3)
> Hermes, can you weigh in on the severity of this issue?

This might be as design. NI developer.

I would expect that when there is no connection or the "requires an Internet connection" page appears, the browser does not put current visited page into history. Then, the browser will not take screenshot like this bug.

Comment 5

[Ben Francis [:benfrancis]]

Alive, is there a way for an AppWindow to know that it's currently displaying the offline neterror message?

Dale, it seems that if there is then we could add an extra conditional so as not to take a screenshot when in this state? https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/places.js#L151

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Ben Francis [:benfrancis] from comment #5)
> Alive, is there a way for an AppWindow to know that it's currently
> displaying the offline neterror message?
> 
> Dale, it seems that if there is then we could add an extra conditional so as
> not to take a screenshot when in this state?
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/places.js#L151

The message is shown by gecko now; so appWindow doesn't know, but you could try to record it when mozbrowsererror with detail.type='offline' is caught in the event handler. Or maybe just try this.inError, but it may include something which is not the network error.

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L930
http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#1285

Comment 7

[Dale Harvey (:daleharvey)]

I think it makes sense to not screenshot pages with a chrome error, desktop doesnt, taking

Comment 8

[Dale Harvey (:daleharvey)]

Should really fix the underlying issue though as this could crop up in different circumstances, the issue is we always want the contents of the actual iframe and not any possible overlay, this probably differs from other consumers of getScreenshot who want to know what the window currently looks like overlays and all

Comment 9

[Dale Harvey (:daleharvey)]

So we could do with skipping http://mxr.mozilla.org/gaia/source/apps/system/js/browser_mixin.js#101

Comment 10

[Dale Harvey (:daleharvey)]

Alive you got any suggestions on how you would like this fixed? I can pass a 'ignoreFrontWindow' arg through, but given it already has a bunch of optional vars it seems messy, we can call iframe.getScreenshot ourselves manually, but would be missing a lot of the fixes and optimisations we get from reusing the AppWindow version

Comment 11

[(inactive after 6/18) Alive Kuo [:alive]]

How about calling appWindow.getBottomMostWindow().getScreenshot() in places? Is there any drawback with this method?

Comment 12

[GitHub Autolander Bot]

Attachment: [gaia] daleharvey:1131268 > mozilla-b2g:master

Comment 13

[Dale Harvey (:daleharvey)]

Comment on attachment 8574756 [details] [review]
[gaia] daleharvey:1131268 > mozilla-b2g:master

> How about calling appWindow.getBottomMostWindow().getScreenshot() in 
> places? Is there any drawback with this method?

Works perfectly.

However I cant think of a real test for this, I can do a unit one that mocks all the app window api and makes sure it gets called on the bottomWindow but that is a useless test.

It may be possible to load net_error.html manually, take a screenshot and then trigger this error and compare the screenshots, its possible but very tricky.

Comment 14

[Kevin Grandon :kgrandon]

Comment on attachment 8574756 [details] [review]
[gaia] daleharvey:1131268 > mozilla-b2g:master

Seems like a fine approach to me. Thanks!

Comment 15

[Dale Harvey (:daleharvey)]

Thanks Kevin

Green @ https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=15dc1ae5b114
Landed @ https://github.com/mozilla-b2g/gaia/commit/91066c758142e8562ca51f89a3f4c0481d12bff8

Comment 16

[Hermes Cheng (inactive after July 27, 2015)]

Attachment: screenshot (build=20150312160232).png

I tried 5~6 times with build [1] and got white screenshot or "Settings" screenshot. I think white screenshot is correct, but there is still "Settings" as proposed issue.

Yeojin, could you please try it with the latest master build?

[1] build info:
Build ID               20150312160232
Gaia Revision          eabe35cf054d47087b37c1ca7db8143717fbd7f3
Gaia Date              2015-03-12 18:01:49
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/42afc7ef5ccb
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150312.194521
Firmware Date          Thu Mar 12 19:45:32 EDT 2015
Bootloader             L1TC000118D0

Comment 17

[Yeojin Chung [:YeojinC]]

This issue still reproduces on Flame Master. 

Result: Settings view is shown under TOP SITES.

Device: Flame Master (KK, 319mb, full flash)
Build ID: 20150313010238
Gaia: eabe35cf054d47087b37c1ca7db8143717fbd7f3
Gecko: 42afc7ef5ccb
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Comment 18

[Hermes Cheng (inactive after July 27, 2015)]

As comment 16 & 17, reopen this bug.

Comment 19

[Gregor Wagner [:gwagner]]

(In reply to Hermes Cheng[:hermescheng] from comment #18)
> As comment 16 & 17, reopen this bug.

The proper workflow is either to back out the code and re-open or file a new bug and mark it blocking the current bug. It gets confusing if we have open bugs where the code is already landed.
I would suggest to file a new bug in this case. You can also use the clone-this-bug option.

Comment 20

[Hermes Cheng (inactive after July 27, 2015)]

(In reply to Gregor Wagner [:gwagner] from comment #19)
> (In reply to Hermes Cheng[:hermescheng] from comment #18)
> > As comment 16 & 17, reopen this bug.
> 
> The proper workflow is either to back out the code and re-open or file a new
> bug and mark it blocking the current bug. It gets confusing if we have open
> bugs where the code is already landed.
> I would suggest to file a new bug in this case. You can also use the
> clone-this-bug option.

Gregor, thanks for your correction.
Open a new bug by cloning this bug and change the status back to resolved.

Comment 21

[Duane Dixon [:ddixon]]

Removing verifyme keyword since this bug has been cloned and Resolved Verified as Bug 1146230.  

Further, I retested bug 1146230 myself with the latest build on an Aries device (build: 20150818010253) and was not able to repro it.  

The build I retested 1146230 with: 

Device: Aries Master
Build ID: 20150818012543
Gaia: d9d99f32762975a370f1abd34a3512bd6fe29111
Gecko: 90d9b7c391d38ae118865bd87b5d011feee6dded
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 43.0a1 (Master)
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

Comment 22

[Duane Dixon [:ddixon]]

Also, I've confirmed that this bug DOES NOT occur on the latest Flame 2.2 nightly build.  

Repro attempts: 0/5 

Environmental Variables:
Device: Flame 2.2
BuildID: 20150819032503
Gaia: 335cd8e79c20f8d8e93a6efc9b97cc0ec17b5a46
Gecko: ebaa55c4247b
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Changing tracking flags and status to 'verified'.