1130694 - Opening the TabsTray causes all tab thumbnails to refresh and save to the DB

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

I started looking into why my Galaxy Nexus was always getting onTrimMemory notifications at level 15 while running Fennec. output in logcat. Level 15 is critical levels, background process are getting killed.

I noticed that a lot of Bitmap, ByteArrays and BitmapCompress were happening via Monitor's allocation tracking.

It turns out that every time we open the TabsTray, we refresh all the thumbnails via a Gecko event and then save the resulting Bitmaps into the DB. This was added in bug 711543, which I r+, but I did have memory concerns back then!

On a whim, I commented out Tabs.refreshThumbnails [1] and I saw zero onTrimMemory notifications.

When restoring tabs after a crash or just cause you have the preference set, I noticed that the "zombie" (STATE_DELAYED) tabs had no thumbnails until I loaded them. There is code in ThumbnailHelper.getAndProcessThumbnailFor(...) that looks for zombie tabs and tries to load a saved thumbnail from the DB. So I uncommented the code and just put a check to only refresh Tabs with a null thumbnail. This made the zombie tabs show a thumbnail, since it was highly likely their thumbnail was in the DB.

The downside of not refresh all the time is stale thumbnails for dynamic tabs, tabs where DOM stuff is changing and updating the page. Given the memory issues we have and the larger thumbnail sizes (high DPI), I am leaning (again) to not unconditionally update the tab thumbnails when opening the TabsTray.

Comment 1

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

To add insult to injury, the current code will load zombie tab thumbnails from the DB, call ThumbnailHelper.setTabThumbnail(...) which calls Tab.updateThumbnail(...) and then saves the bitmap back out to the DB.

Comment 2

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

Attachment: remove-db-stuff v0.1

First patch just removes the code that loads a thumbnail from the DB in ThumbnailHelper.getAndProcessThumbnailFor(...). It's the only code in the class that uses the DB and it forces a few other callers to pass in a DB, even if they don't want the data from the DB.

Removing this code means the other callers no longer need to pass the DB.

Comment 3

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

Comment on attachment 8560832 [details] [diff] [review]
remove-db-stuff v0.1

I forgot to mention that I move the code to load a thumbnail from the DB into a helper method in the Tab class.

Comment 4

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

Attachment: refresh-empty-tabs v0.1

This patch tweaks the Tabs.refreshThumbnails(..) method to only attempt to load thumbnails from the DB into Tabs that currently have no thumbnail. Note that the helper method does not attempt to save the thumbnail back out to the DB, but it does notify the Tab Listeners of the THUMBNAIL change.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8560832 [details] [diff] [review]
remove-db-stuff v0.1

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

::: mobile/android/base/Tab.java
@@ +672,5 @@
> +    public void loadThumbnailFromDB(final BrowserDB db) {
> +        try {
> +            String url = getURL();
> +            if (url == null)
> +                return;

{}, make url final.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8560833 [details] [diff] [review]
refresh-empty-tabs v0.1

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

::: mobile/android/base/Tabs.java
@@ +547,5 @@
>              @Override
>              public void run() {
>                  for (final Tab tab : mOrder) {
> +                    if (tab.getThumbnail() == null) {
> +                        tab.loadThumbnailFromDB(db);

Please file a follow-up: now that we explicitly retrieve from the DB for a given tab, we can turn this inside out and retrieve all of these thumbnails at once, rather than making a separate query for each.

Comment 7

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/8839dac4cffc
remote:   https://hg.mozilla.org/integration/fx-team/rev/fef1003fe34b

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8839dac4cffc
https://hg.mozilla.org/mozilla-central/rev/fef1003fe34b