Tab thumbnails are never displayed/updated

VERIFIED FIXED in Firefox 19

Status

()

Firefox for Android
General
--
major
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: gcp, Assigned: bnicholson)

Tracking

({regression})

Trunk
Firefox 21
ARM
Android
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(firefox19 verified, firefox20 verified, firefox21 verified)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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).
(Reporter)

Updated

5 years ago
Keywords: regression, regressionwindow-wanted
(Reporter)

Comment 1

5 years ago
Bug 817067 is a likely candidate for being the cause.
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.
(Reporter)

Comment 3

5 years ago
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.
I'm still not able to reproduce using the above STR. Is there anything enlightening in the logcat?
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).
Yeah, my history is pretty empty; I've been wiping and reinstalling stuff pretty often lately.
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
Keywords: regressionwindow-wanted
(Reporter)

Comment 8

5 years ago
Created attachment 693272 [details]
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!
(Reporter)

Comment 9

5 years ago
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.
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)
(Reporter)

Comment 11

5 years ago
No Sync in the Nightly. See also STR in comment 3 which doesn't involve Sync.
(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.
(Reporter)

Comment 13

5 years ago
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.

Updated

5 years ago
Duplicate of this bug: 830039
(Reporter)

Comment 15

5 years ago
I'm now seeing this on beta as well.
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → affected
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.

Updated

5 years ago
tracking-fennec: --- → ?
(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.
(Assignee)

Comment 18

5 years ago
I'll take a look at this.
Assignee: nobody → bnicholson
(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?
Flags: needinfo?(wjohnston)
Duplicate of this bug: 829059
(Reporter)

Comment 21

5 years ago
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.
Flags: needinfo?(wjohnston)
(Reporter)

Comment 22

5 years ago
...we might as well remove the entire tabs tray, I was going to say.
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?
Keywords: regressionwindow-wanted
(Assignee)

Comment 24

5 years ago
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.
Blocks: 817067
Keywords: regressionwindow-wanted
(Assignee)

Comment 25

5 years ago
Created attachment 705143 [details] [diff] [review]
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.
Attachment #705143 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 26

5 years ago
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.
(Assignee)

Comment 27

5 years ago
Created attachment 705150 [details] [diff] [review]
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.
Attachment #705143 - Attachment is obsolete: true
Attachment #705143 - Flags: review?(bugmail.mozilla)
Attachment #705150 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 28

5 years ago
Created attachment 705157 [details] [diff] [review]
Continue to process queue on thumbnail capturing failures, v3

Some minor cleanup
Attachment #705157 - Flags: review?(bugmail.mozilla)
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.
Attachment #705157 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Updated

5 years ago
Attachment #705150 - Attachment is obsolete: true
Attachment #705150 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 30

5 years ago
Created attachment 705268 [details] [diff] [review]
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...
Attachment #705157 - Attachment is obsolete: true
Attachment #705268 - Flags: review?(bugmail.mozilla)
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
Attachment #705268 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 32

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
(Assignee)

Comment 33

5 years ago
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
Attachment #705268 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7811eb057627
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21

Updated

5 years ago
status-firefox21: affected → fixed
Comment on attachment 705268 [details] [diff] [review]
Continue to process queue on thumbnail capturing failures, v4

low risk, cleared on central - approving.
Attachment #705268 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 36

5 years ago
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
Attachment #705268 - Flags: approval-mozilla-beta?
(Assignee)

Updated

5 years ago
tracking-fennec: ? → ---

Updated

5 years ago
Duplicate of this bug: 835123
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 .
Attachment #705268 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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 !
Keywords: qawanted, verifyme

Updated

5 years ago
Status: RESOLVED → VERIFIED
status-firefox20: fixed → verified
status-firefox21: fixed → verified
Flags: in-moztrap?(nicolae.cristian)
Keywords: qawanted, verifyme

Updated

5 years ago
QA Contact: aaron.train

Comment 42

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

Anything to change on it ?
Flags: in-moztrap?(nicolae.cristian) → in-moztrap+

Comment 43

5 years ago
Verified fix on Firefox 19.0b2 build 2 (2013-02-05) Samsung Galaxy Tab (Android 3.1)
status-firefox19: fixed → verified
You need to log in before you can comment on or make changes to this bug.