1694699 - Investigate tabs API regression in Firefox 86

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

A Firefox user did recently noticed a regression of the Simple Tab Groups extension on Firefox 86 and tried to identify if the extension does only happen when some other extension is also installed in the same Firefox instance.

Follows link to the reddit thread and to the github issue reported to simple-tab-groups:

• https://www.reddit.com/r/firefox/comments/loptje/firefox_86_extension_conflicts/
• https://github.com/Drive4ik/simple-tab-groups/issues/779

I tried to reproduce the issue locally and I've been able to trigger it on Nightly 88 using the following STR:

• installed "Simple Tab Groups" along with 2 other extensions from the ones listed in the reddit thread (Refined Github and Open In Reader View)
• created 2 groups in the "Simple Tab Groups"
• enable "Group 1" in the first window, and open a single tab (I loaded "about:addons" in it, it may not really matter to be fair)
• enable "Group 2" in the second window, and opened 40 tabs in that window and group (I loaded 40 wikipedia tabs, I've been able to trigger it even with less, but it looked like a race and so I opted to open a bit more)
• close the second window
• Expected behavior:
  • all tabs from the closed groups should have been recreated and hidden in the first window, and all urls and titles of the tabs should match the ones originally closed
• Actual behavior:
  • all or some of the tabs from the closed groups are recreated and hidden in the first window, but without any url and so they are empty tabs
  • for a short amount of time I could see a number of new tabs with the "New Tab" title opened in the first window, shortly before being hidden

I tried to see if the STR described above was consistent enough to get a realistic regression range using mozregression and it did point me to the following pushlog:

• https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ddc758e10a99133112ce71fbeda718303664fee9&tochange=a5b3fa068d5ee122dac27298d7da7d7231d3db49

Only one patch is part of that push and it is Bug 1679688.

It looks realistic as the potential regressing change because:

• the patch did land in Firefox 86 (which does match the first Firefox version on which the user did experience the issue for the first time)
• the change is related to the tabs API, and it does introduce changes to when an extension is expected to be able to receive additional tab details, which include the tab url)

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

needinfo-ing Rob to investigate further if the regression is introduced by Bug 1679688 as it seems.

Comment 2

[Rob Wu [:robwu]]

I cannot reproduce the error with the STR.

I checked the patch from bug 1679688 over and over again, and the logic is fully equivalent, especially when the "tabs" permission is present.
I noticed one potential difference: https://searchfox.org/mozilla-central/diff/cdec8d59d82e26a20352354bc4f496d056886da6/toolkit/components/extensions/parent/ext-tabs-base.js#627-639

this.uri was replaced with this._url as a form of optimization (to avoid checking tab.hasTabPermission twice, which became more complex in the patch). But this.uri was a nsIURI object, whereas this._url is a string. MatchPatternSet::matches accepts both arguments, but the difference is that when the input is a string, that a new nsIURI instance is created. If there is memory pressure, instantiating a new nsIURI may fail, and cause the method to throw, and cause browser.tabs.query to reject with "An unexpected error occurred" to the extension and NS_ERROR_OUT_OF_MEMORY in the browser console.

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1694699 - Avoid unnecessary string->URI conversions

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1679688

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hey Rob,
As we agreed yesterday I tried to reproduce the issue on a nightly build with your patch applied, but I was still able to reproduce the issue.

Given that you told me that you were not yet able to reproduce the same issue locally, I did try to simplify the STR a bit more, make sure that it is always consistent and then tried to get a regression window again (and got to the same regressing bug apparently).

The simplified STR is the one I did record in this screencast: https://youtu.be/ElVSJFQAs8w

To reproduce the bug I did only need to:

• have both STG and "Open in Reader View" extensions installed
• create two windows, in the first window I do not use any group at all
• in the second window, I create and select a group in STG and then I open 40 wikipedia tabs (the extension installed temporarily in the screencast is just an extension that open this 40 tabs when I click on its browserAction)
• close the second window -> all tabs in the closed windows are being re-created in the first window and then weekly hidden (all tabs do still have the expected url at this point)
• open a second window again, select the group -> all tabs in the group are being reopened to the second window (technically they are being moved from the first window, where they were all hidden, into the second window, where they are being shown)
• close the second window for the second time -> all tabs in the closed windows are being re-created in the first window, this time they seems to have the "New Tab" title and they are quickly hidden (and at this point the bug has been triggered)

Would you mind to try this STR and confirm me if you are still unable to trigger it?

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: activitylogs_without_bug.json

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: activitylogs_with_bug.json

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hey Rob,
In comment 6 and comment 7 I have attached some activitylog that I collected while trying to reproduce the issue:

• comment 6 is related to the STR executed while the "open in reader view" extension was disabled
• comment 7 is the same STR with the "open reader view" enabled

I'm not sure if I've been able to capture any log that could help us to restrict the investigation to a particular area, but I thought to attach them to aid your investigation.

Comment 9

[Graham Perrin]

Wow! I never imagined a code review so quickly. Huge thanks to both of you.

Please: is it too soon to estimate whether the developer of any affected extension – I find six in conflict with Simple Tab Groups with Firefox 86.0 – can do anything to mitigate? (Whilst we wait for a fix to land in a release of Firefox.)

Comment 10

[Rob Wu [:robwu]]

According to Luca, the patch doesn't fix the bug, at least not for him. It's still not clear what's happening, consistent reproduction steps would help.

Comment 11

[Graham Perrin]

For what it's worth, Duplicate Tabs Closer (not in conflict) – with its scope set to all windows – helps to visualise the moment at which the bug begins to bite.

Whilst the titles of bugged tabs appear OK, there'll be a red alert in the toolbar icon for Duplicate Tabs Closer.

Comment 12

[Graham Perrin]

Attachment: Screen recording of two extensions in conflict, individually, with Simple Tab Groups

Original (better quality): https://photos.app.goo.gl/iQ7SNSWvtEhqW76z5

This twelve-minute screen recording is partly in response to https://github.com/FilipePS/Traduzir-paginas-web/issues/92#issuecomment-785950841 – to help visualise the conflicts for the developer of Translate Web Pages.

https://addons.mozilla.org/addon/refined-github-/

• added at 06:25 on the timeline
• disabled at 08:08

https://addons.mozilla.org/addon/traduzir-paginas-web/

• added at 08:44
• disabled at 10:27

---

(In reply to Rob Wu [:robwu] from comment #10)

… It's still not clear what's happening, consistent reproduction steps would help.

+1

In this detailed screen recording (not made primarily for STR purposes) there may be two or three points in time that appear to stray from the reproducibility that we want.

I'll aim for STR that are consistent and concise as soon as possible.

In the meantime, two thoughts:

1. ☑ Restore previous session should be enabled – probably not required for STR here, but it is required for normal/everyday use of Simple Tab Groups
2. the bug might be suppressed whilst debug logging is enabled in Simple Tab Groups – I wonder whether any other type of debugging practice might also suppress reproduction of the bug.

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hey Rob, I had a "thread in the back on my head" that was still thinking about my STR and what could be making the issue to be triggered on the second time a window with grouped tabs get closed but not the first time and I think it may be related to pending tabs (the tabs with an url associated but not actually loaded).

I double-checked my guess by making sure to select a certain number of the grouped tabs moved in the second window before closing it for the second time, and then look how many of the hidden tabs created when I closed the second window for the second time. The amount of hidden tabs not turned into "New Tab" empty tabs is exactly the number of tabs that I made sure were not pending anymore.

I think that this detail may be interesting and could be worth to keep in mind while we look for the root cause.

Now, the details that I didn't looked into yet are:

• why the pending tabs are not triggering the issue when the "Open in reader view" extension is not enabled?
• what in the "Open in reader view" extension makes a difference?
• what in the patch identified by mozregress does make a difference related to the pending tabs and the two extension enabled?

Comment 14

[Graham Perrin]

Please try this:

https://gist.github.com/grahamperrin/d0eddafbbd838d60dad3d44ae4ab3a0a/580ed7dea035caeeda838f4ee133f7727f17ecff

Mozilla bug 1694699 - Investigate tabs API regression in Firefox 86

Steps to reproduce, then end, a conflict: draft 01

Not concise, but the steps are orderly and hopefully detailed enough to:

1. be consistently reproducible with Firefox 86.0; and
2. convey a sense of how Simple Tab Groups works with, and without, a conflicting extension.

Comment 15

[Graham Perrin]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #13)

… The amount of hidden tabs not turned into "New Tab" empty tabs is exactly the number of tabs that I made sure were not pending anymore. …

Exactly ☑ and with draft 01 (above) of my steps to reproduce, you should find:

• a red alert for exactly three duplicates at step 28
• exactly three tabs lost as New Tab at step 31.

Comment 16

[Graham Perrin]

💡 https://github.com/FilipePS/Traduzir-paginas-web/issues/92#issuecomment-786097267

• an analysis from the developer of Translate Web Pages
• a basic extension that intends to generate the bug.

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Graham Perrin from comment #16)

💡 https://github.com/FilipePS/Traduzir-paginas-web/issues/92#issuecomment-786097267

• an analysis from the developer of Translate Web Pages
• a basic extension that intends to generate the bug.

Thanks (extended to FelipePS), the minimal extension is very helpful, I think that I see now which is exactly the part of that patch that is triggering it:
-the matchesHostPermission method here is checking if this._url does match the allowed origins, but browser.currentURI.spec is going to be about:blank for a pending (or discarded) tab, that seems to be the exact line that starts to trigger the issue

As an additional confirmation, temporarily removing the call to matchesHostPermission from hasTabPermission or making matchesHostPermission to return false for pending tabs does not trigger the issue anymore.

Now that we have identified the exact place in the regressing patch that does trigger it, we are even nearer to determine the proper fix.

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1694699 - Get the webextensions tab wrapper url from the session store for discarded tabs. r?robwu!

Comment 19

[Graham Perrin]

https://gist.github.com/grahamperrin/d0eddafbbd838d60dad3d44ae4ab3a0a/12da70b880995d193b2214beee8f842f18ad0456

draft 02

• corrections to steps 18 and 19

Comment 20

[Rob Wu [:robwu]]

Minimal STR for the originally reported bug (reduced from comment 19):

1. Install https://addons.mozilla.org/addon/refined-github-/ or (STG_BUG.zip (from comment 16) via about:debugging (=add-on without "tabs" permission, with tabs.onUpdated listener).
2. Install STG: https://addons.mozilla.org/addon/simple-tab-groups/
3. Visit about:preferences and enable Restore previous session
4. Open two tabs (e.g. a github page and another github page).
5. Select the two tabs (Ctrl-Click on the tabs), contextmenu, "Move tab to group", "Create new group".
6. Open a new window, click on the button of STG and choose the group that you had just created.
7. Close the window, confirm.
8. Repeat step 6-7 again.
9. Repeat step 6.

Expected:

• The tabs from step 6 should be restored at step 9.

Actual:

• The first tab is "New Tab", about:blank.

Comment 21

[Rob Wu [:robwu]]

This bug is caused by a pre-existing bug, whose likelihood of being triggered increased as a result of the change from bug 1679688. I can attach a patch that undoes this specific issue, but the underlying root causes are still present (and may happen, although with a smaller likelihood).

Bug 1679688 caused the internal listeners for tabs.onUpdated to invoke tab.hasTabPermission, which accesses browser.currentURI when an extension doesn't have the "tabs" permission. currentURI usually returns the tab's URL, but in the case of discarded tabs, a value is fetched from SessionStore and cached as _cachedCurrentURI. This getter assumes that the following three values are equivalent (and synchronized):

• browser._cachedCurrentURI
• SessionStore.getLazyTabValue(aTab, "url")
• browser.currentURI (non-lazy version)

That's not always true, at least not in two cases:

• When undiscarding a tab, the cached value is not correctly erased (so when the tab is discarded again, an incorrect value may be used). In _insertBrowser in tabbrowser.js, aTab should be browser. This isn't related to this bug, but the cause of bug 1528895 / bug 1522184 / bug 1498432.
• When a window adopts a tab (e.g. using browser.tabs.move to a different window), the onUpdated logic is triggered while "the URL" is about:blank. Stack trace where SessionStore.getLazyTabValue(aTab, "url") === "about:blank" (and _cachedCurrentURI is void) (observed when running STR from comment 20):

getter@chrome://browser/content/tabbrowser.js:2151:113
get _uri@chrome://extensions/content/parent/ext-tabs-base.js:243:5
get matchesHostPermission@chrome://extensions/content/parent/ext-tabs-base.js:203:5
get hasTabPermission@chrome://extensions/content/parent/ext-tabs-base.js:175:1 <--- NEW since bug 1679688
sanitize@chrome://browser/content/parent/ext-tabs.js:220:9
fireForTab@chrome://browser/content/parent/ext-tabs.js:269:34
listener@chrome://browser/content/parent/ext-tabs.js:348:19
hideTab@chrome://browser/content/tabbrowser.js:4288:12
restoreTab@resource:///modules/sessionstore/SessionStore.jsm:4480:18
setTabState@resource:///modules/sessionstore/SessionStore.jsm:3010:10
ss_setTabState@resource:///modules/sessionstore/SessionStore.jsm:298:26
swapBrowsersAndCloseOther@chrome://browser/content/tabbrowser.js:3985:22
adoptTab@chrome://browser/content/tabbrowser.js:4537:17
move@chrome://browser/content/parent/ext-tabs.js:1073:36

(I'm still investigating this stack, but posting the comment because I'm meeting with Luca to discuss this)

Comment 22

[Rob Wu [:robwu]]

Attachment: Bug 1694699 - Minimize access to tab.hasTabPermission

tab.hasTabPermission indirectly triggers an access to
browser.currentURI, which for lazy tab browsers causes an incorrect
value to be cached. To avoid this, skip the call to hasTabPermission.

Comment 23

[Rob Wu [:robwu]]

I added a unit test to https://phabricator.services.mozilla.com/D106622

The circumstances to trigger this bug is:

1. tabs.create + discarded:true (or maybe even tabs.discard).
2. tabs.hide
3. tabs.move AND trigger a read to the tab's URL (tabs.onUpdated from an extension without the "tabs" permission in this bug; there is an already-existing bug where this issue is triggered, when tabs.onUpdated + "urls" filter is used)

The stack trace from comment 21 shows the result of these steps: A discarded tab that has been hidden yields a about:blank URL when it is hidden in the middle of adopting a tab. The read access to browser.currentURI causes the URL to be cached in _cachedCurrentURI, and this value is subsequently used for further browser.currentURI calls, despite the URL changing afterwards.

I'll file a new bug for the reparenting issue, plus a test case. The test case is an extension, but it is possible that the real issue is in a tabbrowser.js (along comment 21's stack).

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6ee8b29a4cb
Avoid unnecessary string->URI conversions r=rpl
https://hg.mozilla.org/integration/autoland/rev/4d91456f4232
Minimize access to tab.hasTabPermission r=rpl,geckoview-reviewers,agi

Comment 25

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/f6ee8b29a4cb
https://hg.mozilla.org/mozilla-central/rev/4d91456f4232

Comment 26

[Rob Wu [:robwu]]

Comment on attachment 9205702 [details]
Bug 1694699 - Minimize access to tab.hasTabPermission

Beta/Release Uplift Approval Request

• User impact if declined: dataloss: Extensions can lose track of the URL of discarded tabs, so session/tab manager type add-ons can lose the user's session.
• 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): Small change specific to extension code without functional changes. The side effect that caused the bug is removed by reordering some logic, in a way that is equivalent to the logic before the regression (bug 1679688). There is also a regression test that verifies that the functionality works as expected now.
• String changes made/needed:

Comment 27

[Julien Cristau [:jcristau]]

Comment on attachment 9205702 [details]
Bug 1694699 - Minimize access to tab.hasTabPermission

approved for 87.0b5

Comment 28

[Graham Perrin]

Thank you.

For release users, in the meantime:

• might there be any reasonable mitigation from developers of affected extensions?

---

https://github.com/FilipePS/Traduzir-paginas-web/issues/92#issuecomment-786071769 observed:

… if I disable the call to the function chrome.tabs.onUpdated.addListener the bug does not happen.

(I'm no developer, but I imagine that this function is essential to extensions such as Simple Tab Groups.)

Comment 29

[Rob Wu [:robwu]]

(In reply to Graham Perrin from comment #28)

For release users, in the meantime:

• might there be any reasonable mitigation from developers of affected extensions?

There are two relevant extensions here:

1. Extensions that rely on the URL of discarded tabs
2. Extensions that trigger this bug in another extension (from group 1) - these extensions use tabs.onUpdated without the tabs permission.

Extensions in the second group can request the tabs permission to not trigger this bug. For example by publishing an update that adds tabs to optional_permissions (optional, to not trigger a permission prompt), and either introducing code that calls browser.permissions.request upon click (e.g. from the options page), or not adding any new UI at all and let users toggle the permission via the existing built-in UI at about:addons. The interface looks like this: https://blog.mozilla.org/addons/2020/11/20/extensions-in-firefox-84/

Extensions in the first group cannot easily work around this. The problem is that the bug causes the extension framework to report an incorrect value for all discarded tabs (as mentioned before, there were already bugs for certain discarded tabs, bug 1498432, so your add-on will still receive incorrect values for a short while). A work-around to that is to separately record the tab's URL, obtained before the tab is discarded. In memory, in extension storage, in browser.sessions.setTabValue / browser.sessions.getTabValue for example. After triggering the bug you cannot easily recover the URL. It is potentially possible to recover the URL by reviving and immediately discarding the tab (because this bug is only triggered when a tab is in a lazy/discarded state), but doing so can be a poor user experience (performance, other bugs, potentially dataloss) so I do not recommend that path.

Comment 30

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bbcbf5f8fdfd
https://hg.mozilla.org/releases/mozilla-beta/rev/c57083231ff3

Comment 31

[Graham Perrin]

🎉 to the uplift; and

(In reply to Rob Wu [:robwu] from comment #29)

Very eloquent, I just need to check my sanity on this:

1. Extensions that rely on the URL of discarded tabs

– Simple Tab Groups is within group 1, yes?

The report in the Refined GitHub area is locked to prevent discussion, but I can draw other developers' attention to your comment.

Many thanks

Comment 32

[Rob Wu [:robwu]]

(In reply to Rob Wu [:robwu] from comment #29)
Very eloquent, I just need to check my sanity on this:

1. Extensions that rely on the URL of discarded tabs

– Simple Tab Groups is within group 1, yes?

Yes.

Comment 33

[Graham Perrin]

Re: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=253886#c2 I built and installed Firefox 86.0_3,2 on FreeBSD 14.0-CURRENT, then re-enabled the six (group 2) extensions that previously triggered the bug in the presence of (group 1) Simple Tab Groups.

The bug is no longer reproducible.

Thanks again 🏆

Comment 34

[Pascal Chevrel:pascalc]

Comment on attachment 9205702 [details]
Bug 1694699 - Minimize access to tab.hasTabPermission

Approved for 86.0.1, thanks.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/b080dea7fdb7
https://hg.mozilla.org/releases/mozilla-release/rev/1d014d732d27

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Added to the 86.0.1 relnotes:

Fixed an issue causing unexpected behavior with extensions managing tab groups