1965343 - replaceGroupWithWindow can briefly result in two non-empty tab groups with the same group ID in different windows

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Rob Wu [:robwu]]

The "Move group to new window" context menu option on a tab group is internally implemented by gBrowser.replaceGroupWithWindow, which does the following:

1. gBrowser.replaceGroupWithWindow creates a new window with the initial tab passed for adoption (source 1 and source 2)
2. browser-init dispatches "before-initial-tab-adopted" (source)
3. replaceGroupWithWindow's logic continues in response to "before-initial-tab-adopted" (source):
   • Group is created in new window, consisting of the temporary blank tab from the new window, which triggers TabGroupCreate
4. (Delay until MozAfterPaint)
5. Original tab is swapped with the temporary blank tab, and the temporary blank tab is removed (source)
   • TabGroupRemoved is finally fired.

In the description above, in the transition from step 2 to step 4, it is possible for two non-empty tab groups with the same group ID in different windows to exist. Specifically, the new window has a tab group with a blank tab, and the original window has the original tab group, and then ultimately the tabs from the old window are adopted by the new window.

I discovered this problem in https://phabricator.services.mozilla.com/D248327#8588123 (original thread started at https://phabricator.services.mozilla.com/D248327#8586345).
There I called replaceGroupWithWindow and effectively waited for TabGroupCreate to fire, and then closed the new window, with the expectation that the tab has been adopted in the new window . With --headless and --verify (or --repeat=10), the test failed due to an unexpected tab, which indicated that the tab was not really adopted. The fact that this happens suggested that tab adoption was not atomic, and as I elaborated at the start of the report, this is indeed the case.

I have since updated the patch to explicitly wait for TabGroupRemoved in the original window, but the issue can still be reproduced by removing await groupRemoved; .

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1965343 - Adopt whole tab group at once in replaceGroupWithWindow

The replaceGroupWithWindow method currently adopts a tab group in
multiple steps, asynchronously. This can result in externally observable
inconsistencies past initial adoption (see bug).

To fix this, this patch changes the adoption logic by passing the tab
group as the initial item to adopt, and adopts the whole group at once
as needed. Now the logic is similar to drag and drop adoption as
implemented in bug 1908441.

Since tabToAdopt is no longer just a tab, but also a tab group (or even
a tab group label since bug 1908441), the logic in ext-browser.js needs
to be adjusted to make sure that it does not mistake non-tab elements
for tabs. Test coverage is in browser_ext_tabGroups_move_onMoved.js,
as not changing that caused the test to fail with:

FAIL Tab did indeed move to the new window - {"oldWindowId":3,"oldPosition":"undefined"} deepEqual {"oldWindowId":3,"oldPosition":1} -

Comment 2

[Rob Wu [:robwu]]

For visibility, the patch here removes the work-around that I added in a unit test from bug 1965057. The passing of that test serves as test coverage for this bug.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f27f08cf4dcb
Adopt whole tab group at once in replaceGroupWithWindow r=tabbrowser-reviewers,sthompson

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2982254050c5
Revert "Bug 1965343 - Adopt whole tab group at once in replaceGroupWithWindow r=tabbrowser-reviewers,sthompson" for causing bc failures @browser_tab_groups.js.

Comment 5

[agoloman]

Backed out for causing bc failures @browser_tab_groups.js.

• Backout link
• Push with failures
• Failure Log

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/80cc20e2740d
Revert "Bug 1965343 - Adopt whole tab group at once in replaceGroupWithWindow r=tabbrowser-reviewers,sthompson" for causing bc failures @browser_tab_groups.js.

Comment 7

[Rob Wu [:robwu]]

(the failure happened because the old code forced a specific tab to be selected after adoption, but the new code did not. I fixed the issue in the patch and requested another review)

Comment 8

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/f27f08cf4dcb

Comment 9

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/80cc20e2740d

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:dwalker, could you have a look please?

For more information, please visit BugBot documentation.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/573a128c82a2
Adopt whole tab group at once in replaceGroupWithWindow r=tabbrowser-reviewers,sthompson

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/573a128c82a2