935604 - Regression: Graphical corruption shown on screen during page load

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Tony Chung [:tchung]]

Attachment: screenshot of scrambled mess

on the Nexus 5, android 4.4, loading any content will display scrambled images before the page is fully loaded.   100% reproducible on any sites.

Logcat:
------
11-06 09:22:29.279: I/GeckoToolbar(8674): zerdatime 3219009 - Throbber start
11-06 09:22:29.609: W/GeckoGLController(8674): GLController::resumeCompositor(1080, 1034) and mCompositorCreated=true
11-06 09:22:29.609: W/GeckoGLController(8674): done GLController::resumeCompositor
11-06 09:22:29.609: W/GeckoGLController(8674): GLController::surfaceChanged(1080, 1701) with mSurfaceValid=true
11-06 09:22:29.609: W/GeckoGLController(8674): GLController::resumeCompositor(1080, 1701) and mCompositorCreated=true
11-06 09:22:29.609: E/libEGL(8674): eglMakeCurrent:740 error 300d (EGL_BAD_SURFACE)
11-06 09:22:29.619: W/GeckoGLController(8674): done GLController::resumeCompositor
11-06 09:22:29.619: W/GeckoGLController(8674): done GLController::surfaceChanged with compositor resume
11-06 09:22:29.619: D/GeckoLayerClient(8674): Window-size changed to (1080,1701)
11-06 09:22:31.959: I/GeckoToolbar(8674): zerdatime 3221683 - Throbber start
11-06 09:22:33.559: E/GeckoConsole(8674): [JavaScript Warning: "SyntaxError: Using //@ to indicate source map URL pragmas is deprecated. Use //# instead" {file: "http://ajax.aspnetcdn.com/ajax/jquery.mobile/1.3.2/jquery.mobile-1.3.2.min.js" line: 7}]
11-06 09:22:36.249: E/GeckoConsole(8674): [JavaScript Warning: "Use of getPreventDefault() is deprecated.  Use defaultPrevented instead." {file: "http://ajax.aspnetcdn.com/ajax/jQuery/jquery-1.7.2.min.js" line: 3}]
11-06 09:22:43.959: E/Profiler(8674): BPUnw: [6 total] thread_register_for_profiling(me=0x7a0246d0, stacktop=0x7d19bcef)
11-06 09:22:43.969: I/GeckoToolbar(8674): zerdatime 3233698 - Throbber stop
11-06 09:22:44.119: E/Profiler(8674): BPUnw: [7 total] thread_register_for_profiling(me=0x77c11bd0, stacktop=0x80effc33)
11-06 09:22:44.429: E/ThermalEngine(202): TM Id 'SKIN_THERMAL_management_1' Sensor 'xo_therm_pu2' - alarm raised 1 at 40.0 degC
11-06 09:22:44.429: E/ThermalEngine(202): ACTION: LCD - Setting max LCD brightness to 229
11-06 09:22:44.569: D/dalvikvm(8674): GC_FOR_ALLOC freed 19523K, 11% free 42124K/46972K, paused 26ms, total 29ms
11-06 09:22:45.689: D/dalvikvm(8674): GC_FOR_ALLOC freed 1265K, 11% free 45255K/50336K, paused 16ms, total 16ms
11-06 09:22:47.709: E/Profiler(8674): BPUnw: [6 total] thread_unregister_for_profiling(me=0x77c11bd0) 
11-06 09:22:52.439: I/wpa_supplicant(2898): wlan0: Associated with 00:1a:1e:66:19:c2
11-06 09:22:52.439: I/wpa_supplicant(2898): wlan0: CTRL-EVENT-CONNECTED - Connection to 00:1a:1e:66:19:c2 completed (auth) [id=0 id_str=]

Repro:
1) instal 26 beta 2 on Nexus 5
2) launch and load any website with content (ie. microsoft.com)
3) Verify during content loading, the display is scrambled looking.  It will paint correctly when the page is done loading.

Expected:
- no scrambled display during loading

ActuaL;
- scrambled loading

Comment 1

[Tony Chung [:tchung]]

Attachment: another screenshot

Comment 2

[Chris Lord [:cwiiis]]

Just a note, I've been using Firefox release on my Nexus 5 for a couple of days and everything has been fine - regression?

Comment 3

[Tony Chung [:tchung]]

(In reply to Chris Lord [:cwiiis] from comment #2)
> Just a note, I've been using Firefox release on my Nexus 5 for a couple of
> days and everything has been fine - regression?

I concur, this doesnt reproduce on fx25 Release.   only on beta 2.

Comment 4

[Tony Chung [:tchung]]

(In reply to Tony Chung [:tchung] from comment #3)
> (In reply to Chris Lord [:cwiiis] from comment #2)
> > Just a note, I've been using Firefox release on my Nexus 5 for a couple of
> > days and everything has been fine - regression?
> 
> I concur, this doesnt reproduce on fx25 Release.   only on beta 2.

found one way to reliably reproduce.

1) completely remove beta, and reinstall beta via adb.
$ adb install -r fennec-26.0b2.multi.android-arm.apk 

2) launch beta browser with clean profile
3) visit the URLs.  
4) verify the scramble.  
5) you can even try clearing Data, and relauching.  or rebooting. still reproducible for me.

This affects Nighlty and Aurora as well.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

Simpler STR

1. Make sure Firefox is not running (Quit Now extension or manage applications)
2. Start Firefox
3. Tap in the awesome bar
4. type in a url or select one from the history

Comment 6

[Teodora Vermesan (:TeoVermesan)]

Attachment: Nexus 4: Nightly 2013-11-07

I can reproduce this on LG Nexus 4 (Android 4.2.2)
STR:
1.Open Firefox
2.Click the URL bar
3.Type in "about:home" (it's already there, but type it in anyway)
4.Hit enter to go
Note: Please see the attachment

Comment 7

[Aaron Train [:aaronmt]]

Reproducible on the LG Nexus 4 (Android 4.3) on my personal device using the steps above in comment #6.

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=33c9ee1b4564&tochange=888fa8b299ef

Needs a manual bi-section from this inbound push. Does anyone spot anything obvious?

Comment 8

[Kevin Brosnan [Ex-Mozilla]]

Gut says mcomella but I am starting a bisect.

Comment 9

[Aaron Train [:aaronmt]]

This is semi busted as well on the Samsung Galaxy SIV; typing about:home while on about:home whites the screen out.
Also reproducible on my LG Nexus 5 I received today.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> Gut says mcomella but I am starting a bisect.

I believe :kbrosnan is referring to bug 924480.

The repro steps in comment 6 may include a separate issue - see bug 929088, filed 10/21. When I filed, I did not see corruption, however, now I do.

A corruption image was first uploaded on 10/31 to bug 929088, whereas bug 924480, the potential regression-causing bug, landed the changes in m-c on 10/30, meaning there could be a possible correlation.

Comment 11

[Kevin Brosnan [Ex-Mozilla]]

Change set abfda5e2577 does not reproduce.
Change set b28a7bf0930d does reproduce.

Mike you are on the hook for introducing this with bug 915918. Reproduces on a Nexus 4 or 5.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

bug 915918 is not in 26 or 27 and the non-reproducing changeset you mentioned (abfda5e2577) does not appear to be valid [1]. Did you mean bug 924480? I landed it on m-c right before bug 915918 [2], and it is present in both 26 & 27.

[1]: https://hg.mozilla.org/mozilla-central/rev/abfda5e2577
[2]: https://hg.mozilla.org/mozilla-central/shortlog/152673

Comment 13

[Aaron Train [:aaronmt]]

On Google Play there are a number of negative reviews mentioning this on Firefox Beta

Comment 14

[Aaron Train [:aaronmt]]

Also reproducible on my Asus Nexus 7 (2013).

Comment 15

[Kevin Brosnan [Ex-Mozilla]]

mcomella said on IRC that he had a way forward. Going to clear the needinfo on me. If this requires additional QA work before the bug can be fixed please needinfo me again.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Diagnosis:

The problem only occurs if BrowserSearch is the active View when editing mode is committed.

The LayerView (web content) is hidden in showHomePager() and shown again in hideHomePager(). BrowserApp.filterEditingMode hides the HomePager to show BrowserSearch screen. Upon exit of editing mode, both hideHomePager (again, showing the LayerView) and hideBrowserSearch are called, which should leave the LayerView as the only visible view. However, it is not shown, (presumably) leading to the invalid underlying graphics buffer being shown.

The problem is that when hideHomePager() gets called initially [1], the current tab's url is set to "about:home" so hideHomePager() exits without taking any action (it gets called again later when the page starts load, and the tab is "selected", which is why the web content eventually returns). hideBrowserSearch() is then called, which removes the BrowserSearch View without displaying the LayerView.

I have a fix where hideHomePager acts on the entered url (passed through BrowserToolbar.onStopEditing) rather than the currently selected tab's url, however, typing "about:home" + enter directly into the url bar will break the awesomescreen until web content is shown, perhaps related to bug 929088. Additionally, I will need to see if my fix adversely affects any other state.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=0053063dd872#494

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Fix graphical corruption on page load.

As per comment 16, this patch fixes the graphical corruption outlined in this
bug with the caveat that typing "about:home" into the URL from any screen will
not show the LayerView, causing graphical corruption and an unusable
awesomescreen (though the url bar still works) until a tab with web content is
selected or the BrowserSearch screen is shown.

I feel this is better than the current state of things as it will affect much
fewer users so I'd like to get this reviewed and uplifted, even if the perfect
fix is not implemented. In the meantime, I will continue working to find that
fix (which I'll post as a separate patch).

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Typing about:home directly does not break tab.

Typing "about:home" directly would cause broken state because
filterEditingMode sets the HomePager's visibility to INVISIBLE,
but onStopEditing does not restore this state when it calls
hideBrowserSearch. showHomePager returns with no action as it uses mLoaded
to determine whether or not it is "visible", leaving no Views displayed
and corruption shown.

Instead, I moved the visibility changes into the hide/showBrowserSearch
methods directly, forcing the visibility changes each time BrowserSearch
is hidden/shown (potentially fixing bug 915825, though there are probably
better ways to do this too). This patch fixes bug 929088.

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 831152 [details] [diff] [review]
Fix graphical corruption on page load.

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

Hmmm, why not simply using the 'commit' listener? The 'stop' listener is not meant to be used with URLs (it will be called for both the 'dismiss' and 'commit' cases).

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

As far as I can see, either:
  1) hideHomePager & hideBrowserSearch will be called before loadURL and thus hideHomePager will need the new URL as an arg (rather than using the URL of the currently selected tab). This would mean we have to call hideHomePager twice - once in onStopEditing without the URL, and again in BrowserApp.commitEditingMode with the url, or move hideHomePager out of onStopEditing which doesn't seem the correct approach
  2) We call hideHomePager & hideBrowserSearch after loadURL is called. This will probably make the code very messy with more listeners though I haven't looked further into it.

Are either of these what you had in mind?

Personally, I think that passing a url is okay (could be better), as long as we're explicit that "null" means no new url will be loaded (which is in the interface comment).

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Fix graphical corruption on page load. v2

Updated with the changes discussed on Vidyo.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Typing about:home directly does not break tab. v2

Rebased.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://tbpl.mozilla.org/?tree=Try&rev=e84b6e51ef22

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 832407 [details] [diff] [review]
Part 2: Typing about:home directly does not break tab. v2

Nevermind, this one does not work.

Comment 25

[Lucas Rocha (:lucasr)]

Comment on attachment 832404 [details] [diff] [review]
Part 1: Fix graphical corruption on page load. v2

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

Looks good with the suggested changes.

::: mobile/android/base/BrowserApp.java
@@ +491,5 @@
>  
>          mBrowserToolbar.setOnStopEditingListener(new BrowserToolbar.OnStopEditingListener() {
>              public void onStopEditing() {
>                  selectTargetTabForEditingMode();
> +                hideHomePager(null);

Create an overwrite of hideHomePager() without arguments that uses the url from the current tab under the hood? Something like:

private void hideHomePager() {
    final Tab selectedTab = Tabs.getInstance().getSelectedTab();
    url = (selectedTab != null) ? selectedTab.getURL() : null;

    hideHomePager(url);
}

@@ +1662,5 @@
> +     */
> +    private void hideHomePager(String url) {
> +        if (url == null) {
> +            final Tab selectedTab = Tabs.getInstance().getSelectedTab();
> +            url = (selectedTab != null) ? selectedTab.getURL() : null;

Move this to a hideHomePager() without arguments.

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Fix graphical corruption on page load. v3

Updated for the changes lucasr mentioned in comment 25.

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Typing about:home directly does not break tab. v3

Rebased and fixed the bug I mentioned in comment 24.

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://tbpl.mozilla.org/?tree=Try&rev=c44721bd60f6

Comment 29

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 832518 [details] [diff] [review]
Part 1: Fix graphical corruption on page load. v3

Move r+.

Comment 30

[Lucas Rocha (:lucasr)]

Comment on attachment 832518 [details] [diff] [review]
Part 1: Fix graphical corruption on page load. v3

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

::: mobile/android/base/BrowserApp.java
@@ +1595,5 @@
>                  // Show the toolbar.
>                  mLayerView.getLayerMarginsAnimator().showMargins(false);
>              }
>          } else {
> +            hideHomePager(tab.getURL());

I suppose this call could have been kept unchanged, no?

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> I suppose this call could have been kept unchanged, no?

The diff is missing the additional context – that tab is passed in as a parameter, so I don't think it makes sense to use the currently selected tab there (https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1573).

Comment 32

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #31)
> > I suppose this call could have been kept unchanged, no?
> 
> The diff is missing the additional context – that tab is passed in as a
> parameter, so I don't think it makes sense to use the currently selected tab
> there
> (https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> BrowserApp.java#1573).

The updateHomePagerForTab() method is only meant to be used with the currently selected tab. My suggestion is just to avoid an unnecessary change i.e. less change, less risk.

I don't feel strongly about it. Up to you.

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Fix graphical corruption on page load. v4

Avoid the unnecessary change as per comment 32. This should be done in a
followup though (bug 939177).

Comment 34

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 833000 [details] [diff] [review]
Part 1: Fix graphical corruption on page load. v4

Move r+.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/49b74d3271df
https://hg.mozilla.org/integration/fx-team/rev/0a96234a7803

Comment 36

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 833000 [details] [diff] [review]
Part 1: Fix graphical corruption on page load. v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Likely bug 924480

User impact if declined: 
  N4/N5/N7 users will see graphical corruption between page loads (FF Beta has already received negative reviews on Google Play for this)

Testing completed (on m-c, etc.): 
  Tested locally (not yet merged to m-c)

Risk to taking this patch (and alternatives if risky):
  Low - we make a parameterized version of hideHomePager, moving the original functionality to the unparameterized version, and call it an additional time.
 
String or IDL/UUID changes made by this patch:
  None

Comment 37

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 832519 [details] [diff] [review]
Part 2: Typing about:home directly does not break tab. v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Unknown

User impact if declined:
  Users who explicitly type "about:home" into the url bar will put the browser into an unusable state until they open a new tab with web content
 
Testing completed (on m-c, etc.): See comment 36

Risk to taking this patch (and alternatives if risky): 
  Medium - we change the interaction between hiding and showing the browser search screen and the awesomescreen (but in a much more sensical way)

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

Comment 38

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 833000 [details] [diff] [review]
Part 1: Fix graphical corruption on page load. v4

Approving for branches now because we need this to go into this week's mobile beta.

Comment 39

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 832519 [details] [diff] [review]
Part 2: Typing about:home directly does not break tab. v3

Only taking this to FF27 (Aurora) since it's not likely to be hit by many users (manually entering about:home is not a popular use case) and given the risk and that it's not needed to go in with patch #1, we can wait.

Comment 40

[Aaron Train [:aaronmt]]

(In reply to [e-mail] [:lsblakk] from comment #39)
> Comment on attachment 832519 [details] [diff] [review]
> Part 2: Typing about:home directly does not break tab. v3
> 
> Only taking this to FF27 (Aurora) since it's not likely to be hit by many
> users (manually entering about:home is not a popular use case) and given the
> risk and that it's not needed to go in with patch #1, we can wait.

http://cl.ly/273c0a0p040n This is reproducible on every website.

Comment 41

[Aaron Train [:aaronmt]]

(In reply to Aaron Train [:aaronmt] from comment #40)
> (In reply to [e-mail] [:lsblakk] from comment #39)
> > Comment on attachment 832519 [details] [diff] [review]
> > Part 2: Typing about:home directly does not break tab. v3
> > 
> > Only taking this to FF27 (Aurora) since it's not likely to be hit by many
> > users (manually entering about:home is not a popular use case) and given the
> > risk and that it's not needed to go in with patch #1, we can wait.
> 
> http://cl.ly/273c0a0p040n This is reproducible on every website.

nm, two patches here.

Comment 42

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1 (FF 26): Fix graphical corruption on page load.

My rebased patch to Beta, just because it wasn't entirely trivial.

Comment 43

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/83cc4f2aa0d1

Comment 44

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1 (FF 27): Fix graphical corruption on page load.

Rebase - easier, but still not clean.

Comment 45

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2 (FF 27): Typing about:home directly does not break tab.

Rebase.

Comment 46

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/bfa55569c7e9
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/bedeac830273

Comment 47

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/49b74d3271df
https://hg.mozilla.org/mozilla-central/rev/0a96234a7803

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/83cc4f2aa0d1

Comment 50

[Ryan VanderMeulen [:RyanVM]]

(In reply to Michael Comella (:mcomella) from comment #46)
> remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/bfa55569c7e9
> remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/bedeac830273

Looks like you forgot to post the beta cset link.

https://hg.mozilla.org/releases/mozilla-beta/rev/83cc4f2aa0d1

Comment 51

[Ioana Chiorean]

Attachment: Beta 26.0b6 screenshot

Following the steps from Teodora's comment I am able to reproduce this on Nexus 4 (android 4.2) but not on Galaxy Nexus (Android 4.3)

Fennec Beta 26.0b6

Comment 52

[Lucas Rocha (:lucasr)]

(In reply to Ioana Chiorean from comment #51)
> Created attachment 8336011 [details]
> Beta 26.0b6 screenshot
> 
> Following the steps from Teodora's comment I am able to reproduce this on
> Nexus 4 (android 4.2) but not on Galaxy Nexus (Android 4.3)
> 
> Fennec Beta 26.0b6

I wonder if this patch has been shipped in 26.0b6?

Comment 53

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

(In reply to Ioana Chiorean from comment #51)
> Created attachment 8336011 [details]
> Beta 26.0b6 screenshot
> 
> Following the steps from Teodora's comment I am able to reproduce this on
> Nexus 4 (android 4.2) but not on Galaxy Nexus (Android 4.3)
> 
> Fennec Beta 26.0b6

Those STR:

> I can reproduce this on LG Nexus 4 (Android 4.2.2)
> STR:
> 1.Open Firefox
> 2.Click the URL bar
> 3.Type in "about:home" (it's already there, but type it in anyway)

Based on comment 39, the patch to fix this part is not on Beta

Comment 54

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

Since Fx26 is as fixed as it seems like it's going to get, I am flipping the flag back from "affected" to "fixed"

Comment 55

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Mark Finkle (:mfinkle) from comment #53)
> Those STR:
> 
> > I can reproduce this on LG Nexus 4 (Android 4.2.2)
> > STR:
> > 1.Open Firefox
> > 2.Click the URL bar
> > 3.Type in "about:home" (it's already there, but type it in anyway)
> 
> Based on comment 39, the patch to fix this part is not on Beta

That is correct. The STR that should be fixed in Beta:

1) Open the awesomescreen (i.e. start the browser)
2) Click the URL bar
3) Type in a URL
4) Hit enter

Expected: The page loads across a white background
Actual: Graphical corruption in the background while the page loads

The reasons behind this are (verbosely) explained in comment 16.

Comment 56

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 27 Beta 1
Device LG Nexus 4
OS: Android 4.2.2