951776 - screen distorts after tapping on any search suggestion on some devices

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Karen Rudnitski [:kar]]

On my Nexus 7 using GA code (Fx 26).

Start typing in the search bar (any ol' search term, like 'boy'). Tap on a search suggestion (any ol' search suggestion, like 'boy names').

The screen distorts before displaying results. Every time. It's awful. Truly truly awful.

Comment 1

[Erin Lancaster [:elan]]

This sounds like Bug 935604 - Regression: Graphical corruption shown on screen during page load

Comment 2

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

I can repro using the Nexus 4 and the effect is similar to bug 935604. I wonder if the second patch that wasn't uplifted there accidentally fixed this.

Finkle, do you think this is worth fixing/investigating, at the very least to see if the second patch from bug 935604 would fix this (since it's baked a while on the other channels)?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

First, let's get Karen (or someone) to verify that Fx27(Beta) is not affected.

Then we can ask for approval to land on release and determine if a Fx26.0.1 will be released. Or if this bug is bad enough to cause us to ship a Fx26.0.1 for mobile.

Comment 4

[Karen Rudnitski [:kar]]

I installed fx27 / beta and can't reproduce it.

It is an awful experience to hit, especially on a GA release. I would advocate for a respin given the number of affected devices.

Comment 5

[Erin Lancaster [:elan]]

FYI: Me, too. Confirmed repro case with Fx26 GA but not Fx27b1.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Mike - OK, sounds like we need an uplift patch, and request for uplift to release. Then we can get RelMan involved too. 

Lukas - We have a possible Fx26.0.1 candidate.

Comment 7

[Aaron Train [:aaronmt]]

I'm confused here.

In https://bugzilla.mozilla.org/show_bug.cgi?id=935604#c51 we mentioned on 26 that this was still broken and then in https://bugzilla.mozilla.org/show_bug.cgi?id=935604#c54 it was toggled to fixed.

Comment 8

[Aaron Train [:aaronmt]]

Video for Lukas: http://www.youtube.com/watch?v=86TQ1srJmns

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

This affects a significant amount of devices, is easily reproducible, and is a new regression to FF26 - given all that we must do a 26.0.1 in order to never ship this to our users.  An email has been sent to release-drivers and we're hoping to kick builds off in the next couple of hours once a patch has been verified and uplifted to mozilla-release branch.  Please ping in IRC for approvals so we don't waste any time.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Aaron Train [:aaronmt] from comment #7)
> I'm confused here.
> 
> In https://bugzilla.mozilla.org/show_bug.cgi?id=935604#c51 we mentioned on
> 26 that this was still broken and then in
> https://bugzilla.mozilla.org/show_bug.cgi?id=935604#c54 it was toggled to
> fixed.

There were multiple STR on bug 935604. In particular, there is:
  1) Going to any page through the search engine screen
  2) Going from about:home to about:home (having to actually type it in via the url bar to get the search engine screen)

(1) Was fixed by patch #1, uplifted to 26, while (2) was fixed by patch #2, uplifted only to 27 since it didn't seem to be a common use case.

Now, this bug is STR #3, which I didn't realize existed. I think patch #2 coincidentally fixes the issue though I will investigate further.

Comment 11

[Aaron Train [:aaronmt]]

(In reply to Michael Comella (:mcomella) from comment #10)
> (In reply to Aaron Train [:aaronmt] from comment #7)
> > I'm confused here.
> > 
> > In https://bugzilla.mozilla.org/show_bug.cgi?id=935604#c51 we mentioned on
> > 26 that this was still broken and then in
> > https://bugzilla.mozilla.org/show_bug.cgi?id=935604#c54 it was toggled to
> > fixed.
> 
> There were multiple STR on bug 935604. In particular, there is:
>   1) Going to any page through the search engine screen
>   2) Going from about:home to about:home (having to actually type it in via
> the url bar to get the search engine screen)

I see. If I had to guess on why this was missed on 26 is because we tested largely #1 with the patches that did land. I'm not entirely sure why we didn't see this search result issue too but we'll convene and talk about it.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Remove graphical corruption when tapping Google search suggestion.

I followed the FF 27 patch #2 from bug 935604 very closely [1].

The important things that changed in that patch were:
  0) Change hideBrowserSearch to manage the HomePager visibility, rather than
keeping that code in filterEditingMode and dismissEditingMode (why? notice that
commitEditingMode is missing here).
  1) hideBrowserSearch should get called before hideHomePager. hideHomePager
sets the Layer View visibility but returns instantly if it's not visible.
Therefore, we call hideBrowserSearch first to reset the HomePager visibility
(after change #0 occurs) so it can properly set the Layer View visibility. The
corruption occurred because the LayerView was not present.

This patch changes from the FF 27 patch in that animateHideHomePager is a
thing. As it turns out, the animation is TODO, so all FF 26 hideHomePager (with
and without animations) are equivalent to the FF 27 hideHomePager methods. See
FF 27 [2] vs FF 26 [3]. Notice the animate parameter is unused.

I removed the first animateHideHomePager in commitEditingMode because it
returns instantly as the HomePager is not visible. This way, it also better
matches the FF 27 patch.

[1]: https://bugzilla.mozilla.org/attachment.cgi?id=8334185&action=diff
[2]: https://mxr.mozilla.org/mozilla-beta/source/mobile/android/base/BrowserApp.java#1650
[3]: https://mxr.mozilla.org/mozilla-release/source/mobile/android/base/BrowserApp.java#1627

Comment 13

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 8350200 [details] [diff] [review]
Remove graphical corruption when tapping Google search suggestion.

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

Sounds good.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Aaron Train [:aaronmt] from comment #11)
> I see. If I had to guess on why this was missed on 26 is because we tested
> largely #1 with the patches that did land. I'm not entirely sure why we
> didn't see this search result issue too but we'll convene and talk about it.

I should have realized that I was the only one who really understood all the different STR and their connections with one another, and then explained it better. Sorry about that.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Remove graphical corruption when tapping Google search suggestion.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Remove graphical corruption when tapping Google search suggestion. v2

There was a bug where the LayerView would not reappear when dismissing (no
commit) the HomePager on a page besides about:home.

I fixed this by reversing the order of hideBrowserSearch and hideHomePager in
dismissEditingMode (things to change #1 in comment 12). This also matches what
happens in release where cancelEdit eventually calls onStopEditing [1]).

Why this works correctly in 27 is beyond me but I will investigate to ensure we
don't have problems there.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#504

Comment 17

[Tony Chung [:tchung]]

tried (In reply to Michael Comella (:mcomella) from comment #10)
> (In reply to Aaron Train [:aaronmt] from comment #7)
> > I'm confused here.
> > 
> > In https://bugzilla.mozilla.org/show_bug.cgi?id=935604#c51 we mentioned on
> > 26 that this was still broken and then in
> > https://bugzilla.mozilla.org/show_bug.cgi?id=935604#c54 it was toggled to
> > fixed.
> 
> There were multiple STR on bug 935604. In particular, there is:
>   1) Going to any page through the search engine screen
>   2) Going from about:home to about:home (having to actually type it in via
> the url bar to get the search engine screen)
> 
> (1) Was fixed by patch #1, uplifted to 26, while (2) was fixed by patch #2,
> uplifted only to 27 since it didn't seem to be a common use case.
> 
> Now, this bug is STR #3, which I didn't realize existed. I think patch #2
> coincidentally fixes the issue though I will investigate further.

FWIW, i can't reproduce this on a Samsung Galaxy S3 device; Android 4.1.2.  Running Dec 5th build, FENNEC_26_0_RELEASE.  Tried all the different steps above (search suggestions, about:home), and the content will load properly as expected.   Must be hardware specific?

Comment 18

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Tony Chung [:tchung] from comment #17)
> FWIW, i can't reproduce this on a Samsung Galaxy S3 device; Android 4.1.2. 
> Running Dec 5th build, FENNEC_26_0_RELEASE.  Tried all the different steps
> above (search suggestions, about:home), and the content will load properly
> as expected.   Must be hardware specific?

The original bug, bug 935604, was only reproducible on the N4 and N7 (if I'm not missing any). In every case, we're not displaying a view and thus we're displaying an empty graphics buffer (presumably, depending on Android's implementation), and I imagine the drivers on the Nexus devices crap up the buffer, while other devices do not.

I think it might just be LG Nexus devices as I cannot repro on a Samsung Galaxy Nexus.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8350218 [details] [diff] [review]
Remove graphical corruption when tapping Google search suggestion. v2

[Approval Request Comment]
Regression caused by (bug #):
  Unknown

User impact if declined:
  Users on certain Nexus devices (N4, maybe N7, more?) will see graphical corruption when selecting a search suggestion, or typing about:home into the url bar while on about:home
 
Testing completed (on m-c, etc.):
  Similar patches have been on 27-29, for a while.
 
Risk to taking this patch (and alternatives if risky):
  We play with the web content and HomePager visibility. In the worse case, one of those does not reappear and we brick the browser, though it'd likely be fixed on force close restart.

String or IDL/UUID changes made by this patch:
  None

Comment 20

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Local build if anyone would like to test it before it gets merged into release:

http://people.mozilla.org/~mcomella/apks/mcomella-951776_01.apk

Comment 21

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

https://hg.mozilla.org/releases/mozilla-release/rev/b84d04cf97c5

Comment 22

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

We're changing how the interaction of visibility between the different Views works, namely the about:home screen (aka the HomePager), the web content (aka the LayerView), and the search suggestions screen. As such, we should probably verify any combination of switching between these three views. Examples:
  1) Clicking the url bar and backing out of the about:home page with content typed
  2) Clicking the url bar and backing out of about:home without content typed
  3) Doing 1-2 but committing instead
  4) Doing 1-3 from both about:home and some page with web content
  5) Making sure the interaction between closing tabs w/ different views (i.e. about:home to web content and vice versa) works correctly

Additionally, I've only tested locally on an N4. We should also test an N7, probably an N5, and a few other devices (granted, less thoroughly) to ensure there are no issues.

I'll followup if I think of anything else.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Also verify that all the ways to leave the about:home screen work correctly:
  * Bookmarks
  * Top sites (thumbnails and list items)
  * History
  * Reading List
  * Tapping Search suggestions
  * Commiting a typed url
  * Selecting a history item in search suggestions

Comment 24

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Verify:
  * Pressing back/forward to get to the about:home screen

Comment 25

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Tested with no issues on Galaxy Nexus.

Note that on devices that the original issue cannot be repro'ed on (e.g. Galaxy Nexus), the testing probably does not need to be as thorough. However, it may be good to try a slower device to check for race conditions.

Comment 26

[Aaron Train [:aaronmt]]

The above patch and landing has broken switch-to-tab content viewing. See http://www.youtube.com/watch?v=ACA5tRK57zk

Please fix and or back-out.

Comment 27

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Show web content when switching to the current tab.

There was a bug when on a page with web content, hitting the url bar, typing in
a query that would allow you to select the currently selected tab, and
selecting that tab will cause the web content not to reappear.

We needed to call hideHomePager after hideBrowserSearch in maybeSwitchToTab
also, not just in dismissEditingMode. The first hideHomePager needs to be
removed because the HomePager is actually visible when this line gets called,
resulting in the HomePager being hidden and the LayerView displayed.
hideBrowserSearch then shows the HomePager again, but I'm honestly not sure why
a broken HomePager is shown instead of a working one (perhaps some code that
runs later).

This ordering follows what's in Nightly [1], where mBrowserToolbar.cancelEdit()
eventually calls hideBS and hideHP in that order.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1343

Comment 28

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Local build for testing: https://people.mozilla.com/~mcomella/apks/mcomella-951776_02.apk

Comment 29

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8350361 [details] [diff] [review]
Show web content when switching to the current tab.

[Approval Request Comment]
Regression caused by (bug #):
  This one. :(

User impact if declined:
  Users who switch to the currently selected tab will not see web content until they switch to a page with about:home, open a new tab, or click the url bar
 
Testing completed (on m-c, etc.):
  Local
 
Risk to taking this patch (and alternatives if risky):
  Medium - It's just reordering a few statements, similar to the first patch, however, the worst case has the same issues as described in comment 19.

String or IDL/UUID changes made by this patch:
  None

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8350361 [details] [diff] [review]
Show web content when switching to the current tab.

Alright let's hope this forward fix addresses the issue.

Comment 31

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Build in comment 28 was outdated up until now, sorry about that.

Comment 32

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

https://hg.mozilla.org/releases/mozilla-release/rev/1445a7e883c5

Comment 33

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

To be explicit, verify:
  * Switch tab functionality, including switching to the currently selected tab.

Comment 34

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Patch #2 is actually a separate issue also present in 27: bug 952336 was filed for that.

Comment 35

[Aaron Train [:aaronmt]]

Using the aforementioned and newest available local build and against the build based on 1445a7e883c5 (https://tbpl.mozilla.org/?tree=Mozilla-Release&rev=1445a7e883c5).

Based upon my personal testing tonight on a handful of manually ran test-cases

✓ = no visible graphical glitch, and function works as intended

LG Nexus 4 (Android 4.4.2) & LG Nexus 5 (Android 4.4.2) & Asus Nexus 7 (2013, Android 4.4.2) & Samsung Galaxy Note II (Android 4.3) (black-screen buffer)

↳ Tapping a search result after inputting text → ✓ (original reproducible issue here on mozilla-release)
↳ Entering editing mode and inputting a standard URL → ✓
↳ Selecting a history item in search suggestions in edit-mode → ✓ 
↳ Inputting 'about:home' from 'about:home' through the URL-bar → ✓
↳ Backing out of edit mode with content typed back to about:home → ✓
↳ Backing out of edit mode with no content typed back to about:home (no keyboard) → ✓
↳ Switching between open tabs with content → ✓
↳ Navigating from a bookmark from within about:home → ✓
↳ Navigating from a history item within about:home → ✓
↳ Navigating from a reading list item within about:home → ✓
↳ Navigating to a top-site item from thumbnails in about:home → ✓
↳ Navigating to a top-site item from the list of items in about:home → ✓ 

Devices I was not able to reproduce this bug on: Samsung Galaxy SIV (Android 4.3), HTC One (4.4), Samsung Galaxy Tab 3 10" (Android 4.2.2), , Samsung Galaxy SII (Android 4.1.2), Samsung Nexus S (Android 4.1), Asus Nexus 7 (2012, Android 4.4.2), Samsung Nexus 10 (Android 4.4) via :mschifer (see #QA backlog)

Comment 36

[Aaron Train [:aaronmt]]

Updating summary to reflect the findings

Comment 37

[Paul Feher]

Tested on LG Optimus 4X (4.1.2) using steps from comment35 I was not able to reproduce this issue

Comment 38

[Teodora Vermesan (:TeoVermesan)]

Based on exploratory testing, steps from comment35 and smoke-tests using LG Nexus 4 (Android 4.2.2) I haven't found any graphical issues.

Comment 39

[u421692]

Tested on Samsung Galaxy Tab 2 (4.1.1), using steps from comment35 and I was not able to reproduce the reported issues