Closed Bug 935604 Opened 6 years ago Closed 6 years ago

Regression: Graphical corruption shown on screen during page load

Categories

(Firefox for Android :: Toolbar, defect, major)

26 Branch
ARM
Android
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 28
Tracking Status
firefox25 --- unaffected
firefox26 + fixed
firefox27 + verified
firefox28 + verified
b2g-v1.2 --- fixed
fennec 26+ ---

People

(Reporter: tchung, Assigned: mcomella)

References

(Blocks 1 open bug, )

Details

(Keywords: regression, reproducible)

Attachments

(9 files, 5 obsolete files)

55.98 KB, image/png
Details
416.96 KB, image/png
Details
58.43 KB, image/png
Details
4.55 KB, patch
sriram
: review+
Details | Diff | Splinter Review
3.20 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
3.59 KB, patch
Details | Diff | Splinter Review
3.14 KB, patch
Details | Diff | Splinter Review
4.50 KB, patch
Details | Diff | Splinter Review
57.57 KB, image/png
Details
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
Attached image another screenshot
Just a note, I've been using Firefox release on my Nexus 5 for a couple of days and everything has been fine - regression?
(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.
(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.
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
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
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?
tracking-fennec: --- → ?
Component: General → Graphics, Panning and Zooming
Summary: [Nexus 5] Scrambled images shown during page loading → Regression: Graphical corruption shown on screen during page load
Gut says mcomella but I am starting a bisect.
tracking-fennec: ? → 26+
Flags: needinfo?(kbrosnan)
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.
> 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.
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.
Blocks: 915918
Flags: needinfo?(kbrosnan)
Blocks: 929088
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
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Flags: needinfo?(kbrosnan)
On Google Play there are a number of negative reviews mentioning this on Firefox Beta
Also reproducible on my Asus Nexus 7 (2013).
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.
Flags: needinfo?(kbrosnan)
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
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).
Attachment #831152 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #831177 - Flags: review?(sriram)
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).
Attachment #831152 - Flags: review?(lucasr.at.mozilla) → review-
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).
Updated with the changes discussed on Vidyo.
Attachment #832404 - Flags: review?(lucasr.at.mozilla)
Attachment #831152 - Attachment is obsolete: true
Attachment #831177 - Attachment is obsolete: true
Attachment #831177 - Flags: review?(sriram)
Comment on attachment 832407 [details] [diff] [review]
Part 2: Typing about:home directly does not break tab. v2

Nevermind, this one does not work.
Attachment #832407 - Flags: review?(sriram)
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.
Attachment #832404 - Flags: review?(lucasr.at.mozilla) → review+
Updated for the changes lucasr mentioned in comment 25.
Attachment #832404 - Attachment is obsolete: true
Assignee: lucasr.at.mozilla → michael.l.comella
Rebased and fixed the bug I mentioned in comment 24.
Attachment #832519 - Flags: review?(sriram)
Attachment #832407 - Attachment is obsolete: true
Comment on attachment 832518 [details] [diff] [review]
Part 1: Fix graphical corruption on page load. v3

Move r+.
Attachment #832518 - Flags: review+
Attachment #832519 - Flags: review?(sriram) → review+
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?
> 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).
(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.
Avoid the unnecessary change as per comment 32. This should be done in a
followup though (bug 939177).
Attachment #832518 - Attachment is obsolete: true
Comment on attachment 833000 [details] [diff] [review]
Part 1: Fix graphical corruption on page load. v4

Move r+.
Attachment #833000 - Flags: review+
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
Attachment #833000 - Flags: approval-mozilla-beta?
Attachment #833000 - Flags: approval-mozilla-aurora?
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
Attachment #832519 - Flags: approval-mozilla-beta?
Attachment #832519 - Flags: approval-mozilla-aurora?
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.
Attachment #833000 - Flags: approval-mozilla-beta?
Attachment #833000 - Flags: approval-mozilla-beta+
Attachment #833000 - Flags: approval-mozilla-aurora?
Attachment #833000 - Flags: approval-mozilla-aurora+
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.
Attachment #832519 - Flags: approval-mozilla-beta?
Attachment #832519 - Flags: approval-mozilla-beta-
Attachment #832519 - Flags: approval-mozilla-aurora?
Attachment #832519 - Flags: approval-mozilla-aurora+
(In reply to lsblakk@mozilla.com [: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.
(In reply to Aaron Train [:aaronmt] from comment #40)
> (In reply to lsblakk@mozilla.com [: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.
My rebased patch to Beta, just because it wasn't entirely trivial.
Attachment #8334148 - Attachment description: Part 1 (FF 26): Fix graphical corruption on page load. v4 → Part 1 (FF 26): Fix graphical corruption on page load.
https://hg.mozilla.org/mozilla-central/rev/49b74d3271df
https://hg.mozilla.org/mozilla-central/rev/0a96234a7803
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Duplicate of this bug: 929088
Status: RESOLVED → VERIFIED
Attached image 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
(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?
(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
Since Fx26 is as fixed as it seems like it's going to get, I am flipping the flag back from "affected" to "fixed"
(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.
Verified fixed on:
Build: Firefox for Android 27 Beta 1
Device LG Nexus 4
OS: Android 4.2.2
You need to log in before you can comment on or make changes to this bug.