1265314 - Fennec always reports one tab being open even when no syncable tabs are

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Android Sync, defect, P1)

Description

[Teodora Vermesan (:TeoVermesan)]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0 	
Build ID:  20160415030231 	

Steps to reproduce:
1. Sign in to Firefox on desktop
2. Sign in to Firefox with the same account on an Android device
3. On the Android device open some tabs and close them
4. On desktop, go to Menu -> Sync Now
5. Open sync tabs sidebar

Expected results:
- "No open tabs" message appears in the tab place under the Android device

Actual results:
- Tabs that were closed before sync are displayed in the list

Note:
- if there are no tabs opened after sync, "No open tabs" message is displayed
- reproducible also on Aurora Dev Edition 47.0a2 (2016-04-15)

Comment 1

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #0)
> Steps to reproduce:
> 1. Sign in to Firefox on desktop
> 2. Sign in to Firefox with the same account on an Android device
> 3. On the Android device open some tabs and close them
> 4. On desktop, go to Menu -> Sync Now
> 5. Open sync tabs sidebar

Isn't it possible here that Android simply hasn't performed a Sync yet, so hasn't updated the list of tabs on the server to an empty list?

Can you please try and reproduce after including "ensure Android has Synced" to the STR?

Comment 2

[Teodora Vermesan (:TeoVermesan)]

I have three pages on my Android device:
1. espn.go.com
2. gmail.com
3. google.com

- Perform "Sync Now" both on Desktop & Android and the three pages are displayed in the sidebar under the Android device.
- Close gmail.com and "Sync Now" => espn and google are displayed in the Sidebar
- Close espn and "Sync Now"=> google is displayed in the sidebar
- Close the last tab, google and "Sync Now" => google is still displayed in the sidebar
On my android device, "last synced: 0 minutes ago" is displayed and I waited long enough to ensure the sync has been done.

Comment 3

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

I can verify this. STR:

1) Open a normal web page as the only tab, sync.
2) Look at the list of tabs reported by Fennec - it correctly lists 1 tab.
3) Replace that tab with Fennec's home screen (which should not be synced), and sync.
4) Look at the list of tabs reported by Fennec

Actual: the tab previously opened in (1) is still reported as the single open tab.
Expected: the tab list should be empty.

Comment 5

[Edouard Oger [:eoger]]

There's also another weirdness I noticed:

STR:

1. Open a fresh Firefox, on about:home
2. Long click on a pinned website => Open in New tab
3. Force a Sync

Expected:
Seeing that new tab in my synced tabs on Desktop

Got:
My synced tabs didn't change

As soon as I switch to that tab, this tab will sync OK. So it looks like we don't sync background-started tabs.

Comment 6

[Edouard Oger [:eoger]]

Note: also happens with a non-pinned website (like an history entry at the bottom of about:home).

Comment 7

[Edouard Oger [:eoger]]

From investigation, here is what happens:

- Tabs launched in the background have lastUsed set to 0 (which makes sense).
=> Our tab record may have lastModified = 0 [0]
=> And we may not sync that record [1]

[0] https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/FennecTabsRepository.java#335,344-346

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/FennecTabsRepository.java#163-165

I wonder if we should use the last_modified property of Tab in [0].

Comment 8

[Edouard Oger [:eoger]]

So I dumped a background-tab Tab cursor:

0 {
   device_type=null
   title=Android Developers Blog: Some SecureRandom Thoughts
   favicon=null
   last_modified=null
   guid=null
   url=https://android-developers.googleblog.com/2013/08/some-securerandom-thoughts.html?m=1
   history=["https:\/\/android-developers.googleblog.com\/2013\/08\/some-securerandom-thoughts.html?m=1"]
   name=null
   last_used=0
   _id=25
   position=0
}

Both last_used and last_modified provide no data about when the tab was created, I see two options here:
- We could add that information to every tab, which is probably a lot of work.
- We could sync tabs unconditionally, which is not super battery/bandwidth efficient.
- Maybe there's a timestamp out there which tells us when the whole "tabs" table was modified?

Comment 9

[Edouard Oger [:eoger]]

Turns out last_modified shouldn't be null since we are updating it here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/LocalTabsAccessor.java#240

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Edouard Oger [:eoger] from comment #7)
> From investigation, here is what happens:
> 
> - Tabs launched in the background have lastUsed set to 0 (which makes sense).
> => Our tab record may have lastModified = 0 [0]
> => And we may not sync that record [1]
> 
> [0]
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/
> main/java/org/mozilla/gecko/sync/repositories/android/FennecTabsRepository.
> java#335,344-346
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/
> main/java/org/mozilla/gecko/sync/repositories/android/FennecTabsRepository.
> java#163-165
> 
> I wonder if we should use the last_modified property of Tab in [0].

It's not at all obvious, but the last_modified property of Tab is coming from the associated Client -- see https://dxr.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/mobile/android/base/java/org/mozilla/gecko/db/TabsProvider.java#94.  Underneath that all is a LEFT OUTER JOIN at https://dxr.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/mobile/android/base/java/org/mozilla/gecko/db/TabsProvider.java#292.  The local client is considered to have GUID == NULL, IIRC, so my guess is all the client information is missing for local tabs, so last_modified is just NULL for local tabs.  And indeed, in your record, you can see that the client information isn't there.

What that means is that overloading last_used to mean things about the use of tabs, and to be the flag by which we decide to sync, is not representing the semantic information that we want to convey.  I think the only way to address this is to separate the "should be synced" bit out from the other semantic information, possibly by adding a "last_modified" flag to the tabs table.

rnewman, can you concur or counter?

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Edouard Oger [:eoger] from comment #9)
> Turns out last_modified shouldn't be null since we are updating it here:
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/db/LocalTabsAccessor.java#240

That... is interesting.  Can you dig further and figure out why that's not getting bumped in the situation you're discussing?  Perhaps our query is wrong for the local client when we're JOINing the tabs and clients?

Comment 12

[Edouard Oger [:eoger]]

Found why we are not getting last_modified: you can't compare NULL values with the = operator in SQL.
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/TabsProvider.java#292

Comment 13

[Edouard Oger [:eoger]]

Attachment: bug-1265314.patch

Mozreview is down, attaching a good-old diff file.
This fixes the problem with background-opened tabs, and also when all tabs are closed (original bug description).

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8851093 [details] [diff] [review]
bug-1265314.patch

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

I have a question.

::: mobile/android/base/java/org/mozilla/gecko/db/TabsProvider.java
@@ +288,5 @@
>                      debug("Using sort order " + sortOrder + ".");
>                  }
>  
>                  qb.setProjectionMap(TABS_PROJECTION_MAP);
> +                qb.setTables(TABLE_TABS + " LEFT OUTER JOIN " + TABLE_CLIENTS + " ON (" + TABLE_TABS + "." + Tabs.CLIENT_GUID + " = " + TABLE_CLIENTS + "." + Clients.GUID +

This I understand.  Consider linking to http://stackoverflow.com/a/5424558 for a discussion of the SQL vs. SQLite behaviour, and noting that we're _not_ assuming the SQLite behaviour here.  (In general, favour non-SQLite specific things when you can.)

nit: full sentence comments throughout.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/FennecTabsRepository.java
@@ +342,5 @@
>          final Tab tab = Tab.fromCursor(cursor);
>          record.tabs.add(tab);
>  
> +        if (record.lastModified == 0) { // Will be set once
> +          record.lastModified = RepoUtils.getLongFromCursor(cursor, BrowserContract.Clients.LAST_MODIFIED);

nit: full sentence comment.

Explain why you dropped the lastUsed logic?  If we always write tabs using the LocalTabsAccessor, then yes, Clients.LAST_MODIFIED should be bumped, although perhaps it will be bumped _past_ the most recent Tabs.LAST_USED.  Do you want to that?  I think you don't -- I think you want COALESCE(MAX(Tabs.LAST_USED), Clients.LAST_MODIFIED), like we do elsewhere.

Comment 15

[Edouard Oger [:eoger]]

Thank you, it looks like we already did The Right Thing [0], only the JOIN needed repairing.

[0] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/TabsProvider.java#112

Comment 16

[Edouard Oger [:eoger]]

Attachment: Bug 1265314 - Set tabs record lastModified to clients.lastModified.

Review commit: https://reviewboard.mozilla.org/r/123982/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123982/

Comment 17

[Edouard Oger [:eoger]]

From IRC conversation:

It looks like the local client last_modified column is only modified when we insert/delete a tab, we can therefore use it to set the tabs record lastModified.

Comment 18

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8851756 [details]
Bug 1265314 - Set tabs record lastModified to clients.lastModified.

https://reviewboard.mozilla.org/r/123982/#review126522

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/FennecTabsRepository.java:59
(Diff revision 1)
>  
>      private final ContentProviderClient tabsProvider;
>      private final ContentProviderClient clientsProvider;
>  
>      protected final RepoUtils.QueryHelper tabsHelper;
> +    protected final RepoUtils.QueryHelper clientsHelper;

In general these `QueryHelper` things aren't very valuable.  Don't change it, but next time, consider removing the existing helper and "just reading" rather than adding a second.  (There's no way you could know this without being told -- so much to clean up in this codebase.)

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/FennecTabsRepository.java:164
(Diff revision 1)
>                  localClientSelection, localClientSelectionArgs, positionAscending);
>              try {
>                final String localClientGuid = clientsDataDelegate.getAccountGUID();
>                final String localClientName = clientsDataDelegate.getClientName();
> -              final TabsRecord tabsRecord = FennecTabsRepository.tabsRecordFromCursor(cursor, localClientGuid, localClientName);
> +              final long localClientLastModified = getLocalClientLastModified();
> +              final TabsRecord tabsRecord = FennecTabsRepository.tabsRecordFromCursor(cursor, localClientGuid, localClientName, localClientLastModified);

Somewhere -- probably here -- you should comment on how the tabs record `lastModified` is determined.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/FennecTabsRepository.java:341
(Diff revision 1)
>     * @param clientGuid
>     *          returned tabs record will have this client GUID.
>     * @param clientName
>     *          returned tabs record will have this client name.
> +   * @param clientLastModified
> +   *          returned tabs record will have this client lastModified attribute.

Drop the `client` reference here.  In the Sync world, there's no explicit reference between the tabs record and the client in this way; it's just the Sync `lastModified` value for this "tabs" record.

That is, the `client` bit here makes sense coming from Fennec, but not as much in the Sync ecosystem.  So I think somebody who understood Sync will know what `lastModified` means independently of understanding the client connection.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/FennecTabsRepository.java:341
(Diff revision 1)
>     * @param clientGuid
>     *          returned tabs record will have this client GUID.
>     * @param clientName
>     *          returned tabs record will have this client name.
> +   * @param clientLastModified
> +   *          returned tabs record will have this client lastModified attribute.

nit: just "... will have this lastModified field".

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/db/TestTabsProvider.java:268
(Diff revision 1)
>              cursor = tabsClient.query(BrowserContractHelpers.TABS_CONTENT_URI, null, TABS_CLIENT_GUID_IS, new String[] { TEST_CLIENT_GUID }, positionAscending);
>              Assert.assertEquals(3, cursor.getCount());
>  
>              cursor.moveToPosition(1);
>  
> -            final TabsRecord tabsRecord = FennecTabsRepository.tabsRecordFromCursor(cursor, TEST_CLIENT_GUID, TEST_CLIENT_NAME);
> +            final TabsRecord tabsRecord = FennecTabsRepository.tabsRecordFromCursor(cursor, TEST_CLIENT_GUID, TEST_CLIENT_NAME, 0);

It should be easy to write a few JUnit 4 tests that test that your scenarios do the right thing.  You'd need to expose your `getLocalClientLastModified` function in some way, but it would be nice to actually see that you handle the cases you care about:

* insert two or three regular tabs, client time is >= max
* insert regular, than last_used = 0 tabs, client time is >= regular
* insert last_used = 0 tabs, client time is "reasonable"
[
Do those cover it?  If you're done with this work, file a ticket and make it a mentor ticket.  I'll mentor it with you.  (BTW, running the JUnit 4 tests from AS is pleasant and it's much faster to develop these tests than Robocop.  At least, it used to be pleasant and fast to develop!)

Comment 19

[Edouard Oger [:eoger]]

Comment on attachment 8851756 [details]
Bug 1265314 - Set tabs record lastModified to clients.lastModified.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123982/diff/1-2/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5846a70dfa3c
Set tabs record lastModified to clients.lastModified. r=nalexander

Comment 21

[Mozilla Autoland]

We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.

Comment 22

[Edouard Oger [:eoger]]

It's okay Autoland, you did good the first time.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5846a70dfa3c

Comment 24

[Edouard Oger [:eoger]]

Comment on attachment 8851756 [details]
Bug 1265314 - Set tabs record lastModified to clients.lastModified.

Approval Request Comment
[Feature/Bug causing the regression]: 1265314/1346924
[User impact if declined]: If the users opens tabs in the background or closes all tabs on android, it will not be reflected on other devices.
[Is this code covered by automated tests?]: yes partially
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: not really, the STR are in the beggining of this bug
[List of other uplifts needed for the feature/fix]: -
[Is the change risky?]: Not really risky
[Why is the change risky/not risky?]: we only change the frequency (as a side effect) we send our list of open tabs.
[String changes made/needed]: -

Comment 25

[Edouard Oger [:eoger]]

My bad I forgot we would need to uplift bug 1352608 too.

Comment 26

[Gerry Chang [:gchang]]

Hi :eoger,
Can you put the right information in the [Feature/Bug causing the regression]? This should not be the bug itself.

Hi :TeoVermesan, 
Can you help check if the issue is fixed as expected in the latest nightly?

Comment 27

[Gerry Chang [:gchang]]

+Mihai to help check the fix.

Comment 28

[Edouard Oger [:eoger]]

From the wiki "[Feature/regressing bug #]: If this is a fallout from a feature/bug, or a backout please state the reason along with the bug number", actually the bug has always been there.

Comment 29

[Gerry Chang [:gchang]]

Comment on attachment 8851756 [details]
Bug 1265314 - Set tabs record lastModified to clients.lastModified.

Fix a sync issue. Aurora54+.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

(In reply to Edouard Oger [:eoger] from comment #25)
> My bad I forgot we would need to uplift bug 1352608 too.

This never got an approval request though? Sounds like we don't want to uplift this yet?

Comment 31

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7baf8f12a643

Comment 32

[Edouard Oger [:eoger]]

Thank you!

Comment 33

[Ioana Chiorean]

Mihai Ninu is in PTO so Sorina will look over it.

Comment 34

[Sorina Florean [:sflorean]]

Attachment: 2017_04_18_16_22_01_Firefox_Open_Tabs.png

Platforms:
- Android: Huawei Honor (Android 5.1.1), Asus ZenPad 8 (Android 6.0.1);
- Win 10 (64);
Build: Nightly 55.0a1 and Aurora Dev Edition 54.0a2 from 04-18.

Following the steps from description this issue is not reproducible, the message "No open tabs" is displayed on Synced Tabs Sidebar.

Comment 35

[Stefan Deiac]

Based on comment 34 I will remove the qe-verify flag.