regression: both tabs.onCreated and tabs.onMoved are unexpectedly notified for new tab opened from link

RESOLVED FIXED in Firefox 60

Status

defect
P1
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: yuki, Assigned: mixedpuppy)

Tracking

(Blocks 1 bug, {regression})

Trunk
mozilla61
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60+ fixed, firefox61 unaffected)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
On Google Chrome and Firefox 59, middle-click on a link opens a tab next to the current tab and notifies only a "tabs.onCreated" event with correct "index". On the other hand, Firefox 60 beta and Nightly 61 notify "tabs.onCreated" with wrong "index" and extra "tabs.onMoved" event is also notified.

Steps to reproduce:
1. Start Firefox.
2. Go to "about:config" and set "browser.tabs.insertRelatedAfterCurrent"
   to "true".
3. Install any addon which has "tabs" permission.
   Assume that you install Tab Center Redux:
   https://addons.mozilla.org/ja/firefox/addon/tab-center-redux/.
4. Open remote debugger for the addon.
5. Choose "Console" tab and run following script:
   ------------
   chrome.tabs.onCreated.addListener(tab=>
     console.log(`tabs.onCreated, index=${tab.index}`));
   chrome.tabs.onMoved.addListener((id, info)=>
     console.log(`tabs.onMoved, from ${info.fromIndex} to ${info.toIndex}`));
   ------------
6. Prepare two tabs A(index=0) and B(index=1) with something webpage.
   Assume that you open "https://www.mozilla.org/en-US/firefox/".
7. Select the tab A and middle-click on any link in the page.
   Assume that you click the link "Get Involved".

Actual result:
The console says:
> tabs.onCreated, index=2 
> tabs.onMoved, from 2 to 1

Expected result:
The console says:
> tabs.onCreated, index=1
(Reporter)

Comment 1

a year ago
I've tested on some different browser versions.

Firefox ESR52.7.2 on Ubuntu 16.04LTS(64bit):
> tabs.onCreated, index=1

Firefox 59.0 on Ubuntu 16.04LTS(64bit):
> tabs.onCreated, index=1

Firefox 60.0b4 on Ubuntu 16.04LTS(64bit):
> tabs.onCreated, index=2 
> tabs.onMoved, from 2 to 1

Nightly 61.0a1 on Ubuntu 16.04LTS(64bit):
> tabs.onCreated, index=2 
> tabs.onMoved, from 2 to 1

Google Chrome 52.0.2743.116 on Ubuntu 16.04LTS(64bit):
> tabs.onCreated, index=1
(Reporter)

Updated

a year ago
(Reporter)

Comment 2

a year ago
This regression affects to some addons which provides custom tab UI by tracking events on browser tabs. Actually Tree Style Tab addon (*) fails to detect a new tab is opened next to the current or not, and TST misdetects the tab as "unexpectedly moved before tracked by TST itself". As the result TST wrongly moves the tab to unexpected position. That appears as a visual glitch for users.

* https://addons.mozilla.org/firefox/addon/tree-style-tab/
(Reporter)

Comment 3

a year ago
The "TabOpen" custom DOM event triggers "tabs.onCreated" WE event on both Firefox ESR52 and Nightly 61:

https://dxr.mozilla.org/mozilla-esr52/rev/241da74b33dc3148c96002ebc128fd4353cbb07a/browser/components/extensions/ext-tabs.js#114
https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/browser/components/extensions/ext-browser.js#337

Comments on both versions say that wait until next tick to determine correct index of newly opened tab. However, the hack doesn't work as expected on Nightly 61. To confirm that, run following script on the browser console (Ctrl-Shift-J):

----
window.addEventListener('TabOpen', () => { Promise.resolve().then(() => console.log('TabOpen')); }, { once: true });
window.addEventListener('TabMove', () => { console.log('TabMove') }, { once: true });
----

 * Firefox 59.0.1 says: TabMove => TabOpen.
 * Nightly 61 says: TabOpen => TabMove.

To get a result same to the one on ESR52, I need to use different hack like:

----
window.addEventListener('TabOpen', () => { requestAnimationFrame(() => console.log('TabOpen')); }, { once: true });
window.addEventListener('TabMove', () => { console.log('TabMove') }, { once: true });
----

By the way, from this result I think that all hacky codes using 'Promise.resolve().then()' to do something at the next tick may be affected.
(Reporter)

Comment 4

a year ago
More research. Testing code:
----
window.addEventListener('TabOpen', () =>
  { console.log('TabOpen');
    Promise.resolve().then(() => console.log('TabOpen (next tick)')); }, { once: true });
window.addEventListener('TabMove', () =>
  { console.log('TabMove');
    Promise.resolve().then(() => console.log('TabMove (next tick)')); }, { once: true });
----

Result on ESR52:
    TabOpen
 => TabMove
 => TabOpen (next tick)
 => TabMove (next tick)

Result on Nightly 61:
    TabOpen
 => TabOpen (next tick)
 => TabMove
 => TabMove (next tick)
(Reporter)

Comment 5

a year ago
The most easiest way to solve this API incompatibility is: ensure dispatch a "tabs.onCreated" event after the new tab is completely moved. This patch introduces new custom event, and the event tracker waits until the new custom event is dispatched, instead of simple "Promise.resolve()" hack. 

pros: easy to implement, certainly works.
cons: new DOM event may make things more slower.

How about this approach?
(Assignee)

Comment 6

a year ago
Comment on attachment 8960240 [details] [diff] [review]
Example solution with new custom event

Adding an additional event is not desirable.  Based on the comment for TabOpen, it seems that the move should happen prior to the the event.  Alternately, TabOpen could be moved to the end of the tab operation.
Attachment #8960240 - Flags: feedback-
(Assignee)

Comment 7

a year ago
Lets get some feedback specifically on moving some of this logic around.  Either moving the tab prior to the event, or moving the event to happen after the move.  I'm inclined towards the former as it will keep TabOpen consistent with how it works now (ie. prior to loadURI), except that it will end up with the correct index.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(gijskruitbosch+bugs)

Comment 8

a year ago
Can you find out when this broke using mozregression? ( https://mozilla.github.io/mozregression/ )

I expect this will have something to do with either the Promise/microtask changes or the switch from XBL to plain JS for the tabbrowser.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(yuki)
From handling "TabOpen" event [1]

        // We need to delay sending this event until the next tick, since the
        // tab does not have its final index when the TabOpen event is dispatched.
        Promise.resolve().then(() => {


Promise.resolve().then() isn't processed in the next tick with the new microtask handling, IIUC.

[1] https://hg.mozilla.org/mozilla-central/file/bfb7edfd0436/browser/components/extensions/ext-browser.js#l354
(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> From handling "TabOpen" event [1]
> 
>         // We need to delay sending this event until the next tick, since the
>         // tab does not have its final index when the TabOpen event is
> dispatched.
>         Promise.resolve().then(() => {
> 
> 
> Promise.resolve().then() isn't processed in the next tick with the new
> microtask handling, IIUC.
> 
> [1]
> https://hg.mozilla.org/mozilla-central/file/bfb7edfd0436/browser/components/
> extensions/ext-browser.js#l354

looks like there are several places that uses this idiom,
maybe we should check each case to see if expected things are still happening.
  https://searchfox.org/mozilla-central/search?q=Promise.resolve().then(()
(Reporter)

Comment 11

a year ago
(In reply to :Gijs from comment #8)
> Can you find out when this broke using mozregression? (
> https://mozilla.github.io/mozregression/ )

Thank you for navigation. I've found the first build which has the regression.

last good:
  build_date: 2018-03-04 23:15:39.139000
  build_type: inbound
  changeset: c842abb7cfbf39e0c1cfb9a1f3a1d6415cd10744
  pushlog_url: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c842abb7cfbf39e0c1cfb9a1f3a1d6415cd10744&tochange=84ea422093dd615bd191a3e579ed299d96c802f1

first bad: 
  build_date: 2018-03-05 04:41:13.400000
  build_type: inbound
  changeset: 69f9ae4f6e8943d24569a9e4380f6df869f56b73
  pushlog_url: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=190b536928f8a8ca96e52101d2013c88a1a66384&tochange=69f9ae4f6e8943d24569a9e4380f6df869f56b73

The bug 1193394 is in the list of commits of the first bad build. The result matches to the comment #9:

> Promise.resolve().then() isn't processed in the next tick with the new
> microtask handling, IIUC.
Flags: needinfo?(yuki)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> From handling "TabOpen" event [1]
> 
>         // We need to delay sending this event until the next tick, since the
>         // tab does not have its final index when the TabOpen event is
> dispatched.
>         Promise.resolve().then(() => {
> 
> 
> Promise.resolve().then() isn't processed in the next tick with the new
> microtask handling, IIUC.

That is unfortunately the case, but only when we don't wind up in a microtask when the tab is created. Which we often aren't, as a side-effect of the weird way we handle microtasks in chrome code. Content code can always depend on being in a microtask when its events are dispatched by JS callers in the same document, but that's not currently the case for chrome code. :(
Flags: needinfo?(kmaglione+bmo)
Yeah, unfortunate.  I think a proper solution is what Shane mentioned in comment 7.   I am wondering there is any side effects if we move the tab before dispatching 'TabOpen' event.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Yeah, unfortunate.  I think a proper solution is what Shane mentioned in
> comment 7.   I am wondering there is any side effects if we move the tab
> before dispatching 'TabOpen' event.

I think that's probably the correct solution in this case. It's probably what I would have done in the first place if I didn't have legacy add-ons to worry about.

But I also think that we should fix this footgun for chrome callers. I was aware of the spec behavior when I wrote the original code, and anyone else aware of the spec behavior would expect it to work as intended. If we don't do something about it, we're going to keep running into more of these kinds of issues.
(Assignee)

Updated

a year ago
Assignee: nobody → mixedpuppy
Priority: -- → P1

Updated

a year ago
See Also: → 1445828
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Depends on: 1344749
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Yeah, unfortunate.  I think a proper solution is what Shane mentioned in
> comment 7.   I am wondering there is any side effects if we move the tab
> before dispatching 'TabOpen' event.

The side effect would be dispatching TabMove before TabOpen, which to me doesn't seem sane at all, since consumers typically start tracking a tab after TabOpen but might also have a TabMove listener on the tab bar before that.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8961523 - Attachment is obsolete: true
(Assignee)

Comment 18

a year ago
Comment on attachment 8961827 [details]
Bug 1446913 - finalize tab index prior to TabOpen

(In reply to Dão Gottwald [::dao] from comment #16)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> > Yeah, unfortunate.  I think a proper solution is what Shane mentioned in
> > comment 7.   I am wondering there is any side effects if we move the tab
> > before dispatching 'TabOpen' event.
> 
> The side effect would be dispatching TabMove before TabOpen, which to me
> doesn't seem sane at all, since consumers typically start tracking a tab
> after TabOpen but might also have a TabMove listener on the tab bar before
> that.

I was looking at the best way to address that.  I think from a tab creation pov, we shouldn't do both open and move events, we only need the open event with the correct position.

We could either refactor movetabto a whole bunch, or as the last patch here, just skip the move event.
Attachment #8961827 - Flags: feedback?(dao+bmo)
Comment on attachment 8961827 [details]
Bug 1446913 - finalize tab index prior to TabOpen

(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> Comment on attachment 8961827 [details]
> Bug 1446913 - finalize tab index prior to TabOpen
> 
> (In reply to Dão Gottwald [::dao] from comment #16)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> > > Yeah, unfortunate.  I think a proper solution is what Shane mentioned in
> > > comment 7.   I am wondering there is any side effects if we move the tab
> > > before dispatching 'TabOpen' event.
> > 
> > The side effect would be dispatching TabMove before TabOpen, which to me
> > doesn't seem sane at all, since consumers typically start tracking a tab
> > after TabOpen but might also have a TabMove listener on the tab bar before
> > that.
> 
> I was looking at the best way to address that.  I think from a tab creation
> pov, we shouldn't do both open and move events, we only need the open event
> with the correct position.
> 
> We could either refactor movetabto a whole bunch, or as the last patch here,
> just skip the move event.

I think that's okay but we should:

- audit our TabOpen and TabMove listeners for this API change
- introduce read-only tab.index to make that the blessed way to get the index right after TabOpen. This can just use _tPos under the hood.
Attachment #8961827 - Flags: feedback?(dao+bmo)

Updated

a year ago
Blocks: 1442843
(Assignee)

Comment 21

a year ago
(In reply to Dão Gottwald [::dao] from comment #19)

> - audit our TabOpen and TabMove listeners for this API change

I started looking over all the addtab/movetabto usage.  There is a lot going on here, quite often addtab/movetabto calls are made, sometimes with pintab (which also calls movetabto) in-between them.  I think this calls for some kind of refactoring that is outside the scope of this bug.  The try passes tests, do we need more for this?
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 22

a year ago
I'm writing up tests for the current patch, but am actually having a problem reproducing the problem where both onCreated and onMoved are called.  onMoved specifically ignores onMoved after onCreated, added with bug 1350522.

https://searchfox.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#417
Flags: needinfo?(yuki)
I wasn't concerned about the callers but about the TabOpen and TabMove listeners that might expect the current order of events.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 24

a year ago
(In reply to Dão Gottwald [::dao] from comment #23)
> I wasn't concerned about the callers but about the TabOpen and TabMove
> listeners that might expect the current order of events.

I didn't see anything outside of webext that looks at the order.  I started digging into why we do that which is where i started seeing all the crazy calls going on.  Basically we ignore the first tabmove after tabopen in one of the apis.  That can remain, it's unaffected by this change.  The primary motivation at this point is to get a correct tabid in the tabopen call under this particular scenario.
(Reporter)

Comment 25

a year ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #22)
> I'm writing up tests for the current patch, but am actually having a problem
> reproducing the problem where both onCreated and onMoved are called. 
> onMoved specifically ignores onMoved after onCreated, added with bug 1350522.
> 
> https://searchfox.org/mozilla-central/source/browser/components/extensions/
> ext-tabs.js#417

I'm just focusing to the compatibility of WebExtensions API, and I've verified that the bug I originally reported has been fixed, with this testing script (see also my initial comment. Note that you need to run this script in a remote debugger for an addon having "tabs" permission):

----
chrome.tabs.onCreated.addListener(tab=>
  console.log(`tabs.onCreated, index=${tab.index}`));
chrome.tabs.onMoved.addListener((id, info)=>
  console.log(`tabs.onMoved, from ${info.fromIndex} to ${info.toIndex}`));
----

All results are same to expected for me.

 * only "tabs.onCreated" should be reported with correct index
   * new tab opened at next to the current, by middle-click on a link
   * new tab opened at the end of the tab bar by "New Tab" button
 * only "tabs.onMoved" should be reported
   * already opened tab is moved in a window
 * both "tabs.onCreated" nor "tabs.onMoved" should be reported
   * already opened tab is moved to a new window ("detach" action)
   * already opened tab is moved across windows ("attach" action)
 * both "tabs.onCreated" and "tabs.onMoved" are reported: this should not happen, because it is incompatible to old Firefox and Google Chrome.

Does this comment helps you?
Flags: needinfo?(yuki)
(Reporter)

Comment 26

a year ago
Sorry, there is a typo:

(In reply to YUKI "Piro" Hiroshi from comment #25)
>  * both "tabs.onCreated" nor "tabs.onMoved" should be reported

Correct:

 * both "tabs.onCreated" nor "tabs.onMoved" should *NOT* be reported
(Assignee)

Comment 27

a year ago
(In reply to YUKI "Piro" Hiroshi from comment #25)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #22)
> > I'm writing up tests for the current patch, but am actually having a problem
> > reproducing the problem where both onCreated and onMoved are called. 
> > onMoved specifically ignores onMoved after onCreated, added with bug 1350522.
> > 
> > https://searchfox.org/mozilla-central/source/browser/components/extensions/
> > ext-tabs.js#417
> 
> I'm just focusing to the compatibility of WebExtensions API, and I've
> verified that the bug I originally reported has been fixed

I don't know how it was broken.  There are places where more than one TabMove event will occur, but not AFAIK from clicking to open a tab.

> , with this
> testing script (see also my initial comment. Note that you need to run this
> script in a remote debugger for an addon having "tabs" permission):

Do these problems happen otherwise?  ie. when you are not using remote debugging?


> ----
> chrome.tabs.onCreated.addListener(tab=>
>   console.log(`tabs.onCreated, index=${tab.index}`));
> chrome.tabs.onMoved.addListener((id, info)=>
>   console.log(`tabs.onMoved, from ${info.fromIndex} to ${info.toIndex}`));
> ----
> 
> All results are same to expected for me.
> 
>  * only "tabs.onCreated" should be reported with correct index
>    * new tab opened at next to the current, by middle-click on a link
>    * new tab opened at the end of the tab bar by "New Tab" button
>  * only "tabs.onMoved" should be reported
>    * already opened tab is moved in a window
>  * both "tabs.onCreated" nor "tabs.onMoved" should be reported
>    * already opened tab is moved to a new window ("detach" action)
>    * already opened tab is moved across windows ("attach" action)
>  * both "tabs.onCreated" and "tabs.onMoved" are reported: this should not
> happen, because it is incompatible to old Firefox and Google Chrome.
> 
> Does this comment helps you?

No.  It seems like you are saying that you get both onCreated and onMoved for all the cases above that last point.  But you also said your original report is "fixed", but the original report is covered in the list above.  how about you outline it like this:

* new tab opened at next to the current, by middle-click on a link
   - expect: onCreated
   - actual: ?
   - chrome: onCreated
   - new regression: yes or no ?
     - firefox versions affected: ?

* new tab opened at the end of the tab bar by "New Tab" button
   - expect: onCreated
   - actual: ?
   - chrome: onCreated
   - new regression: yes or no ?

* already opened tab is moved in a window
   - expect: onMoved
   - actual: ?
   - chrome: onMoved
   - new regression: yes or no ?

* already opened tab is moved to a new window ("detach" action)
   - expect: onCreated + onMoved
   - actual: ?
   - chrome: onCreated + onMoved
   - new regression: yes or no ?

* already opened tab is moved across windows ("attach" action)
   - expect: onCreated + onMoved
   - actual: ?
   - chrome: onCreated + onMoved
   - new regression: yes or no ?
(Reporter)

Comment 28

a year ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #27)
> > , with this
> > testing script (see also my initial comment. Note that you need to run this
> > script in a remote debugger for an addon having "tabs" permission):
> 
> Do these problems happen otherwise?  ie. when you are not using remote
> debugging?

Yes, an actual addon Tree Style Tab is affected. I described steps to reproduce based on the remote debugger, for non-addon developers. If you include the testing script to your addon you'll see just same result.

> No.  It seems like you are saying that you get both onCreated and onMoved
> for all the cases above that last point.  But you also said your original
> report is "fixed", but the original report is covered in the list above. 
> how about you outline it like this:

Sorry I'm missing what is the point. Because something changes landed to both mozilla-beta and mozilla-central branches, the term "regression" now means two status:

 A. regression described at my initial comment.
 B. regression triggered by recent changes landed after my initial comment.

So, I try to re-explain all cases based on the latest builds.

Script to run on the remote debugger:
----
chrome.tabs.onCreated.addListener(tab=>
  console.log(`tabs.onCreated, index=${tab.index}`));
chrome.tabs.onMoved.addListener((id, info)=>
  console.log(`tabs.onMoved, from ${info.fromIndex} to ${info.toIndex}`));
chrome.tabs.onDetached.addListener((id, info)=>
  console.log(`tabs.onDetached, index=${info.oldPosition}`));
chrome.tabs.onAttached.addListener((id, info)=>
  console.log(`tabs.onAttached, index=${info.newPosition}`));
----
To run the code above on a remote debugger, I installed Tab Session Manager addon which includes "tabs" permission:
https://addons.mozilla.org/firefox/addon/tab-session-manager/

Firefox versions:
mozilla-esr52: Firefox 52.7.3
mozilla-release=Firefox 59.0.1
mozilla-beta=Firefox 60.0b7, build ID=20180326164103
mozilla-central=Nightly 61.0a1, build ID=20180327220126

* new tab opened at next to the current, by middle-click on a link
  (regression described at the initial comment)
   - expect on mozilla-esr52:   onCreated
   - expect on mozilla-release: onCreated
   - actual on mozilla-beta:    onCreated + onMoved
     - regression A: yes (regression described at my initial comment)
     - regression B: no
   - actual on mozilla-central: onCreated
     - regression A: no (maybe fixed by something recent changes)
     - regression B: no
   - chrome:                    onCreated

* new tab opened at the end of the tab bar by "New Tab" button
  (regression described at the initial comment)
   - expect on mozilla-esr52: onCreated
   - expect on mozilla-release: onCreated
   - actual on mozilla-beta:    onCreated
     - regression A: no
     - regression B: no
   - actual on mozilla-central: onCreated
     - regression A: no
     - regression B: no
   - chrome:                    onCreated

* already opened tab is moved in a window
  (not described at the initial comment)
   - expect on mozilla-esr52:   onMoved
   - expect on mozilla-release: onCreated
   - actual on mozilla-beta:    onMoved
     - regression A: no
     - regression B: no
   - actual on mozilla-central: onMoved
     - regression A: no
     - regression B: no
   - chrome:                    onMoved

* already opened tab is moved to a new window ("detach" action)
  (not described at the initial comment)
   - expect on mozilla-esr52:   onDetached + onAttached
   - expect on mozilla-release: onDetached + onAttached
   - actual on mozilla-beta:    onDetached + onAttached
     - regression A: no
     - regression B: no
   - actual on mozilla-central: onDetached + onAttached
     - regression A: no
     - regression B: no
   - chrome:                    onDetached + onAttached

* already opened tab is moved across windows ("attach" action)
  (not described at the initial comment)
   - expect on mozilla-esr52:   onDetached + onAttached
   - expect on mozilla-release: onDetached + onAttached
   - actual on mozilla-beta:    onDetached + onAttached
     - regression A: no
     - regression B: no
   - actual on mozilla-central: onDetached + onAttached
     - regression A: no
     - regression B: no
   - chrome:                    onDetached + onAttached
(Reporter)

Comment 29

a year ago
I'm very sorry that there are some mistakes in my comment #28. Updated version:

* new tab opened at next to the current, by middle-click on a link
  (regression described at the initial comment)
   - expect on mozilla-esr52:   onCreated
   - expect on mozilla-release: onCreated
   - actual on mozilla-beta:    onCreated + onMoved
     - regression A: yes
     - regression B: no
   - actual on mozilla-central: onCreated
     - regression A: no (maybe fixed by something recent changes)
     - regression B: no
   - chrome:                    onCreated

* new tab opened at the end of the tab bar by "New Tab" button
  (regression described at the initial comment)
   - expect on mozilla-esr52:   onCreated
   - expect on mozilla-release: onCreated
   - actual on mozilla-beta:    onCreated
     - regression A: no
     - regression B: no
   - actual on mozilla-central: onCreated
     - regression A: no
     - regression B: no
   - chrome:                    onCreated

* already opened tab is moved in a window
  (not described at the initial comment)
   - expect on mozilla-esr52:   onMoved
   - expect on mozilla-release: onMoved
   - actual on mozilla-beta:    onMoved
     - regression A: no
     - regression B: no
   - actual on mozilla-central: onMoved
     - regression A: no
     - regression B: no
   - chrome:                    onMoved

* already opened tab is moved to a new window ("detach" action)
  (not described at the initial comment)
   - expect on mozilla-esr52:   onDetached + onAttached
   - expect on mozilla-release: onDetached + onAttached
   - actual on mozilla-beta:    onDetached + onAttached
     - regression A: no
     - regression B: no
   - actual on mozilla-central: onDetached + onAttached
     - regression A: no
     - regression B: no
   - chrome:                    onDetached + onAttached

* already opened tab is moved across windows ("attach" action)
  (not described at the initial comment)
   - expect on mozilla-esr52:   onDetached + onAttached
   - expect on mozilla-release: onDetached + onAttached
   - actual on mozilla-beta:    onDetached + onAttached
     - regression A: no
     - regression B: no
   - actual on mozilla-central: onDetached + onAttached
     - regression A: no
     - regression B: no
   - chrome:                    onDetached + onAttached
(Assignee)

Comment 30

a year ago
This test replicates the reported str.  It passes on nightly and fails on beta.
(Assignee)

Updated

a year ago
See Also: → 1449700
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8961827 - Attachment is obsolete: true
(Assignee)

Comment 33

a year ago
Addressing the tabadd/move issue has been moved to bug 1449700.

This bug will now address the beta-specific problem.  The first patch (test-only) is intended for both m-c and m-b.

Comment 34

a year ago
mozreview-review
Comment on attachment 8963303 [details]
Bug 1446913 beta-only workaround microtask on content clicking,

https://reviewboard.mozilla.org/r/232200/#review237764

::: browser/modules/ContentClick.jsm:23
(Diff revision 1)
>  var ContentClick = {
>    // Listeners are added in nsBrowserGlue.js
>    receiveMessage(message) {
>      switch (message.name) {
>        case "Content:Click":
> +        Promise.resolve().then(() => {

Please add a comment about why we're doing this.
Attachment #8963303 - Flags: review?(kmaglione+bmo) → review+

Comment 35

a year ago
mozreview-review
Comment on attachment 8963302 [details]
Bug 1446913 - test TabOpen/TabMove event sequence during content click,

https://reviewboard.mozilla.org/r/232198/#review237766
Attachment #8963302 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 41

a year ago
Comment on attachment 8963303 [details]
Bug 1446913 beta-only workaround microtask on content clicking,

Approval Request Comment
[Feature/Bug causing the regression]: 1193394
[User impact if declined]: webextension tab events do not happen correctly
[Is this code covered by automated tests?]: yes.  tests and one base fix have been pushed to inbound.  Those will need to be uplifted (see comment 40).  This patch will then need to be added for beta only.
[Has the fix been verified in Nightly?]: The tests will run on nightly, but the bug is beta-only.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: patches pushed in comment 40 
[Is the change risky?]: moderately
[Why is the change risky/not risky?]: this does affect all middle clicks, but tests are all passing
[String changes made/needed]: none
Attachment #8963303 - Flags: approval-mozilla-beta?
(Assignee)

Updated

a year ago
Attachment #8963672 - Flags: review?(kmaglione+bmo)

Comment 42

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7bbada38055f
https://hg.mozilla.org/mozilla-central/rev/7dc68125762c
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8963303 [details]
Bug 1446913 beta-only workaround microtask on content clicking,

Fix a new WebExtension regression in Fx60. Approved for 60.0b9.
Attachment #8963303 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 45

a year ago
You missed the beta-only patch, which is the actual fix.
Flags: needinfo?(csabou)
I already told him on IRC. He's fixing it.
Flags: needinfo?(csabou) → in-testsuite+
(In reply to Shane Caraveo (:mixedpuppy) from comment #41)
> [Is this code covered by automated tests?]: yes.  tests and one base fix
> have been pushed to inbound.  Those will need to be uplifted (see comment
> 40).  This patch will then need to be added for beta only.
> [Has the fix been verified in Nightly?]: The tests will run on nightly, but
> the bug is beta-only.
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Shane's assessment on manual testing needs and the fact that this fix has automated tests.
Flags: qe-verify-

Updated

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