Closed Bug 1577480 Opened 3 months ago Closed 3 months ago

browser.tabs.hide API doesn't work on tabs that are using geolocation or were previously sharing any media using webRTC

Categories

(Firefox :: Tabbed Browser, defect, P2)

70 Branch
Desktop
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox69 --- unaffected
firefox70 --- verified
firefox71 --- verified

People

(Reporter: drive4ik, Assigned: pbz)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Attached file hide-tab-bug.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

Firefox API browser.tabs.hide not worked on some tabs.

Steps for reproduce:

  1. Load temporary extension (extract from zip first)
  2. Open youtube, select any video and click play
  3. Open new tab
  4. On NOT active youtube tab click context menu and select 'hide this tab'

Firefox 70b1 and above (Nightly)
Tested on Windows and linux

I am the owner of the addon Simple Tab Groups
https://addons.mozilla.org/firefox/addon/simple-tab-groups/
and after the release of Firefox 70, it will stop working normally. And the release is coming soon - September 3

Please, fix this bug.

And thanks for your's work! :)

Actual results:

The tab was not hidden, there was no error and result is empty.

All conditions from the doc are met, but the tab was not hidden
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/hide

Expected results:

Tab need to be hidden

Component: Untriaged → General
OS: Unspecified → Windows 10
Priority: -- → P2
Product: Firefox → WebExtensions
Hardware: Unspecified → Desktop
Summary: browser.tabs.hide not worked on some tabs → browser.tabs.hide API not worked on some tabs
Version: 70 Branch → Firefox 70

As a user of Simple Tab Groups, I'm significantly bothered by this.

Speaking of OS, I'm seeing this on Ubuntu 19.04 personally.

Same bug on FF70.0b3. Very annoying!
The excellent component 'Simple Tab Groups' is broken and probably other 'tabgroups' 'panorama' clones.

I can confirm this bug with the Firefox Developer Edition 70.0b4 (64-bit) on Windows 1903

I can confirm that I was able to reproduce this issue on FF70, on Win10x64 and macOS 10.13.6.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: General → Frontend

the same on macOS Mojave 10.14.6 and FF 70.0b5

I asked Victor to run a bisect to identify the regression range, and mozregression has identified the following range:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b739e8708f02a437bdd5ab5430291da904ac393b&tochange=2ec1c2b5fda9c39ad005da4cf87c2b0ee7623df3

I took a look to those 3 patches and it seems that the tab hide API regression has been introduced in

Looking at the changes applied in the patch, I guess that the tab is not being hidden because of the !aTab._sharingState check in the gBrowser.hideTab method, because with the changes applied in Bug 630614 aTab._sharingState is going to be an object even if the tab is not currently sharing any media using webRTC.

Hi Paul,
what would be a better way of checking the tab sharing state after the change you applied in Bug 630614?
at a first glance I think it should be something like aTab._sharingState && aTab._sharingState.webRTC && aTab._sharingState.webRTC.sharing,
but there may be a better way that I may be missing.

Flags: needinfo?(pbz)

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

Hi Paul,
what would be a better way of checking the tab sharing state after the change you applied in Bug 630614?
at a first glance I think it should be something like aTab._sharingState && aTab._sharingState.webRTC && aTab._sharingState.webRTC.sharing,
but there may be a better way that I may be missing.

Hi Luca, Thanks for notifying me! I've submitted a patch which updates the hideTab check, similar to what you proposed, and also cleans up the webRTC sharing object when sharing ends.

Flags: needinfo?(pbz)

I am glade there will be a patch for this.
FireFox Developer Edition V 70.0b3 (64-bit). Both Win 10 and macOs Monjave

Assignee: nobody → pbz
Status: NEW → ASSIGNED
Regressed by: 630614

[Tracking Requested - why for this release]:
Regression in webextension api functionality in 70. We should uplift the fix if we can.

Some notes from the review:

It's probably not necessary to fix this immediately in this bug, since we want to uplift quickly and arguably it's not Paul's job to look into this. Luca, can you take a look? Thanks!

Flags: needinfo?(lgreco)
Flags: in-testsuite?

(In reply to Johann Hofmann [:johannh] from comment #12)

Some notes from the review:

It's probably not necessary to fix this immediately in this bug, since we want to uplift quickly and arguably it's not Paul's job to look into this. Luca, can you take a look? Thanks!

As I mentioned in the phabricator revision, it is ok with me if we agree on deferring the fix on the test case to a followup, but it would be great to land the regression fix with a test that covers it, and so in my last comment I provided a diff for that (https://phabricator.services.mozilla.com/D45820#1393821).
(But don't worry if the fix lands without the changes to the test, I'll deal with it in a followup if it is not there yet).

About "logging a warning message when tabs.hide doesn't hide a tab because it is shared", it sounds reasonable but something that we can definitely defer to a follow up.

:johannah would you mind to file a bugzilla issue for it?

Flags: needinfo?(lgreco)
Pushed by pzuhlcke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4f48950d509
Fixed hideTab webRTC sharing state check. r=johannh
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Please nominate this for Beta approval when you get a chance.

Flags: needinfo?(pbz)
Keywords: regression

Hello,

Verified the fix using the latest Nightly (71.0a1/20190918215055) under Windows 10 Pro 64-bit and macOS High Sierra 10.13.6.

Using the provided extension, tabs can now be properly hidden, confirming the fix.

Status: RESOLVED → VERIFIED
Component: Frontend → Tabbed Browser
Product: WebExtensions → Firefox
Target Milestone: mozilla71 → ---
Version: Firefox 70 → 70 Branch
Target Milestone: --- → Firefox 71

Comment on attachment 9092612 [details]
Bug 1577480 - Fixed hideTab webRTC sharing state check. r=johannh

Beta/Release Uplift Approval Request

  • User impact if declined: Partial breakage of add-on hide-tab functionality.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See first bug comment
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Small front-end patch. Fix has test coverage and has been verified in nightly.
  • String changes made/needed:
Flags: needinfo?(pbz)
Attachment #9092612 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Summary: browser.tabs.hide API not worked on some tabs → browser.tabs.hide API doesn't work on tabs using geolocation

I don't think the updated title fits. This is regressed by the geolocation patch, but it does not affect tabs using geolocation specifically. Also affects webrtc.

(In reply to Paul Zühlcke [:pbz] from comment #19)

I don't think the updated title fits. This is regressed by the geolocation patch, but it does not affect tabs using geolocation specifically. Also affects webrtc.

I was under the impression that we intentionally do not allow hiding tabs using webrtc, and your patch shouldn't change this. Am I missing something?

(In reply to Dão Gottwald [::dao] from comment #20)

(In reply to Paul Zühlcke [:pbz] from comment #19)

I don't think the updated title fits. This is regressed by the geolocation patch, but it does not affect tabs using geolocation specifically. Also affects webrtc.

I was under the impression that we intentionally do not allow hiding tabs using webrtc, and your patch shouldn't change this. Am I missing something?

That's right. But the issue also made it impossible to hide tabs which previously had an active webrtc sharing state.

Summary: browser.tabs.hide API doesn't work on tabs using geolocation → browser.tabs.hide API doesn't work on tabs that are using geolocation or were previously sharing any media using webRTC
QA Whiteboard: [qa-triaged]

Comment on attachment 9092612 [details]
Bug 1577480 - Fixed hideTab webRTC sharing state check. r=johannh

P1 regression, fix verified in Nightly and patch has tests, approved for 70beta9 , thanks!

Attachment #9092612 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Hello,

Verified the fix using the latest Beta (70.0b9/20190923154733) under Windows 10 Pro 64-bit and macOS High Sierra 10.13.6.

Using the provided extension, tabs can now be properly hidden, confirming the fix.

You need to log in before you can comment on or make changes to this bug.