1230831 - Tabs without group information get their own group in bookmarks, instead of being put in the active group, causing confusion about missing groups in tab groups migration page

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Screencast of blinking

I upgraded to a Nightly with the Panorama migration, got a new "Bookmarked Tab Groups" folder, and got a tab with chrome://browser/content/aboutTabGroupsMigration.xhtml

The listbox in the tab is acting weird, though. It only shows 1 of the 3 groups, and the  scroll bar is blinking. O_o I wonder if something is stuck trying to add items from the other group that should be listed, causing the scrollbar to trigger but never finishing? (It's been blinking at about 1Hz for a few minutes).

According to the bookmarks manager, the group that's show has 8 items, "Group 1" has 55 items, and "Group 2" has 88 items. Group 1 was my current group and seems to have been properly restored. I'd expect the migration list to show Group 2 (but not Group 1).

No errors in the error console...

Comment 1

[Justin Dolske [:Dolske]]

Is aboutTabGroupsMigration.xhtml expected to work across reloads/restarts? I reloaded the tab but now the list is blank.

Comment 2

[Justin Dolske [:Dolske]]

I can reproduce this with a new Firefox (release) profile, copying Firefox-tabgroups-backup.json over to sessionstore.js, and then running Nightly. So we can debug at Mozlando this week. :)

Comment 3

[Justin Dolske [:Dolske]]

From debugging with Gijs...

We versified that I really only have 2 tab groups -- the "discoball" group listed (as in screenshot), and the active group. But curiously, some of the active tabs have a tabview-tab property specifying the groupID (in the JSON), and some don't (no tabview-tab property at all). The difference causes the active tabs to be split into two separate bookmark folders during the migration. Gijs is going to look at fixing this so that we assume any tabs without a tabview-tab property are actually part of the active group.

This means the aboutTabGroupsMigration.xhtml contents are correct (only showing the 1 non-active group). The blinking scrollbar is weird, but we're assuming that's just a XUL bug.

Comment 4

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1230831 - fix treatment of tabs with no group information, r?mconley

Bug 1230831 - fix treatment of tabs with no group information, r?mconley

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8697991 [details]
MozReview Request: Bug 1230831 - fix treatment of tabs with no group information, r?mconley

https://reviewboard.mozilla.org/r/27757/#review24953

::: browser/modules/TabGroupsMigrator.jsm:104
(Diff revision 1)
> +        let activeGroupId = null;

groupID... activeGroupId... *twitch twitch*

:)

Nit: Can we go for activeGroupID?

::: browser/modules/TabGroupsMigrator.jsm:113
(Diff revision 1)
> +              group = tabViewData.groupID + "";

What's this `+ ""` thing about? I assume we are doing some coersion here? What are the possible values of groupID?

::: browser/modules/TabGroupsMigrator.jsm:128
(Diff revision 1)
> +              tabsWithoutGroup.push(tab);

I know there's logic in here to null tabsWithoutGroup, and I know that we shouldn't enter this block if tabsWithoutGroup is null (since activeGroupID should be set)... still, it might be worth either asserting that tabsWithoutGroup is not null, or maybe just checking to ensure it to be a bit defensive. The logic works out, but the spreading out of the state of tabsWithoutGroup across multiple blocks makes me a bit squirrely. Feel free to push back if that's not warranted.

::: browser/modules/test/xpcshell/test_TabGroupsMigrator.js:299
(Diff revision 1)
> +  if (group2) {

Didn't we just assert that it exists? Why the check?

::: browser/modules/test/xpcshell/test_TabGroupsMigrator.js:313
(Diff revision 1)
> +  if (fallbackActiveGroup) {

Same as above - why the check if we asserted its existence? I know we'll throw if fallbackActiveGroup doesn't exist, but isn't that okay for a test that was going to fail anyway, or is there an advantage here I'm not seeing?

Comment 6

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/27757/#review24975

::: browser/modules/TabGroupsMigrator.jsm:113
(Diff revision 1)
> +              group = tabViewData.groupID + "";

It *should* always be a string, though what it stores is a number, and the tab group code seems to not always assign anything here so it could be null/undefined.

So yes, I'm coercing it to a string. Note that the previous code coerced to a string, too. I rewrote the code to check for falsy-ness so that now I don't need to check for the string "undefined" or "null". Does that make sense?

::: browser/modules/TabGroupsMigrator.jsm:128
(Diff revision 1)
> +              tabsWithoutGroup.push(tab);

It's JS code, I'm not sure how to "assert" in the strict sense of the word.

I understand the concern but I'm not really sure how to address it - I could add a check and make it an empty array if it's null? But we really should never hit this. I don't really want to throw because this code is in the startup path and is dealing with user data - it should ideally never throw. More thoughts? I could Cu.reportError and then still assign an empty array again, or something?

::: browser/modules/test/xpcshell/test_TabGroupsMigrator.js:299
(Diff revision 1)
> +  if (group2) {

So, it seems that `Assert.foo()` throws if the condition is not met, and the rest of the test doesn't run. That doesn't happen in mochitests, and I think it's wrong, and so I wrote the code assuming that that wouldn't be the case. I can remove it if you prefer. Certainly in mochitests just:

```js
ok(foo, ...);
is(foo.bar, ..., ...);
```

the second line would throw if foo was null (ie the first test failed). Essentially that would hide failures lower down in the test, meaning you could potentially fix one failure and then rerun (re-push to try) and just hit another one. I don't think our tests should work that way (or at least not by default), but there we are.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8697991 [details]
MozReview Request: Bug 1230831 - fix treatment of tabs with no group information, r?mconley

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

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8697991 [details]
MozReview Request: Bug 1230831 - fix treatment of tabs with no group information, r?mconley

https://reviewboard.mozilla.org/r/27757/#review24993

I'm good with these changes. Thanks Gijs!

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0fb60a06349b

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0fb60a06349b

Comment 11

[:Gijs (he/him)]

Comment on attachment 8697991 [details]
MozReview Request: Bug 1230831 - fix treatment of tabs with no group information, r?mconley

Approval Request Comment
[Feature/regressing bug #]: tab groups migration code
[User impact if declined]: sorting and naming issues in the tab groups migration page
[Describe test coverage new/current, TreeHerder]: has tests
[Risks and why]: pretty low risk because of the tests, and also, aurora only just branched
[String/UUID change made/needed]: no - I'm reusing existing strings so this is safe for aurora.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8697991 [details]
MozReview Request: Bug 1230831 - fix treatment of tabs with no group information, r?mconley

The earlier comment was for bug 1231114. Oops. Sorry, I'm writing too many at the moment...

Approval Request Comment
[Feature/regressing bug #]: tab groups migration code
[User impact if declined]: if tab groups for some reason doesn't tag tabs as belonging to the active group (which apparently happens...), they don't get migrated correctly
[Describe test coverage new/current, TreeHerder]: has automated tests!
[Risks and why]: pretty low risk because of the tests, and also, aurora only just branched
[String/UUID change made/needed]: no

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8697991 [details]
MozReview Request: Bug 1230831 - fix treatment of tabs with no group information, r?mconley

Has tests, we removed group tabs, taking it.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c71c57675de4

Comment 15

[Brindusa Tot, Desktop QA]

Justin, can you suggest a scenario to test this? We verified migration of tab groups and works correctly.
But, I tried to reproduce the the bug on older builds(from 2015-12-06), in order to verify that it is not reproducing with latest Nightly build. 
Wondering if you can help us with "some of the active tabs have a tabview-tab property specifying the groupID (in the JSON), and some don't (no tabview-tab property at all)" as you mentioned in comment #3.

Comment 16

[:Gijs (he/him)]

(In reply to Brindusa Tot from comment #15)
> Justin, can you suggest a scenario to test this? We verified migration of
> tab groups and works correctly.
> But, I tried to reproduce the the bug on older builds(from 2015-12-06), in
> order to verify that it is not reproducing with latest Nightly build. 
> Wondering if you can help us with "some of the active tabs have a
> tabview-tab property specifying the groupID (in the JSON), and some don't
> (no tabview-tab property at all)" as you mentioned in comment #3.

Justin and I tried to figure out why the tabgroups code sometimes doesn't set the relevant information at the time and couldn't. I didn't want to spend too much time on this (reverse engineering code that has already been removed from the product), so I didn't look into it in too much detail. But there's an automated test that checks that the code behaves correctly here, so I'm not sure manual verification of this particular issue by QA would be useful. It should be possible for Justin to verify that the code now works correctly with his backup of the relevant session file, though, so maybe he can verify?

Comment 17

[Justin Dolske [:Dolske]]

Sorry, I've since deleted the JSON session file that was hitting this. I'm pretty confident we fixed the issue I had, since we debugged this together at Mozlando. Automated tests should suffice.

Comment 18

[Brindusa Tot, Desktop QA]

Based on previous comment set flag qe‑verify to "-".

Comment 19

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Test Successful

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	        beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
No blinking scroll bar is there for restore session tab in the given release.

Actual Results: 
As expected