Closed Bug 1129614 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox43 fixed, fennec41+)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed
fennec 41+ ---

People

(Reporter: bkelly, Assigned: mfinkle, Mentored)

References

Details

(Keywords: regression, reproducible)

Attachments

(2 files, 3 obsolete files)

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.
This is an N5 on Android L.  Nightly 38.0a1 (2015-02-04).
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.
tracking-fennec: --- → ?
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
Summary: fennec nightly stops updating tab previews sometimes → Regression: Sometimes thumbnails in the tabs drawer are not updated, they expire
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.
tracking-fennec: ? → 38+
Once it triggered for me I waited many minutes and the thumbnail did not update.  It appeared the page was fully loaded.
Mentioned in triage that there wasn't much info dumped to console. It would be helpful to get a build with some additional logging.
Assignee: nobody → rnewman
(Alternatively, maybe mhaigh has some info)
This needs a new assignee, 'cos I'm not going to get to it for a long while.
Assignee: rnewman → nobody
tracking-fennec: 38+ → ?
mhaigh, can you take this?
Assignee: nobody → mhaigh
tracking-fennec: ? → 38+
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
Status: NEW → ASSIGNED
tracking-fennec: 38+ → 39+
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.
Assignee: mhaigh → mark.finkle
tracking-fennec: 39+ → 41+
Mentor: mark.finkle, nalexander
Attached patch thumbnail-expire-fix WIP (obsolete) — Splinter Review
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.
(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?
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.
Attached patch tabs_provider_merging.patch (obsolete) — Splinter Review
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.
Attachment #8616088 - Flags: review?(rnewman)
Attachment #8616088 - Flags: review?(nalexander)
Depends on: 1172077
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!
Attachment #8616088 - Attachment is obsolete: true
Attachment #8616088 - Flags: review?(rnewman)
Attachment #8616088 - Flags: review?(nalexander)
Bug 1172077 just made this a lot easier. Patch coming up.
Attached patch thumbnail-expire-fix WIP v2 (obsolete) — Splinter Review
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.
Attachment #8614291 - Attachment is obsolete: true
(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?
Flags: needinfo?(nalexander)
(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.)
Flags: needinfo?(nalexander)
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.
Attachment #8628595 - Attachment is obsolete: true
Attachment #8654657 - Flags: review?(nalexander)
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.
Attachment #8654657 - Flags: review?(nalexander) → review+
https://hg.mozilla.org/mozilla-central/rev/fa2ee4b3a700
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
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
Let's file new bugs.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.