819953 - Tab thumbnails are never displayed/updated

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

Tab thumbnails no longer update on my Galaxy Tab 10.1 with Android 3.2. Only about:home gets a thumbnail, everything else keeps the "Firefox" gray placeholder forever.

This is a recent regression (+- 4 days).

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Bug 817067 is a likely candidate for being the cause.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Hmm. it works for me on Android 3.1. I will update to Android 3.2, but in the meantime can you provide more specific STR? All I did was load a few pages and follow links with the tab tray open and the thumbnails were updating as expected.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Ok, it is correlated to session restore.

STR: Open a number of tabs on different sites. Wait till everything loads. Exit Firefox via the menu. Restart Firefox, and on about:home click "restore tabs from last time".

The restored tabs never get a thumbnail. New tabs also never get a thumbnail.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

I'm still not able to reproduce using the above STR. Is there anything enlightening in the logcat?

Comment 5

[Wesley Johnston (:wesj)]

Kats, I suspect your history is pretty empty? I'm not surprised if we delete some thumbnails when going in the background and don't have them when we restore. We delete as many as we can on purpose (to save disk space).

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Yeah, my history is pretty empty; I've been wiping and reinstalling stuff pretty often lately.

Comment 7

[Kevin Brosnan [Ex-Mozilla]]

I'm not able to reproduce this behavior. Having 5 or so pages restored opening the tab view results in the Firefox tab placeholder, however within a few moments the expected tab thumbnails are drawn. removing regressionwindow-wanted

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Attachment: adb logcat when reproducing

This is a logcat from when the issue reproduces. Not sure if this is relevant:

E/GeckoConsole(12600): Warning: setDisplayPort resolution did not match zoom for background tab!
E/GeckoConsole(12600): Warning: setDisplayPort resolution did not match zoom for background tab!
E/GeckoConsole(12600): Warning: setDisplayPort resolution did not match zoom for background tab!
E/GeckoConsole(12600): Warning: setDisplayPort resolution did not match zoom for background tab!

Comment 9

[Gian-Carlo Pascutto [:gcp]]

It reproduces reliably in Nightly (big profile, used lots). Doesn't reproduce in my own build from m-c (much smaller profile).

I guess I'll need to do a build with extra logging and configure it so it goes over my Nightly profile.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

gcp, do you have sync set up in your nightly? If so, are the tabs you're seeing from desktop? if so, they won't have thumbnails (and we probably shouldn't be showing them)

Comment 11

[Gian-Carlo Pascutto [:gcp]]

No Sync in the Nightly. See also STR in comment 3 which doesn't involve Sync.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #8)
> E/GeckoConsole(12600): Warning: setDisplayPort resolution did not match zoom
> for background tab!
> E/GeckoConsole(12600): Warning: setDisplayPort resolution did not match zoom
> for background tab!
> E/GeckoConsole(12600): Warning: setDisplayPort resolution did not match zoom
> for background tab!
> E/GeckoConsole(12600): Warning: setDisplayPort resolution did not match zoom
> for background tab!

These are not relevant to thumbnails. They do indicate something is unexpected though and I'll file a bug for it. I've been seeing those messages for a while and haven't bothered to do anything about it.

gcp, something else you could try is download the relevant db files from your profile and looking at them using sqlite3 - if they have the thumbnail data but are not showing the thumbnails then something is wrong; if there is no thumbnail data there then likely we are just pruning it to save disk space.

Comment 13

[Gian-Carlo Pascutto [:gcp]]

Still reproduces with latest Nightly. I checked the browser.db file and there are 13 PNG thumbnails saved, but the tabs from last session are not (all) among them.

Comment 15

[Gian-Carlo Pascutto [:gcp]]

I'm now seeing this on beta as well.

Comment 16

[Aaron Train [:aaronmt]]

I can reproduce this on Nightly; I just have a script that opens 30 tabs; about 7 of 30 get thumbnails; the remainder are blank. I do see lots of onTrimMemory() messages (level 15/20) when this happens though I dont think that's related.

Comment 17

[Wesley Johnston (:wesj)]

(In reply to Aaron Train [:aaronmt] from comment #16)
> I can reproduce this on Nightly; I just have a script that opens 30 tabs;
> about 7 of 30 get thumbnails; the remainder are blank. I do see lots of
> onTrimMemory() messages (level 15/20) when this happens though I dont think
> that's related.

This seems expected to me. If you're hitting critical memory levels (TRIM_MEMORY_RUNNING_CRITICAL  = 15), we dump the thumbnails to save some for you.

Comment 18

[Brian Nicholson (:bnicholson)]

I'll take a look at this.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #17)
> (In reply to Aaron Train [:aaronmt] from comment #16)
> > I can reproduce this on Nightly; I just have a script that opens 30 tabs;
> > about 7 of 30 get thumbnails; the remainder are blank. I do see lots of
> > onTrimMemory() messages (level 15/20) when this happens though I dont think
> > that's related.
> 
> This seems expected to me. If you're hitting critical memory levels
> (TRIM_MEMORY_RUNNING_CRITICAL  = 15), we dump the thumbnails to save some
> for you.

wes, is this won't WONTFIX?

Comment 21

[Gian-Carlo Pascutto [:gcp]]

Please read the entire bug. This happens without any memory pressure too. If we WONTFIX we might as well.

I even saw this today on a Galaxy S3, beta from Market, freshly installed with only 2 tabs.

Comment 22

[Gian-Carlo Pascutto [:gcp]]

...we might as well remove the entire tabs tray, I was going to say.

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

Can we get a patch to add logging around the purging to determine if we're purging when we're not supposed to? or if we aren't saving the thumbnails correctly?

Comment 24

[Brian Nicholson (:bnicholson)]

Thumbnails will break any time there's an error in http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidBridge.cpp#2461. In my case, the error is happening at http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidBridge.cpp#2487 - probably because the tabs tray is open before a page's width/height are known. When there's an error in this method, notifyThumbnail (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ThumbnailHelper.java#143) is never called, so that tab never gets popped off the queue. This, in turn, prevents all future thumbnails from being processed since the queue is stuck.

Comment 25

[Brian Nicholson (:bnicholson)]

Attachment: Continue to process queue on thumbnail capturing failures

We should be more robust in CaptureThumbnail() by making sure we always notify Java upon completion, even when there's a failure. If there is a failure, this patch pops the failed tab and continues processing the remainder of the queue. This means the thumbnail capture will be aborted in the scenario I'm describing above, but it should try a second time (and hopefully succeed) once a document stop happens.

Comment 26

[Brian Nicholson (:bnicholson)]

Looking at this more carefully, I think it might be possible for this patch to result in an infinite loop if a tab is removed during the thumbnail capture. "Tabs.getInstance().getTab(tabId)" would return null, so the "tab != pendingThumbnails.peek()" check inside of processNextThumbnail would fail, causing the same already destroyed tab to be processed again...then rinse, repeat.

Comment 27

[Brian Nicholson (:bnicholson)]

Attachment: Continue to process queue on thumbnail capturing failures, v2

Adds a null check to processNextThumbnail for the reasons described above. If the tab is null, we assume it's the same tab at the beginning of the queue and remove it. This should be a safe assumption as described in the comments, but if we happen to be wrong for some reason, the worst that should happen is that we throw away a thumbnail when we shouldn't.

Comment 28

[Brian Nicholson (:bnicholson)]

Attachment: Continue to process queue on thumbnail capturing failures, v3

Some minor cleanup

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 705157 [details] [diff] [review]
Continue to process queue on thumbnail capturing failures, v3

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

Good catch, that definitely explains all the weirdness. However I think it might be cleaner to make CaptureThumbnail to return a bool instead of a nsresult (since it's only called in one place, and that doesn't even check the return value), and then have the caller (in nsAppShell.cpp) check the return value and call the notify method with the appropriate success value. However that can be done as a follow-up if you don't want to do that here. r=me with the comment below fixed, but I'd like to see the new patch if you choose to do the refactoring instead.

::: widget/android/AndroidBridge.cpp
@@ +2470,5 @@
> +    JNIEnv* env = GetJNIEnv();
> +    if (!env) {
> +        return NS_ERROR_FAILURE;
> +    }
> +

Need to create an AutoLocalJNIFrame here. Its destructor will check for exceptions thrown by things like env->CallStaticVoidMethod, so we need to create it up here before any of the calls to SendThumbnail.

Comment 30

[Brian Nicholson (:bnicholson)]

Attachment: Continue to process queue on thumbnail capturing failures, v4

This patch keeps the existing CaptureThumbnail() implementation mostly the same and moves the thumbnail notification to nsAppShell as suggested. Looking at nsAppShell, I think we actually need similar checks in the ThumbnailRunnable in case a tab/window is removed before it's run, so I added those. Hopefully we now send the notification at all failure points.

Also, in several places in AndroidBridge, I see "AutoLocalJNIFrame jniFrame(env);". But the single argument constructor for AutoLocalJNIFrame is expecting an nEntries int, not a JNIEnv pointer: http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidBridge.h#576. I'm not sure what the impact is, but that doesn't seem right...

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 705268 [details] [diff] [review]
Continue to process queue on thumbnail capturing failures, v4

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

Much better, thanks. r=me with comments addressed.

(In reply to Brian Nicholson (:bnicholson) from comment #30)
> Also, in several places in AndroidBridge, I see "AutoLocalJNIFrame
> jniFrame(env);". But the single argument constructor for AutoLocalJNIFrame
> is expecting an nEntries int, not a JNIEnv pointer:
> http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidBridge.
> h#576. I'm not sure what the impact is, but that doesn't seem right...

There is a second constructor that takes a JNIEnv and an optional int; that is the one that gets picked by the compiler, with the default value for the int.

::: widget/android/AndroidBridge.cpp
@@ +2461,5 @@
> +void
> +AndroidBridge::SendThumbnail(jobject buffer, int32_t tabId, bool success) {
> +    JNIEnv* env = GetJNIEnv();
> +    if (!env)
> +        return;

Add a comment indicating that if we don't get the env here then the thumbnail loop will get stalled again. Not much we can do about it though.

@@ +2513,5 @@
>      }
>  
>      JNIEnv* env = GetJNIEnv();
>      if (!env)
>          return NS_OK;

This return should also be a NS_ERROR_FAILURE

Comment 32

[Brian Nicholson (:bnicholson)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #31)
> There is a second constructor that takes a JNIEnv and an optional int; that
> is the one that gets picked by the compiler, with the default value for the
> int.

Oh yeah, oops.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7811eb057627

Comment 33

[Brian Nicholson (:bnicholson)]

Comment on attachment 705268 [details] [diff] [review]
Continue to process queue on thumbnail capturing failures, v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 817067
User impact if declined: thumbnails stop showing up for pages
Testing completed (on m-c, etc.): just landed m-i
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7811eb057627

Comment 35

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 705268 [details] [diff] [review]
Continue to process queue on thumbnail capturing failures, v4

low risk, cleared on central - approving.

Comment 36

[Brian Nicholson (:bnicholson)]

Comment on attachment 705268 [details] [diff] [review]
Continue to process queue on thumbnail capturing failures, v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 817067
User impact if declined: thumbnails stop showing up for pages
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 37

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/82e55e84d69b

Comment 39

[bhavana bajaj [:bajaj]]

Comment on attachment 705268 [details] [diff] [review]
Continue to process queue on thumbnail capturing failures, v4

Approving as this is low risk, android only , FF19 regression. 

Please land it no later than EOD to get it into 19.0b4 .

Comment 40

[bhavana bajaj [:bajaj]]

(In reply to Aaron Train [:aaronmt] from comment #16)
> I can reproduce this on Nightly; I just have a script that opens 30 tabs;
> about 7 of 30 get thumbnails; the remainder are blank. I do see lots of
> onTrimMemory() messages (level 15/20) when this happens though I dont think
> that's related.

Aaron can you please help verify this no longer happens on aurora ?Will be good to have a confirmation that this is fixed as this is being uplifted to beta.Thanks !

Comment 41

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-beta/rev/c8ba1f0d0f3a

Comment 42

[Ioana Chiorean]

There is already a TC in Moztrap for this: https://moztrap.mozilla.org/manage/case/919

Anything to change on it ?

Comment 43

[Paul Feher]

Verified fix on Firefox 19.0b2 build 2 (2013-02-05) Samsung Galaxy Tab (Android 3.1)