Closed Bug 1650956 Opened 4 years ago Closed 3 years ago

"Undo Close Tabs" does not restore multiple tabs when "tabs.remove()" closed multiple tabs

Categories

(WebExtensions :: Frontend, enhancement, P3)

78 Branch
enhancement

Tracking

(firefox85 fixed)

RESOLVED FIXED
85 Branch
Tracking Status
firefox85 --- fixed

People

(Reporter: joe, Assigned: liz, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

  1. Open multiple tabs
  2. Close multiple tabs using the "tabs.remove(tabIds)" API, where tabIds is an array of tab IDs to close
  3. Open tab context menu and click "Undo Close Tab"

Actual results:

The "Undo Close Tab" menu label failed to change to "Undo Close Tabs", and when clicked only opened 1 of the closed tabs.

Expected results:

The "Undo Close Tab" menu item label should have updated to read "Undo Close Tabs", and when clicked should have re-opened all of the tabs that were closed by "tabs.remove()".

(I've attached an image with a more graphical breakdown of what's happening in case my explanation is unclear -- please let me know if there's anything I can do to further clarify the situation)

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → Tabbed Browser

Hello! I have tried to reproduce the issue following the STR provided in the description but I was unable to reproduce it using Ubuntu 18.04 LTS with Firefox 78.0.2 and Firefox Nighlty 80.0a1 (2020-07-06).
If you could answer these questions

  1. Does the issue reproduce with a new profile? Here is link to how you can create a new profile: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Multiple_profiles
  2. Do you use addons and if so can you provide a list with them?
  3. Is the issue reproducing on the latest Nightly version? Here is a link from where you can download it: https://nightly.mozilla.org/

Thank you!

Flags: needinfo?(joe)

Hi Negritas,

Just a reminder that this issue is dealing with how the "Undo Close Tabs" feature interacts with tabs closed via the tabs.remove() API, so if you tried to reproduce this issue without using an add-on that calls "tabs.remove()", that would explain why you cannot reproduce the issue.

To answer your questions:

  1. Yes, the issue occurs with a new profile (once an applicable extension is installed). It also happens with a fresh install of Firefox.
  2. Yes, I use add-ons. This issue revolves around how add-ons interact with the "Undo Close Tabs". You probably will not be able to reproduce this issue without using an add-on of some sort.
  3. I'm not sure whether this applies to the latest nightly version, but I'm reluctant to install it except as a last resort.

To reiterate, this issue occurs when I try to use any add-on which uses the tabs.remove() API (see https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/remove). To reproduce this issue, please install an add-on which uses the "tabs.remove()" API. Here are a few I'm aware of that you could use:

(Disclaimer: these are all extensions I've written, though I'm sure there are plenty written by others that this applies to)

Each of these add-ons adds a menu item to the tab context menu to close tabs. When you click any of these items, it calls tabs.remove(tabs), where tabs is an array of tabs to close. All of the tabs are being closed in a single call to tabs.remove(). The expected behavior is for the "Undo Close Tab" menu to read "Undo Close Tabs", and clicking it should cause all of the tabs that were closed via tabs.remove() to be restored. Instead, only 1 tab is restored.

Please let me know if you have any other questions. Once again, this requires an add-on which calls tabs.remove() in order to reproduce.

Flags: needinfo?(joe)

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(dao+bmo)
Component: Tabbed Browser → Frontend
Flags: needinfo?(dao+bmo)
Product: Firefox → WebExtensions

Hello,

I’ve attempted to reproduce the issue based on the provided STR on the latest Nightly (81.0a1/20200728213249), Beta (80.0b1/20200728204253) and Release (79.0/20200720193547), under Windows 10 Pro 64-bit and Ubuntu 16.04 LTS, however without success.

Using the add-on from https://addons.mozilla.org/en-US/firefox/addon/close-other-tabs-item/, multiple tabs (on each tab a page was loaded such as YouTube, Twitter, Wikipedia, etc.) were opened. From the first tab, the “Close Tabs to the Right” option was used and afterwards the “Undo Close Tabs” option. All tabs previously closed were properly restored.

What I’ve noticed though is that new blank tabs (not having a page loaded on them), if closed as mentioned above, will not be restored once the “Undo Close Tabs” option is used.

Flags: needinfo?(mixedpuppy)

redirecting needinfo to reduce Shane's load.

Flags: needinfo?(mixedpuppy) → needinfo?(rob)

Confirmed.

STR:

  1. Open a few tabs (e.g. open example.com, example.net, example.org at the start).
  2. Visit about:debugging and inspect any extension with a background page, and run the following code snippet from the console of the extension:
ts = (await browser.tabs.query({})).map(a=>a.id).slice(0, 3);
console.log(ts);
await browser.tabs.remove(ts);

(an alternative to this step is to install one of the add-ons from comment 3 and use the add-on to close multiple tabs at once)
3. Right-click on any tab and confirm that "Undo Close Tabs" is visible. Click it.

Expected:

  • At step 3, all three closed tabs are restored.

Actual:

  • At step 3, the label is "Undo Close Tab" and only one tab is restored.

This could be fixed by changing the implementation of tabs.remove @ https://searchfox.org/mozilla-central/rev/73e4e809df771baeb61635b25f53dfb44e930904/browser/components/extensions/parent/ext-tabs.js#805-808
to call gBrowser.removeTabs instead of individual calls to gBrowser.removeTab.

This is probably a very simple code change. Writing unit tests would probably take most of the efforts.
joe - do you want to try your hand at writing a patch? There is a guide to get started at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(rob)
Summary: "Undo Close Tabs" does not work as expected with "tabs.remove()" API → "Undo Close Tabs" does not restore multiple tabs when "tabs.remove()" closed multiple tabs

Hi Rob,

Thank you for the confirmation! I've been meaning to post more comprehensive reproduction steps, I appreciate you taking a closer look in the meantime.

I think writing a patch would be a great weekend project, I'd be happy to take a stab at that. I'll try to keep this thread updated in case I should run into any blocking issues.

Thanks again!

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(mixedpuppy)
Mentor: tomica
Severity: -- → N/A
Type: defect → enhancement
Keywords: good-first-bug
Priority: -- → P3

Clearing the old needinfo (Rob did set the severity field but forgot to clear the needinfo assigned to Shane).

Flags: needinfo?(mixedpuppy)

Hi there! I'm one of the Outreachy applicants, and I'd like to work on this bug.

Right now I'm working on setting up my dev environment and then I'll be sure to submit a patch as soon as I can, or if I'm stuck I'll post back here again with any questions. Thank you to Luca and Rob for your work as Outreachy mentors!

Work in progress patch, add automated test to replicate bug.

Assignee: nobody → liz
Status: NEW → ASSIGNED
Attachment #9181439 - Attachment description: Bug 1650956 - Add failing browser test. r=robwu,rpl → Bug 1650956 - Fix bug and add browser tests. r=robwu,rpl
Attachment #9181439 - Attachment description: Bug 1650956 - Fix bug and add browser tests. r=robwu,rpl → Bug 1650956 - browser.tabs.remove() now calls removeTabs(). r=robwu,rpl
Pushed by rmaries@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b952d7de3e3e
browser.tabs.remove() now calls removeTabs(). r=robwu

Looks like there are two test failures that happen on different machines/jobs,

  • browser/components/extensions/test/browser/browser_ext_tabs_successors.js
  • browser/components/extensions/test/browser/browser_ext_tabs_update.js

The last one (browser_ext_tabs_update.js) does not contain browser.tabs.remove, so the failure is probably indirectly caused by the previous failing test. This is unfortunate, but not uncommon: the browser tests run in the same browser, so if an earlier test failed, the state of the browser may be unpredictable, which can result in unexpected test failures in later tests.

The first test failure (browser_ext_tabs_successors.js) does contain multiple instances of browser.test.remove with an array argument, so I would recommend to start investigating that. If it doesn't fail locally, try running with --verify to trigger it more often:

./mach test browser/components/extensions/test/browser/browser_ext_tabs_successors.js
to run once.

./mach test browser/components/extensions/test/browser/browser_ext_tabs_successors.js --verify
to run with test verification (i.e. repeatedly, under chaos mode).

Once you've resolved this specific failure, I suggest to also find other instances of browser.tabs.remove with an array argument, and run them with ./mach test, and maybe even also ./mach test --verify to confirm that these tests are not failing - https://searchfox.org/mozilla-central/search?q=browser.tabs.remove%28%5B&path=&case=false&regexp=false

Mentor: tomica → rob

The issue causing the test to time out seems to be a dialogue box, "You are about to close 3 tabs. Are you sure you want to continue?" The dialogue box is triggered by the last sub-test in browser_ext_tabs_successors.js -- if I comment out that last sub-test, the test passes with --verify. And just for funsies I also ran it with all the subtests, with --verify, and just manually clicked "Close" every time the dialogue box popped up and all the tests do pass. In the dialogue, unchecking the box for "Warn me when I attempt to close multiple tabs" doesn't stick for the rest of the tests.

Each of the four sub-tests in this file ends with at least one call to browser.tabs.remove, passing in an array of tab IDs, but only the last sub-test (testMoveInSuccession_ignoreTabsInOtherWindows) triggers the dialogue box. I haven't been able to identify why. I noticed that the testTabSuccessors sub-test and the testMoveInSuccession_ignoreTabsInOtherWindows sub-test both pass in tab IDs from multiple windows, so I don't think it's related to that...

Any suggestions for where to look next?

Is the dialog from tabs.remove or a side effect of your test?

The tabs.remove extension API should not require additional user interaction before the removal of the tabs.

I did a very quick search for the error message and found that the name of the message is tabs.closeWarningMultiple.
It is used in warnAboutClosingTabs at https://searchfox.org/mozilla-central/rev/d866b96d74ec2a63f09ee418f048d23f4fd379a2/browser/base/content/tabbrowser.js#3025
Searching for uses of warnAboutClosingTabs yields multiple results, including removeAllTabsBut and warnAboutClosingWindow. The last one is used by removeTabs: https://searchfox.org/mozilla-central/rev/d866b96d74ec2a63f09ee418f048d23f4fd379a2/browser/base/content/tabbrowser.js#3225-3232

The latter shows a problematic scenario: when an extension calls tabs.remove on all tabs, a dialog will probably appear. This should NOT happen. If there is no test coverage for the case, then you should add an explicit unit test.

A way to resolve this is to support another new, appropriately option in removeTabs, to change the behavior to not prompt.

Hi Liz,

Is there anything that I can do to help you with relanding the patch?

Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e71a1560120a
browser.tabs.remove() now calls removeTabs(). r=robwu,Gijs
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch

Congrats on landing this patch, Liz! 🎉 Your contribution has been added to our recognition wiki at https://wiki.mozilla.org/Add-ons/Contribute/Recognition.

Welcome onboard! We look forward to seeing you around.

Flags: needinfo?(liz)
You need to log in before you can comment on or make changes to this bug.