Last Comment Bug 751690 - Parts of long pages stay on low-res tiles forever
: Parts of long pages stay on low-res tiles forever
Status: VERIFIED FIXED
[mozilla-central]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 15 Branch
: ARM Android
: -- normal (vote)
: ---
Assigned To: Chris Lord [:cwiiis]
:
: Sebastian Kaspari (:sebastian)
Mentors:
: 751838 753003 (view as bug list)
Depends on:
Blocks: 752910
  Show dependency treegraph
 
Reported: 2012-05-03 13:24 PDT by Sam Tobin-Hochstadt [:samth]
Modified: 2016-07-29 14:24 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
beta+


Attachments
Patch (3.09 KB, patch)
2012-05-08 15:20 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
rnewman: feedback+
Details | Diff | Splinter Review
Patch (4.79 KB, patch)
2012-05-08 16:42 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Sam Tobin-Hochstadt [:samth] 2012-05-03 13:24:06 PDT
Sometimes, if I scroll down on a large page while it's loading, I eventually reach low-resolution tiles, and they never get converted to high-res tiles, unless I reload the page.  

This happened before the switch to low-resolution tiles -- previously it was just displayed as all-black.

Unfortunately, I can't make this happen reliably, but it most often happens when all three of the following are the case:

1. I'm on a slow network (ie, 3G).
2. I scroll down quickly on a very long page.
3. I double-tap to zoom before the page finishes loading.
Comment 1 Chris Lord [:cwiiis] 2012-05-04 03:12:15 PDT
I saw this happening the other day, but also have no solid way of reproducing... It seems to happen more often after restoring fennec from the background, but that may just be coincidence.
Comment 2 Aaron Train [:aaronmt] 2012-05-05 13:54:10 PDT
*** Bug 751838 has been marked as a duplicate of this bug. ***
Comment 3 Richard Newman [:rnewman] 2012-05-07 11:54:53 PDT
I've seen this too. Happens a lot on forum pages. Didn't get around to filing yet.

Happened to me on a Transformer tablet on wifi, so it doesn't seem to be much to do with speed.
Comment 4 Kevin Brosnan [:kbrosnan] 2012-05-07 11:58:48 PDT
Browsing around http://ftp.mozilla.org seems to cause this to happen.
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2012-05-07 12:02:09 PDT
Chris can you look into this?
Comment 6 Richard Newman [:rnewman] 2012-05-07 12:14:21 PDT
Attempted to repro, but couldn't. Dammit, Fennec, you're too fast!
Comment 7 Chris Lord [:cwiiis] 2012-05-08 07:47:13 PDT
I'm having a hard(/impossible) time reproducing this. If anyone has reliable, or even semi-reliable STR, please list them.
Comment 8 Paul Feher 2012-05-08 08:27:05 PDT
I've described a similar behavior in the Bug 751838 which was duplicated after this one. Maybe the STR posted there will help.
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-08 08:49:54 PDT
I was able to reproduce this on gmail (basic HTML site, not the mobile one) after pinch zooming. I just retested today using a nightly and I can't reproduce it anymore.
Comment 10 Aaron Train [:aaronmt] 2012-05-08 08:50:59 PDT
You wont see this on Nightly because low res is busted: bug 752910.
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-08 08:58:46 PDT
(In reply to Aaron Train [:aaronmt] from comment #10)
> You wont see this on Nightly because low res is busted: bug 752910.

OK. I tested in Aurora, using 2012-05-07 and I see this issue on GMail. Use the desktop site. I can't pan around. Pinch zoom in a decent amount. Now I can pan around and when I do the page can go low-res and not go high-res.
Comment 12 Chris Lord [:cwiiis] 2012-05-08 11:13:50 PDT
Ok, after poking at the lifecycle code for bug 752910, I've found solid STR for any page page.

1- Load a page.
2- Press the awesome-bar to bring up the awesome-screen
3- Press the home button
4- Load the browser again

You will now be in a state where the compositor will not redraw anything - you'll still see the old rendered content, retained tiles and the low-res cache, but no tiles will get updated.

This seems to be due to the stop/start events in GeckoApp, and what happens when isApplicationInBackground is true. Due to what I assume is a work-around, isApplicationInBackground only ever returns false when an activity has been launched (such as the awesome-screen).

I'll take a look at this tomorrow.
Comment 13 Vladimir Vukicevic [:vlad] [:vladv] 2012-05-08 12:25:51 PDT
Take a glance at bug 752368 and maybe try the patch there -- it could be related, though the symptoms are different (I get just a black content area when the content/compositor gets wedged, not old content).  But it could be related.
Comment 14 Vladimir Vukicevic [:vlad] [:vladv] 2012-05-08 12:29:30 PDT
Yeah, different than bug 752368; ignore that.  I can reproduce it in my build with those patches applied.  I also noticed that once it's in the state after the STR from comment #12, I can navigate to a new page and get its content rendered... but zooming in/panning/etc. doesn't update any tiles beyond that.
Comment 15 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-05-08 15:14:05 PDT
*** Bug 753003 has been marked as a duplicate of this bug. ***
Comment 16 Sriram Ramasubramanian [:sriram] 2012-05-08 15:20:19 PDT
Created attachment 622171 [details] [diff] [review]
Patch

This puts a static variable in GeckoApplication, which knows whether the application was sent to background or not. Every activity relies on that static variable to ascertain if the application is in background or not.

Note: isApplicationSentToBackground() is not intended to be used by any activity. It will be used by GeckoApplication to see whether it is in foreground or not.
Comment 17 Richard Newman [:rnewman] 2012-05-08 15:29:15 PDT
Comment on attachment 622171 [details] [diff] [review]
Patch

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

::: mobile/android/base/GeckoActivity.java.in
@@ +69,2 @@
>      public boolean isApplicationInBackground() {
> +        return GeckoApplication.isApplicationInBackground();

Why is this static, when the rest of this file uses ((GeckoApplication) getApplication()).foo() ?

::: mobile/android/base/GeckoApplication.java
@@ +10,5 @@
>  import android.app.Application;
>  
>  public class GeckoApplication extends Application {
>  
> +    private static boolean mInBackground = false;

Kill the `static`. Otherwise you're just going to have to fix this later (e.g., for Bug 753102).

(And isn't m* the warped-Hungarian for "member"? If this is static, it should be sInBackground.)
Comment 18 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-08 15:33:57 PDT
Comment on attachment 622171 [details] [diff] [review]
Patch

r+ but I agree about dropping the static stuff
Comment 19 Sriram Ramasubramanian [:sriram] 2012-05-08 16:42:31 PDT
Created attachment 622205 [details] [diff] [review]
Patch

This patch removes the "static" member.

I felt "isApplicationSentToBackground()" will still be a misnomer for anyone trying to use on the activity. Hence I used "isGeckoActivityOpened()" which reflects the actual state of the variable.
Comment 20 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-08 17:25:52 PDT
Is this patch causing the crashes you told me about on IRC?
Comment 21 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-08 20:56:07 PDT
(In reply to Mark Finkle (:mfinkle) from comment #20)
> Is this patch causing the crashes you told me about on IRC?

<sriram> i tried removing the "static" 
<sriram> but it crashes the app
<sriram> like.. WINDEATH
<sriram> with no exception

So maybe we should try to use the original patch, which uses the "static" bits for now.

Chris - Can you try that patch out when you get a chance?
Comment 22 Chris Lord [:cwiiis] 2012-05-09 05:28:35 PDT
(In reply to Mark Finkle (:mfinkle) from comment #21)
> (In reply to Mark Finkle (:mfinkle) from comment #20)
> > Is this patch causing the crashes you told me about on IRC?
> 
> <sriram> i tried removing the "static" 
> <sriram> but it crashes the app
> <sriram> like.. WINDEATH
> <sriram> with no exception
> 
> So maybe we should try to use the original patch, which uses the "static"
> bits for now.
> 
> Chris - Can you try that patch out when you get a chance?

Patch seems to work for me, can't easily duplicate the issue in comment #12 anymore.
Comment 23 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-09 07:38:36 PDT
https://hg.mozilla.org/mozilla-central/rev/be5203774846
Comment 24 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-09 12:43:11 PDT
Comment on attachment 622205 [details] [diff] [review]
Patch

[Triage Comment]
Comment 25 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-09 13:02:15 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/9bd737acd13f
Comment 26 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-05-11 11:55:30 PDT
Verified on Galaxy S II; 5/11 Nightly.

Note You need to log in before you can comment on or make changes to this bug.