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

VERIFIED FIXED in Firefox 62

Status

P1
normal
VERIFIED FIXED
7 months ago
6 months ago

People

(Reporter: Oriol, Assigned: Oriol)

Tracking

unspecified
mozilla62

Firefox Tracking Flags

(firefox62 verified)

Details

Attachments

(2 attachments)

(Assignee)

Description

7 months ago
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.
(Assignee)

Comment 1

7 months ago
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.
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

7 months ago
mozreview-review
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.
Attachment #8980895 - Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Keywords: checkin-needed

Comment 6

6 months ago
Pushed by apavel@mozilla.com:
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
Keywords: checkin-needed
Priority: -- → P1

Comment 7

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/548c628a1059
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62

Comment 8

6 months ago
Created attachment 8983423 [details]
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.

Updated

6 months ago
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified

Updated

6 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.