1129614 - Regression: Sometimes thumbnails in the tabs drawer are not updated, they expire

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Screenshot_2015-02-04-15-43-07.png

I noticed today that periodically nightly Firefox for Android stops updating previews in the tab switcher.  See the attached screenshot.

I don't have a good STR yet.  I use homescreen bookmarks heavily.  It seems like its more likely to reproduce if the phone has gone to sleep for a while.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This is an N5 on Android L.  Nightly 38.0a1 (2015-02-04).

Comment 2

[Aaron Train [:aaronmt]]

I saw this too. Thanks for filing. When I hit this I had just opened a bunch of links off Techmeme in the background. I'll see if I can get a logcat.

Comment 3

[Aaron Train [:aaronmt]]

Ran into this on trunk again today after opening some tabs off the top-sites on about:home, after we get a

D/GeckoToolbar( 5865): onTabChanged: THUMBNAIL
D/GeckoBrowserApp( 5865): BrowserApp.onTabChanged: 4: THUMBNAIL

I see

D/GeckoBrowserProvider( 5865): Expiring thumbnails.

CC :rnewman

Comment 4

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

Tracking so we can investigate. Note though, the thumbnails will not appear until the pages finish loading. It seems to work for me, if I wait.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Once it triggered for me I waited many minutes and the thumbnail did not update.  It appeared the page was fully loaded.

Comment 6

[Aaron Train [:aaronmt]]

Mentioned in triage that there wasn't much info dumped to console. It would be helpful to get a build with some additional logging.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(Alternatively, maybe mhaigh has some info)

Comment 8

[Richard Newman [:rnewman]]

This needs a new assignee, 'cos I'm not going to get to it for a long while.

Comment 9

[:Margaret Leibovic]

mhaigh, can you take this?

Comment 10

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

I have a theory that the thumbnails are being expired (based on comment 3), and since they're not in the DB and not in the Tab object (when we get restored after an OOM), we show the empty thumbnail image.

We do not force a thumbnail update for all tabs when the Tab Tray opens (bug 1130694). We found that to be very memory intensive.

If my theory is correct, then Ben should see the thumbnail update when the page updates in comment 5. I don't know if "waiting" means "just left the tabs tray open" or "closed tabs try and reopened". The former would not work. The latter should work. Since Ben says the thumbnail did not update, my theory might be wrong and something else is going on.

We could add some logging here [1] to see if the Tab has an empty thumbnail and if the load from DB also fails.

We expire the thumbnails here [2] and try to keep 15 around. That is based on the frecency, which means one of the open tabs might get it's thumbnail expired when we try to expire thumbnails.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#581
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java#317

Comment 11

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

I have confirmed that comment 10 is the primary cause. If I "pin" some of the open tabs to Top Sites, they thumbnails show up correctly in the Tabs Tray. The reason is that we do not expire thumbnails that have been pinned:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java#329

So, I think we need to find a way to pass in a list/array of URLs for open tabs, and explicitly not expire them.

Comment 12

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

Attachment: thumbnail-expire-fix WIP

This patch does a few things, but is not enough to really fix anything yet:
1. Decouples the thumbnail expiration from history expiration
2. Creates a simpleminded way to pass a list of excluded URLs into the thumbnail expiration code
3. Still needs to pass in the list of open tab URLs so we never expire those URLs.

It might be nice to have access to the tabs DB from browser DB. It would make the SQL much easier.

I was considering something much more simple, but not as "correct". Trying to just ignore the most recently saved thumbnails might skip them open tabs from expiration, but it would be very hit-or-miss.

Comment 13

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #11)
> I have confirmed that comment 10 is the primary cause. If I "pin" some of
> the open tabs to Top Sites, they thumbnails show up correctly in the Tabs
> Tray. The reason is that we do not expire thumbnails that have been pinned:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/
> BrowserProvider.java#329
> 
> So, I think we need to find a way to pass in a list/array of URLs for open
> tabs, and explicitly not expire them.

At the expense of another DB join, we could also match against the "tabs" database.  Is there a reason not to take that approach?  Maybe tabs are in a different DB?

Comment 14

[Richard Newman [:rnewman]]

We've discussed using browser.db for tabs storage.

This kills a DB connection and some on-disk overhead, at the cost of doing the CP work to have them share a database (which we already do for ReadingListProvider, SearchHistoryProvider, and BrowserProvider itself). That's not a lot of work, and it would also allow us to do this join.

Nick, Mark: file a bug for this if you want it, and I can flesh it out and mentor.

N.B., the join would rely on the tabs DB being up to date, which might not be the case.

Comment 15

[Ahmed Khalil]

Attachment: tabs_provider_merging.patch

I've wrote a patch that could make the tabs and clients tables to be written to the browser.db, instead of being written into a separate DB file.

Hopefully, after this change joining with the tabs table when expiring the thumbnails would be much easier.

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 8616088 [details] [diff] [review]
tabs_provider_merging.patch

Ahmed: could you change the commit message and put this in Bug 1172077? Let's track this work there.

Thanks!

Comment 17

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

Bug 1172077 just made this a lot easier. Patch coming up.

Comment 18

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

Attachment: thumbnail-expire-fix WIP v2

This patch is much more simple. It uses the existence of URLs in the 'tabs' table to exclude open thumbnails from expiration.

I need to check that the 'tabs' table is populated even is Sync is not setup. I also noticed that tabs opened in the background are not getting a thumbnail. I thought that was working.

Comment 19

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

(In reply to Mark Finkle (:mfinkle) from comment #18)

> I need to check that the 'tabs' table is populated even is Sync is not
> setup. I also noticed that tabs opened in the background are not getting a
> thumbnail. I thought that was working.

Nick - Can you help answer this question?

Comment 20

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #19)
> (In reply to Mark Finkle (:mfinkle) from comment #18)
> 
> > I need to check that the 'tabs' table is populated even is Sync is not
> > setup. I also noticed that tabs opened in the background are not getting a
> > thumbnail. I thought that was working.
> 
> Nick - Can you help answer this question?

Sorry for the delay.  finkle investigated this himself, and discovered that the answer is no -- the local portion of the tabs DB is only populated when Sync is configured.  See https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#94.

This was an optimization to save disk access when Sync is not configured.  We agree that we should /always/ write to the DB now, as device capabilities have significantly improved.  (In fact, testing for Account existence hits an Android DB -- so it may have been as slow to check as to just write.)

It's worth noting that we may be able to avoid the Runnable altogether -- but there's a write debouncing step that may still have value, and may impact this ticket.  That is, it's technically possible for a tab to be open but not written to the DB yet, and therefore cleared from the favicon DB.  I think that window is small (and perhaps addressed by the history DB being fresh) so it's worth ignoring it.  (Or just commenting on it.)

Comment 21

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

Attachment: thumbnail-expire-fix v0.1

OK. This patch removes the Sync check so we always save current tabs to the tabs.db, which allows the new SQL to work. We should not be removing thumbnails of currently open tabs anymore.

Testing on device seems to show it working, but I think it will take time to see more memory pressure situation. Worth landing though.

Comment 22

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8654657 [details] [diff] [review]
thumbnail-expire-fix v0.1

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

wfm.  Do  consider s/persistAllTabs/queuePersistAllTabs/ and cleaning the surface API.

::: mobile/android/base/Tabs.java
@@ +83,1 @@
>          public PersistTabsRunnable(final Context context, Iterable<Tab> tabsInOrder) {

While you're here, consider removing |persistAllTabs| and replacing the single consumer with |queuePersistAllTabs|.

::: mobile/android/base/db/BrowserContract.java
@@ +272,3 @@
>      public static final class Tabs implements CommonColumns {
>          private Tabs() {}
> +        public static final String TABLE_NAME = "tabs";

Consider making TABLE_NAME part of CommonColumns and enforcing that it exists everywhere.

::: mobile/android/base/db/BrowserProvider.java
@@ +57,5 @@
>      static final String TABLE_BOOKMARKS = Bookmarks.TABLE_NAME;
>      static final String TABLE_HISTORY = History.TABLE_NAME;
>      static final String TABLE_FAVICONS = Favicons.TABLE_NAME;
>      static final String TABLE_THUMBNAILS = Thumbnails.TABLE_NAME;
> +    static final String TABLE_TABS = Tabs.TABLE_NAME;

I find these aliases to be irritating -- the IDE lets you see users from Tabs.TABLE_NAME, and indirection just gets in the way -- but that's not an issue for this patch :)

@@ +321,3 @@
>                             ")";
>          trace("Clear thumbs using query: " + sql);
> +        Log.i(LOGTAG, "Clear thumbs using query: " + sql);

Why trace and Log.i?  One or the other, I think.

Comment 23

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

https://hg.mozilla.org/integration/fx-team/rev/fa2ee4b3a70027f91912352f54ed4693cc3ef96b
Bug 1129614 - Thumbnails in the tabs drawer are expired r=nalexander

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa2ee4b3a700

Comment 25

[u549602]

This issue could still be observed by following the below mentioned repro-steps:

1.User has several individual tabs opened and minimized in the tab tray
2.User enters a search item in the search field
3.After the available pages are displayed user taps to select the page and navigates away from tab before the page is fully loaded
4.User repeats step 2 and 3 several times 
5.User selects previously opened tab
6.User opens tab tray and views page thumbnails 

Expected result:
Step 5:
User asserts that the page has fully loaded in the background
Step 6:
All the thumbnails are displaying fully loaded page view

Actual result:
Step 6:
Thumbnails display pages still in search engine even though all the pages were fully loaded in the background

Please see more details in the following video 
https://youtu.be/VylGjgiKCjE

Comment 26

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

Let's file new bugs.