1500180 - Listener for "tabs.onCreated" is not called for a tab opened after the last tab is closed with "browser.tabs.closeWindowWithLastTab" = "false"

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[YUKI "Piro" Hiroshi]

Attachment: testcase.xpi

# Description

When "browser.tabs.closeWindowWithLastTab" is set to "false", closing a last tab in a window will does both "removing of the last tab" and "reopening a new last tab". Those events are also notified to addons, but the event for "reopening a new last tab" operation is actually not delivered.

This problem is orifinally reported at https://github.com/piroor/treestyletab/issues/2057


# Steps to reproduce

1. Download Nightly's tar.bz2 archive and unpack it.
2. Start Nightly with a new profile.
3. Go to about:config.
4. Set "browser.tabs.closeWindowWithLastTab" to "false".
5. Go to about:debugging.
6. Load the attached testcase.xpi as a temporary addon.
7. Ctrl-Shift-J to open the browser console.
8. Ctrl-N to open a new browser window.
9. Clear all logs in the browser console.
10. CLick the "X" button of the last tab in the window opened at the step 8.

# Actual result

In the browser console, only "onRemoved" is logged like as:
----
onRemoved  3 background.js:2:45 
----

# Expected result

In the browser console, both "onRemoved" and "onCreated" are logged like as:
----
onRemoved  3 background.js:2:45 
onCreated  4 background.js:1:43 
----

# Environment

Name: Firefox
Version: 64.0a1
Build ID: 20181018123730
Update Channel: nightly
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:64.0) Gecko/20100101 Firefox/64.0
OS: Linux 4.4.0-137-generic
Multiprocess Windows: 2/2 (Enabled by default)
Web Content Processes: 4/8
Enterprise Policies: Inactive
Google Key: Found
Mozilla Location Service Key: Found
Safe Mode: false

Comment 1

[YUKI "Piro" Hiroshi]

I cannot reproduce this problem on Windows:

Name: Firefox
Version: 64.0a1
Build ID: 20181018123730
Update Channel: nightly
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0
OS: Windows_NT 10.0
Multiprocess Windows: 2/2 (Enabled by default)
Web Content Processes: 4/8
Enterprise Policies: Inactive
Google Key: Found
Mozilla Location Service Key: Found
Safe Mode: false

So this problem is possibly a platform specific.

Comment 2

[YUKI "Piro" Hiroshi]

The testcase contains just one "background.js" including following two lines:

----
browser.tabs.onCreated.addListener(tab => console.log('onCreated ', tab.id));
browser.tabs.onRemoved.addListener(tabId => console.log('onRemoved ', tabId));
----

The problem happens only on the first time of "close last tab" on a new window. On the second time, closing a last tab reports both "onRemoved" and "onCreated" logs as expected. (So unexpected error of addon-side from tabs.onRemoved listener is not a trigger of the problem.)

Comment 3

[YUKI "Piro" Hiroshi]

Bisection result by mozregression on my environment (Ubuntu):

49:24.84 INFO: No more inbound revisions, bisection finished.
49:24.84 INFO: Last good revision: 0abcb61b9fc1d858ee127f49ddc2becaf98bf3c4
49:24.84 INFO: First bad revision: 90fd055a420277d91231bb79508e8bd69b3b86f8
49:24.84 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0abcb61b9fc1d858ee127f49ddc2becaf98bf3c4&tochange=90fd055a420277d91231bb79508e8bd69b3b86f8

There is only one change: https://hg.mozilla.org/integration/autoland/rev/90fd055a4202

> Bug 1358813 - avoid flushing layout when notifying IME of focus events, r=masayuki

Comment 4

[YUKI "Piro" Hiroshi]

In the commit, there are some changes around flushing of layout. My Windows PC is newer and powerful than my Ubuntu PC, so I guess that this problem happens only on a poor environment.

Comment 5

[YUKI "Piro" Hiroshi]

I've confirmed that this problem happens on another Windows PC, so I've removed platform information of this bug.

Comment 6

[YUKI "Piro" Hiroshi]

Bisection on the another Window PC also reports that the problem is started from the commit same to Ubuntu.

And, when this problem happens, an error is reported in the console before the "onRemvoed" log:

----
this.browser is null; can't access its "frameLoader" property ext-tabs-base.js:299
get frameLoader
chrome://extensions/content/parent/ext-tabs-base.js:299:5
get frameLoader
chrome://browser/content/parent/ext-browser.js:676:5
get width
chrome://browser/content/parent/ext-browser.js:750:5
convert
chrome://extensions/content/parent/ext-tabs-base.js:615:7
convert
chrome://extensions/content/parent/ext-tabs-base.js:1842:17
listener
chrome://browser/content/parent/ext-tabs.js:406:26
emit
resource://gre/modules/ExtensionCommon.jsm:310:24
emitCreated
chrome://browser/content/parent/ext-browser.js:594:5
handleEvent/<
chrome://browser/content/parent/ext-browser.js:457:13
----

The stack is started at:
https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/browser/components/extensions/parent/ext-browser.js#456
This means that "TabOpen" DOM event for the reopened tab is correctly tracked but "tab-created" message is not emitted due to the error.

Comment 7

[YUKI "Piro" Hiroshi]

OK, I got what happens. When the problem happens:

1. "TabOpen" DOM event is handled.
2. Emitting of "tab-created" message is reserved at: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/parent/ext-browser.js#455
3. "TabClose" DOM event is handled.
4. "tab-removed" message is immediately emitted at: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/parent/ext-browser.js#470
5. Internal tab object for the closed last tab is destroyed at: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/parent/ext-browser.js#383
6. Callback function registered at the step 2 is called.
7. Firefox tries to calculate the width of the newly opened tab, but it is not opened yet, so fallback to calculation based on the current tab. But the current tab is the closing last tab, and the internal tab object for the tab has already been destroyed at the step 5, so Firefox fails to get width of the tab from already destroyed internal tab object and throws an error, and "tab-created" message for the newly open tab is never emitted.

Comment 8

[YUKI "Piro" Hiroshi]

Here is the point at the step 7 of the previous comment:
https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/toolkit/components/extensions/parent/ext-tabs-base.js#614

On a success case with the last good revision, the newly opened tab is already initialized so "result.width" is non-zero, and the tab object for already closed current tab won't be touched. On the other hand, on the failure case, the newly tab is not initialized yet and "result.width" is zero, thus the tab object for already closed current tab is unexpectedly touched.

Comment 9

[YUKI "Piro" Hiroshi]

I've tried to create a patch with a change:

----
--- a/browser/components/extensions/parent/ext-browser.js
+++ b/browser/components/extensions/parent/ext-browser.js
@@ -447,17 +447,17 @@ class TabTracker extends TabTrackerBase 
           // Save the current tab, since the newly-created tab will likely be
           // active by the time the promise below resolves and the event is
           // dispatched.
           let currentTab = nativeTab.ownerGlobal.gBrowser.selectedTab;
 
           // We need to delay sending this event until the next tick, since the
           // tab could have been created with a lazy browser but still not have
           // been assigned a SessionStore tab state with the URL and title.
-          Promise.resolve().then(() => {
+          nativeTab.ownerGlobal.requestAnimationFrame(() => {
             this.emitCreated(event.originalTarget, currentTab);
           });
         }
         break;
 
       case "TabClose":
         let {adoptedBy} = event.detail;
         if (adoptedBy) {
----

It solves this regression but introduces too many test failures...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c55f3670bac3e0cc7a07b25cca66074ff4bc45e5&selectedJob=206546946

Comment 10

[:Gijs (he/him)]

Thanks for finding this, diving into it, and offering such a thorough explanation, Piro!

I'm not a webextensions code expert, but I wonder if instead of using a fallback tab, we could just cache the geometry of the last tab in a given window about which we got an event and for which we got non-zero bounds for use in this case. It looks like when TabClose is dispatched, the tab object should have bounds.

Either that, or force a layout flush on the new tab in this edgecase, assuming that there's no animation or something like that.

Comment 11

[YUKI "Piro" Hiroshi]

Attachment: take around only geometry data of the fallback tab for pended operations

The fundamental problem of this bug is that the newly opened tab is not initialized yet when "Promise.resolve()" is resolved. Sadly I have no idea to fix it.

On the other hand, after I read related codes I realized that the "fallbackTab" is actually used only for getting its width and height. It looks no need to get size of the fallbackTab when it is finally referred. By this patch only size information is delivered to pended operations. How about this approach?

Result on tryserver:
https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=978583857be27cbe094585b02dfb90258163d632&selectedJob=206568413

Comment 12

[YUKI "Piro" Hiroshi]

Oops, I forgot to reload before posting.

(In reply to :Gijs (he/him) from comment #10)
> I'm not a webextensions code expert, but I wonder if instead of using a
> fallback tab, we could just cache the geometry of the last tab in a given
> window about which we got an event and for which we got non-zero bounds for
> use in this case. It looks like when TabClose is dispatched, the tab object
> should have bounds.

Thank you for the advice, and my patch looks doing that.

There is a concern about new regressions, and actually tehre are some failures are reported on the tryserver's result. I don't know yet why they become fail.

Comment 13

[Shane Caraveo (:mixedpuppy)]

I'd suggest fixing the getter in ext-tabs-base to:

  get frameLoader() {
    return this.browser && this.browser.frameLoader;
  }

This should result in the getter in ext-browser to do what it should.  You'll end up with zero width/height during onCreated, but it seems like that should be ok.

However, your last patch looks to cover the needed fallback items, however I think:

const frameLoader = currentTab.linkedBrowser;

Should be:

const {frameLoader} = currentTab.linkedBrowser;

Comment 14

[YUKI "Piro" Hiroshi]

Attachment: take around only geometry data of the fallback tab for pended operations (v2)

Thank you for the feedback, mixedpuppy! I've updated my patch.

Comment 15

[YUKI "Piro" Hiroshi]

Attachment: take around only geometry data of the fallback tab for pended operations (v2.1)

Fix ESLint error.

Result on tryserver:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=34f6131a63edbfcd8ed7aab6cab1b71cf628ef29&selectedJob=206867892

Comment 16

[YUKI "Piro" Hiroshi]

I've researched around failed tests and realized that I may need something fundamental solution around frame loader.

In old revisions (before this regression) "Promise.resolve()" is resoloved after the frame loader is initialized and it had effective size. On the other hand, currently "Promise.resolve()" is resolved before the frame loader has effective size. So there are two different solutions:

 A. Modifying those tests to pass with current behavior. For example: waiting until the frame loader is completely loaded, or accepting 0 size for frames. This is cosmetic, and it may break addon compatibility. I have concerns around edge cases, for example, combination of browser.tabs.onCreated and browser.webRequest.
 B. Introducing new event to be dispatched when the frame loader is initialized completely. It should be dispatched on the time same to "Promise.resolve()" in old revisions. But currently I don't know how to do this.

Which should I do?

Comment 17

[YUKI "Piro" Hiroshi]

Hmm, sorry, I misunderstood what's happen. Now I'm recreating the patch.

Comment 18

[YUKI "Piro" Hiroshi]

Attachment: take around only geometry data of the fallback tab for pended operations (v2.2)

Fix timing to backup fallback tab size.

Result on tryserver:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a00014f297860472574888898b88c8b6a76578e7
All failures are intermittent except the last one. The last ESLint error is for a garbage line I forgot to remove and it has been removed from the attached patch.

Comment 19

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9018961 [details] [diff] [review]
take around only geometry data of the fallback tab for pended operations (v2.2)

This looks good but we need a test.

Comment 20

[YUKI "Piro" Hiroshi]

This problem quite depends on platform performance, and happens only on low performance environment. I cannot imagine how "test" it on a environment which the problem doesn't happen - Gjis, do you have any idea? (or, who should I ask to?)

Comment 21

[:Gijs (he/him)]

(In reply to YUKI "Piro" Hiroshi from comment #20)
> This problem quite depends on platform performance, and happens only on low
> performance environment. I cannot imagine how "test" it on a environment
> which the problem doesn't happen - Gjis, do you have any idea? (or, who
> should I ask to?)

I assume that you could create an automated test that covers the case with the pref set and the closing of the last tab in a window - that would presumably cover this case - it might go intermittent but I'd definitely expect it to go orange on debug on infra, without your fix. Would that not work?

Comment 22

[Shane Caraveo (:mixedpuppy)]

What Gijs said is what I intended, but I don't think it should go intermittent.

Comment 23

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #22)
> What Gijs said is what I intended, but I don't think it should go
> intermittent.

Right, sorry, I think I wasn't very clear. I meant that it may not be possible to have a test that fails 100% of the time if the fix in this bug broke, but I'd expect a test to at least go intermittent and/or fail on debug only if we regress the fix.

Comment 24

[YUKI "Piro" Hiroshi]

Attachment: take around only geometry data of the fallback tab for pended operations (v2.3)

Add automated test.

Gjis, thank you for suggestion! I was too serious. I've just added a test based on the steps I told at the initial comment.

Comment 25

[YUKI "Piro" Hiroshi]

Attachment: take around only geometry data of the fallback tab for pended operations (v2.4)

Remove needless logging.

Comment 26

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9019087 [details] [diff] [review]
take around only geometry data of the fallback tab for pended operations (v2.4)


>diff --git a/browser/components/extensions/test/browser/browser_ext_tabs_events.js b/browser/components/extensions/test/browser/browser_ext_tabs_events.js

>+  async function background() {
>+    const events = [];
>+    let windowId;
>+    browser.tabs.onCreated.addListener(tab => {
>+      events.push(`onCreated@${tab.windowId}`);
>+      browser.test.assertEq(`onRemoved@${windowId}=>onCreated@${windowId}`,
>+                            events.join("=>"),
>+                            "expecting onCreated after onRemoved on same window");

Just assertEq(windowId, tab.windowId, ...
No need for "events".
Also test that you get the width/height, probably sufficient to test that they are non-zero unless you want to save the dimensions from onRemoved.

>+    browser.tabs.onRemoved.addListener((tabId, info) => {
>+      events.push(`onRemoved@${info.windowId}`);
>+      windowId = info.windowId;
>+      browser.test.assertEq(`onRemoved@${windowId}`,
>+                            events.join("=>"),
>+                            "expecting onRemoved before onCreated");

Just save windowId, no need for a test.  The onCreated test will fail if onRemoved doesn't happen first.

r+ with changes.

Comment 27

[YUKI "Piro" Hiroshi]

Attachment: take around only geometry data of the fallback tab for pended operations (v2.5)

Updated test.

Thank you for reviewing.

(In reply to Shane Caraveo (:mixedpuppy) from comment #26)
> The onCreated test will fail if
> onRemoved doesn't happen first.

It is intentional, because the order of these events should be compatible to old versions of Firefox.

* I've confirmed that onRemoved happens before onCreated on this case,
  with Firefox ESR60 and Firefox 62.
* Chrome doesn't provide similar config, so I think that the behavior
  on old Firefox looks a reasonable reference.

Comment 28

[Shane Caraveo (:mixedpuppy)]

(In reply to YUKI "Piro" Hiroshi from comment #27)

> (In reply to Shane Caraveo (:mixedpuppy) from comment #26)
> > The onCreated test will fail if
> > onRemoved doesn't happen first.
> 
> It is intentional, because the order of these events should be compatible to
> old versions of Firefox.

Sorry, I meant that as a reason that you didn't need a test statement in onRemoved.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa0bea21d616
Take around only geometry data of the fallback tab for pended operations. r=mixedpuppy

Comment 30

[Arthur Iakab [arthur_iakab]]

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

Comment 31

[YUKI "Piro" Hiroshi]

Comment on attachment 9019215 [details] [diff] [review]
take around only geometry data of the fallback tab for pended operations (v2.5)

I've confirmed that I can apply the patch to the mozilla-beta as-is.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1358813

User impact if declined: This regression breaks compatibility with addons which listening tab events.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Changes are enclosed around codes for WebExtensions API implementation. Tab opening/closing of Firefox itself is not affected.

String changes made/needed: nothing

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9019215 [details] [diff] [review]
take around only geometry data of the fallback tab for pended operations (v2.5)

Fixes a compatibility issue for addons listening for tab events. Approved for 64.0b4.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cad7fc1b8cf2

Comment 34

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I managed to successfully reproduce the initial issue using 64.0a1 (2018-10-18). I can confirm that 65.0a1 (2018-10-24) is verified fixed across platforms (Windows 10 x64, Ubuntu 16.04 x64, macOS 10.12.6).

Comment 35

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

The fix is successfully applied on 64.0b4 build2 (20181025233934), too. Confirmed this using Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13.6.