Treestyletab: The tree is broken after Simple Tab Groups switch

Created on 12 Jun 2019 · 29Comments · Source: piroor/treestyletab

Short description

After switching to a new group (managed by Simple Tab Groups extension), the tree is broken.

Steps to reproduce

I can't reproduce it on purpose. I start using FF and suddenly this happens.
As a developer, I understand how important is this. I'm sorry.

Expected result

The trees keep their structure, internally and visually after changing groups.

Actual result

After switching to a new group (managed by Simple Tab Groups extension), the tree is broken.
This doesn't happen everytime, and not always break in the same way.

Sometimes it flattens visually, but the little triangles in parent tabs continue working, but collapsing/expanding the wrong tabs.

Sometimes the tree keep working fine, but it's hierarchy changes lilke this:

Original tree:

A
- A1
- A2
B
- B1
- B2
C
- C1
- C2

where A, B and C are top level tabs.

Changed tree:

A
- A1
- A2
- B
- B1
- B2
- C
  - C1
  - C2

B and C lost it's top level position and began children of the previous sub-tree

Environment

Linux Mint 19.1
Firefox 67.0
STG 3.4.4
TST 3.0.17

I have also installed these extensions that may be interfering:
Firefox Multi-Account Containers (I'm not using it yet. Everything is on the default container)
Auto Tab Discard 0.3.2 (https://add0n.com/tab-discard.html)
uBlock Origin (with it's default settings)

There are other extension, but not related to tabs.

I'm not sure if this is a bug of STG or TST's.
I'm reporting it in both issue trackers.

help wanted partially fixed

Source

cebas

All 29 comments

TST tries to fixup tree structure when tabs are hidden or shown in a tree. You can deactivate this behavior with the option: TST options => Tree Behavior => When visibility of tabs are changed by other addons => Keep tree structure including hidden tabs. Could you try it?

piroor on 12 Jun 2019

I will and report back to you. Thanks

cebas on 12 Jun 2019

I tried with both options and the problem remains.

This is what I've done:

Test A with the option "Fix up the tree" set

main group (1) selected
tree in group 1 is broken
fix the broken tree to be like I want it:

A
- A1
- A2
B
- B1
- B2
C
- C1
- C2

close FF
open FF
tree is fine

change to group 2

change back to group 1
broken tree:

A
- A1
- A2
- B
- B1
- B2
- C
  - C1
  - C2

Test B with the option "Keep tree structure" set

select group 1
fix tree
switch to group 2
switch to group 1
broken again

cebas on 12 Jun 2019

I might to understand why such a problem happens. On my case, I prepared tree as:

A
- A1
- A2
B
- B1
- B2
C
- C1
- C2

and they become flat completely after tabs group switching. With detailed logging I've realized that all tabs are moved when tabs groups are switched. Assume that there are two groups: the Group 1 [X, Y] and the Group 2 [A-C2]:

I'm in the Group 1. Tabs are sorted as:
- [X, Y, A, A1, A2, B, B1, B2, C, C1, C2] (only X and Y are visible / others are hidden)
I'm switching to the Group 2. Tabs are moved by Simple Tab Groups extension as:
1. [A, X, Y, A1, A2, B, B1, B2, C, C1, C2]
2. [A, A1, X, Y, A2, B, B1, B2, C, C1, C2]
3. [A, A1, A2, B, X, Y, B1, B2, C, C1, C2]
4. [A, A1, A2, B, B1, X, Y, B2, C, C1, C2]
5. [A, A1, A2, B, B1, B2, X, Y, C, C1, C2]
6. [A, A1, A2, B, B1, B2, C, X, Y, C1, C2]
7. [A, A1, A2, B, B1, B2, C, C1, X, Y, C2]
8. [A, A1, A2, B, B1, B2, C, C1, C2, X, Y]
Simple Tab Groups changes visibility of tabs as:
- [A, A1, A2, B, B1, B2, C, C1, C2, X, Y] (tabs from A to C2 are visible / X and Y are hidden)

While these operations, tree was broken by those tab moves. For example:

On the step 2-1, a parent tab "A" is moved.
Then it is detached from its tree, and "A1" is promoted to a new parent of "A2".
On the next step 2-2, "A1" is moved.
Then it is detached from its tree. "A2" becomes to a root tab (without no parent).

This result is produced with a TST option: "Tree Behavior" => "When a parent tab is moved or closed by someone from outside of the tree sidebar" => "Treat as a parent by operations inside the tree sidebar. / Treat just a solo tab, by operations outside the tree sidebar." Because tab moves produced by Simple Tab Groups is treated as "operations outside the tree sidebar", they are moved as solo tabs.

And I've tried again with a different option: "Tree Behavior" => "When a parent tab is moved or closed by someone from outside of the tree sidebar" => "Treat as a parent always (move or close all descendant tabs also together). whether the tree sidebar is visible or not." Then tabs are structured as you described: B becomes the last child of A, C becomes the last child of B. The result is produced with these steps:

Tabs are ordered internally as:
- X
- Y
- A
  - A1
  - A2
- B
  - B1
  - B2
"A" is moved to the first position.
- A
- X
- Y
  - A1 (still child of A)
  - A2 (still child of A)
- B
  - B1
  - B2
Then "A1" and "A2" are moved together by TST, because they are children of "A". Tabs are:
- A
  - A1
  - A2
- X
- Y
- B
  - B1
  - B2
"A1" is moved next to "A". But it is already there, so no tab move is produced.
"A2" is moved next to "A1". But it is already there, so no tab move is produced.
"B" is moved next to "A2".
- A
  - A1
  - A2
- B
- X
- Y
  - B1 (still child of B)
  - B2 (still child of B)
TST detects that "B" is placed between "A2" and "X", ~~Because "B" was moved from bottom to top, TST conjectures that the user aimed to attach "B" to a tree, as a next sibling of "A2".~~ Because "B" is not active, TST conjectures that the tab "B" is a new tab opened from "A" and placed as its last new child next to last opened child "A2" (because this is a builtin behavior of Firefox itself.)
- A
  - A1
  - A2
  - B (attached to "A" as its last child, next sibling of "A2")
- X
- Y
  - B1 (still child of B)
  - B2 (still child of B)
TST moves children of "B".
- A
  - A1
  - A2
  - B
    - B1
    - B2
- X
- Y

C, C1, and C2 are also moved like that.

piroor on 12 Jun 2019

With the commit e5d3f44, the case exact same to this report will be treated more correctly. However, TST's auto-fixing of tree structure for moved tabs is very complex, so similar problems may still there around different patterns...

piroor on 12 Jun 2019

👍1

It's working fine now. Thanks!
I have extensions auto update enabled, and as this has been working fine since some days ago, I think that release 3.1.0 fixed the issue.

cebas on 21 Jun 2019

I'm still seeing this issue with 3.1.2.

But, if I read correctly the fix won't be available until 3.1.3. Is that correct @piroor ?

(Side note: Thanks for your work on this amazing plugin)

m-thomson on 1 Jul 2019

I already uploaded 3.1.3 to AMO and it is still waiting for review.

piroor on 2 Jul 2019

🎉1

@piroor With TST 3.1.5 and STG 3.4.4, the tree structure still can be corrupted when switching the group.

dksher on 23 Jul 2019

@dksher As discussed above, tree corruption is triggered by tab moves produced by STG, and only some cases are fixed by TST. I need detailed steps to reproduce the problem you saw, like the initial comment of this issue.

piroor on 24 Jul 2019

I can't reproduce it on purpose as with op. When I switch the group, the tree structure sometimes get broken.

Original tree:

A
B
- B1
- B2
C
- C1

Broken tree Case 1:

Broken tree Case 2:

A
- C1
B
- B1
- B2
C _(lost C1 but still has ▼ mark)_

Firefox 68.0.1
TST 3.1.5
STG 3.4.4
Other addons: uBlock Origin, Multiple Tab Handler, etc.

dksher on 24 Jul 2019

@piroor
Broken tree Case 3:

A
- B2
- B1
B
C
- C1

dksher on 1 Aug 2019

Broken tree Case 4:

dksher on 3 Aug 2019

@dksher I think that on your cases there were some hidden tabs between visible tabs, for example assume that there are two groups X and Y:

[X]A
[Y]D
[X]B
[Y]E
- [X]B1
- [X]B2
[X]C
- [X]C1

Could you inspect and paste the full tabs information before and after switching of tab groups? Steps:

Open debugger for Tree Style Tab and load background/background.js.
Choose the "Console" tab.
Run a script: JSON.stringify(Tab.getAllTabs().map(tab => Object.assign({}, tab.$TST.export(true), {title: null, url: null, previousUrl: null, favIconUrl: null, $TST: null, parentId: tab.$TST.parentId, children: tab.$TST.childIds }))) before switching of tab groups.
Then a result string is reported in the console, so expand it and copy-and-paste to somewhere.
Switch tab groups.
Run the script above again, and copy-and-paste the result.

piroor on 5 Aug 2019

@piroor The examples I posted were simplified and my FF actually has a lot of tabs. Does the debug string have any privacy-sensitive information? I'm a bit worried.

dksher on 5 Aug 2019

The script above removes URL, title, and favicon's URL of each tab from grabbed data. Only one sensitive data is: the internal name of containers like firefox-default, firefox-container-1, and others. Rest properties are same to the one exposed for an addon which doesn't have a permission to access privacy data. If you don't want to upload full data to here but you can expose them to me, please send it to me via email.

piroor on 5 Aug 2019

@piroor An email sent. Thanks

dksher on 5 Aug 2019

Thanks! But after inspection I've understood that I wrote wrong steps. I asked you to get two JSONs on these steps:

initial state ( => the first JSON)
switch to another group
back to the initial group ( => the second JSON)

but actually I needed JSONs of all these three steps to inspect what happened at 1-2 and 2-3. Could you collect dumped data again?

piroor on 6 Aug 2019

@piroor OK, I dumped JSONs on all the three steps and sent them via email.

dksher on 6 Aug 2019

Thanks! Things I've understood after inspection are:

There are three or more groups.
- The initial group (A): tabs 1-78
- The switched group (B): tabs 79-91
- Rest tabs: tabs 92-147
When you switch the active group from A to B, 13 tabs become visible and 78 tabs become hidden.
- Then the original tree structure of hiding 78 tabs is completely kept.
When you go back to the group A, 78 tabs become visible and 13 tabs become hidden.
- The appearing 78 tabs are moved to the top of the tab bar.
- Then the tree structure of appearing 78 tabs is partially broken.

The initial state:

* 2
* 3
* 4
* 5
  * 6
  * 7
  * 8
  * 9
  * 10
  * 11
* 12
  * 13
  * 14
  * 15
  * 16
  * 17
  * 18
  * 19
  * 20
  * 21
  * 22
  * 23
* 24
  * 25
  * 26
  * 27
  * 28
  * 29
  * 1
  * 30
  * 31
  * 32
  * 33
  * 34
  * 35
  * 36
  * 37
  * 38
  * 39
* 40
  * 41
  * 42
  * 43
  * 44
* 45
* 46
  * 47
  * 48
* 49
* 50
  * 51
  * 52
    * 53
      * 54
    * 55
* 56
* 57
  * 58
  * 59
  * 60
* 61
* 62
* 63
* 64
* 65
* 66
  * 67
* 68
  * 69
* 70
* 71
* 72
  * 73
    * 74
* 75
* 76
* 77
  * 78

----

* 79 (hidden)
* ... (hidden)
* 147 (hidden)

On the other hand, the final state is:

* 2
* 3
  * 39 (child of 24)
  * 38 (child of 24)
  * 37 (child of 24)
  * 36 (child of 24)
  * 35 (child of 24)
* 4
* 5
  * 6
  * 7
  * 8
  * 9
  * 10
  * 11
* 12
  * 13
  * 14
  * 15
  * 16
  * 17
  * 18
  * 19
  * 20
  * 21
  * 22
  * 23
* 24
  * 25
  * 26
  * 27
  * 28
  * 29
  * 1
  * 30
  * 31
  * 32
  * 33
  * 34
* 40
  * 41
  * 42
  * 43
  * 44
* 45
* 46
  * 47
  * 48
* 49
* 50
  * 51
  * 52
    * 53
      * 54
    * 55
* 56
* 57
  * 58
  * 59
  * 60
* 61
* 62
* 63
* 64
* 65
* 66
  * 67
* 68
  * 69
* 70
* 71
* 72
  * 73
    * 74
* 75
* 76
* 77
  * 78

----

* 79 (hidden)
* ...
* 147 (hidden)

As figured above, tabs 35-39 are wrongly placed before their parent, and their order is inverted.

piroor on 6 Aug 2019

Thanks for the inspection. Yes, that's the case I sent. Note that there are also other patterns of tree corruption as I mentioned above.

dksher on 6 Aug 2019

I've prepared 143 (blank) tabs and organized tree as exactly same to your case, and I've tried to switch groups. But sadly the problem you saw doesn't happen on my environment.

So I think that the problem depends on the system performance. When the system performance is enough high, TST may process moved tabs at a time. On the other hand, if the performance is not enough, something other process may be inserted between processes for moved tabs. To resolve this problem fundamentally, I think that one more abstraction layer may be required between the raw WebExtensions API and TST's current abstraction layer, like ftt.

piroor on 6 Aug 2019

Specs are Core i7 4790K plus 16GB RAM on my side, which is powerful enough for most apps including FF I believe. It might not be enough to control 140+ tabs though.

dksher on 6 Aug 2019

With the commit 9579231 I've introduced a guard for bulk moved tabs. TST tries to fix up tree structure when a tab is detected as "moved", but after the commit TST doesn't try that if neighbor tabs are not changed after the tab is moved. I hope this change is effective for moved tabs operated by Simple Tab Groups or similar add-ons.

Currently the system to create development builds automatically for each commit is stopped, so you need to build XPI by yourself or use about:debugging to try the latest codes.

piroor on 6 Aug 2019

@Drive4ik left a comment in the bug I filed on his github that may be useful to this issue: https://github.com/Drive4ik/simple-tab-groups/issues/399#issuecomment-519049043

cebas on 7 Aug 2019

With TST 3.1.7 and STG 4.0.1.2, there are no issues so far. Thank you!

dksher on 17 Aug 2019

@cebas how are things for you? If resolved, can you close this issue?