Closed
Bug 1230831
Opened 9 years ago
Closed 9 years ago
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
Categories
(Firefox Graveyard :: Panorama, defect)
Firefox Graveyard
Panorama
Tracking
(firefox45 fixed, firefox46 fixed)
RESOLVED
FIXED
Firefox 46
People
(Reporter: Dolske, Assigned: Gijs)
References
Details
Attachments
(2 files)
(deleted),
video/quicktime
|
Details | |
(deleted),
text/x-review-board-request
|
mconley
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details |
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...
Reporter | ||
Comment 1•9 years ago
|
||
Is aboutTabGroupsMigration.xhtml expected to work across reloads/restarts? I reloaded the tab but now the list is blank.
Reporter | ||
Comment 2•9 years ago
|
||
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. :)
Reporter | ||
Comment 3•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Summary: Missing group in migration tab → 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
Assignee | ||
Comment 4•9 years ago
|
||
Bug 1230831 - fix treatment of tabs with no group information, r?mconley
Attachment #8697991 -
Flags: review?(mconley)
Updated•9 years ago
|
Attachment #8697991 -
Flags: review?(mconley)
Comment 5•9 years ago
|
||
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?
Assignee | ||
Comment 6•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
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/
Attachment #8697991 -
Flags: review?(mconley)
Comment 8•9 years ago
|
||
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!
Attachment #8697991 -
Flags: review?(mconley) → review+
Comment 10•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0fb60a06349b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Comment hidden (obsolete) |
Assignee | ||
Comment 12•9 years ago
|
||
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•9 years ago
|
||
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.
Attachment #8697991 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•9 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/c71c57675de4
Comment 15•9 years ago
|
||
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.
Flags: needinfo?(dolske)
Assignee | ||
Comment 16•9 years ago
|
||
(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?
Reporter | ||
Comment 17•9 years ago
|
||
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.
Flags: needinfo?(dolske)
Comment 19•9 years ago
|
||
[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
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•