1463751 - Tab-specific data not updated when tab is moved to another window and old window closes

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P1)

Description

[Oriol Brufau [:Oriol]]

The fix in bug 1451176 assumed that the "tab-detached" event would be emitted before "tab-select". This is not the case when the tab that is moved between windows is the last one of the old window, and thus the window closes.

 1. Install https://bugzilla.mozilla.org/attachment.cgi?id=8964946 in about:debugging
 2. You need at least two open windows, one of which with a single tab
 3. Click the browser action in that window, it will display a tab-specific badge.
 4. Drag the tab into another window, the old window will close.

Result: the badge is not displayed. But if you select another tab, and go back to the former one, then the button will be updated and the badge will be displayed.

It seems this is because "tab-detached" events can be emitted by two different codes:

https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/browser/components/extensions/parent/ext-browser.js#389
https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/browser/components/extensions/parent/ext-browser.js#479

In the second one, it's too late.

Comment 1

[Oriol Brufau [:Oriol]]

My first attempt was with "tab-attached". It seems to be emitted before "tab-select", but "tab-attached" is delayed a tick in order to have the tab index. This was probably a race condition, and my test failed due to this delay (the values were checked before being updated).

So now I'm trying adding a new "tab-adopted" event. Seems to work, will post a patch soon.

Comment 2

[Oriol Brufau [:Oriol]]

Attachment: Bug 1463751 - Tab-specific data not updated when tab is moved to another window and old window closes

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

Comment 3

[Oriol Brufau [:Oriol]]

Comment on attachment 8980895 [details]
Bug 1463751 - Tab-specific data not updated when tab is moved to another window and old window closes

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

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8980895 [details]
Bug 1463751 - Tab-specific data not updated when tab is moved to another window and old window closes

https://reviewboard.mozilla.org/r/247070/#review254022

Just a few nits below, primarily cleanup some comments and code documentation.

::: browser/components/extensions/parent/ext-browser.js:191
(Diff revision 2)
>      // fromBrowse will be false in case of e.g. a hash change or history.pushState
>      let fromBrowse = !(flags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT);
>      this.emit("location-change", tab, fromBrowse);
>    }
>  
> -  tabDetached(eventType, {nativeTab, adoptedBy}) {
> +  tabAdopted(eventType, {nativeTab, adoptedTab}) {

Doc string on this function.  Seeing the changes below, I'd like to change the args to something like {adoptingTab, adoptedTab} or {newTab, oldTab}

::: browser/components/extensions/parent/ext-browser.js:312
(Diff revision 2)
>  
> +  adopt(nativeTab, adoptedTab) {
> +    if (!this.adoptedTabs.has(adoptedTab)) {
> +      this.adoptedTabs.set(adoptedTab, nativeTab);
> +      this.setId(nativeTab, this.getId(adoptedTab));
> +      this.emitAdopted(nativeTab, adoptedTab);

Seems the only place this is called, just call this.emit here.

::: browser/components/extensions/parent/ext-browser.js:376
(Diff revision 2)
>          let {adoptedTab} = event.detail;
>          if (adoptedTab) {
> -          this.adoptedTabs.set(adoptedTab, event.target);
> -
>            // This tab is being created to adopt a tab from a different window.
>            // Copy the ID from the old tab to the new.

this comment no longer makes sense, ditto with comments in other areas where adopt is now called.

Comment 5

[Oriol Brufau [:Oriol]]

Comment on attachment 8980895 [details]
Bug 1463751 - Tab-specific data not updated when tab is moved to another window and old window closes

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/548c628a1059
Tab-specific data not updated when tab is moved to another window and old window closes r=mixedpuppy

Comment 7

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/548c628a1059

Comment 8

[marius.santa]

Attachment: tab specific data.gif

Reproduced in Firefox 62.0a1 (20180523100147).
Retested and verified in Firefox 62.0a1 (20180605100102) on Windows 10 64Bit and MacOS 10.13.4.