1387976 - Normal tabs are not remembering their previously muted state and outputs sound after restart

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P2)

Description

[Emil Ghitta, QA [:emilghitta]]

Attachment: Muted normal tabs.gif

[Affected versions]
Firefox 57.0a1 (Build Id:20170806100257)
Firefox 55.0 (Build Id:20170803103124)

[Affected platforms]:
Windows 10 64bit.
macOS 10.11.6.
Ubuntu 14.04 64 bit.

[Steps to reproduce]:
1.Launch Firefox with a clean profile.
2.Access the https://www.youtube.com/ webpage.
3.Open a video in a new tab.
4.Mute the opened tab.
5.Restart Firefox.
6.Reload all tabs.

[Expected result]:
The previously opened tab remembers it's previous state and is muted.

[Actual result]:
The previously opened tab outputs sound.

[Regression range]:
I don't think that this is a regression since it is reproducible with Firefox 48.0a1 also.

[Additional notes]
Please observe the attached screencast for further information.
This issue is also reproducible after restoring the previous session after a crash.
This issue is not reproducible with pinned tabs.

Comment 1

[Mike de Boer [:mikedeboer]]

So I'm thinking that someone who worked on the mute tab indicators before would be the ideal candidate to fix this most efficiently.
Dão, since Ehsan is away for a while, I can't really ask him... do you know who may be able to triage this tab mute issue?

Comment 2

[Dão Gottwald [:dao]]

:alwu worked somewhat recently on bug 1347791 which seems related.

Comment 3

[Mike de Boer [:mikedeboer]]

Indeed, thanks! Alastor, do you or someone on your team have time to work on this bug? I'd be more than happy to mentor the Sessionstore component related bits!

Comment 4

[Alastor Wu [:alwu]]

Sure, I can take this bug, but I have other works need to be done first.
I'll go back on it later.

Comment 5

[Abdoulaye O. LY]

Attachment: Some changes on tabbrowser.xml to allow Lazy Tabs Browser to toggle mute audio in order to restore their previous mute state

Comment 6

[Abdoulaye O. LY]

FYI: There is no test case for this patch, thanks on giving hints in case we need one.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Looks good! I tested out your patch on my machine and it fixes the bug for me :)

browser/base/content/test/general/browser_audioTabIcon.js has some tests for this feature. I ran the test locally and it continues to pass, which is good news. To run the test, you would run the following command:
> `./mach test browser/base/content/test/general/browser_audioTabIcon.js`

We should add some new tests for the work that you are doing, but not in that file since that test takes far too long to run already (over 30 seconds). We can't add any new tests to /browser/base/content/test/general since that directory is too large. It also helps to put the tests in a specific directory so it is easy to run a small subset of tests to check that a minor change hasn't broken a feature. You can add the new test in `/browser/base/content/test/tabs`. Feel free to follow the style of browser_audioTabIcon.js, specifically it looks like get_tab_state() would be useful.

It looks like you may be able to simulate a restart of a specific tab by doing this: https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/browser/base/content/test/general/browser_restore_isAppTab.js#35-50

Comment 8

[Abdoulaye O. LY]

Attachment: Adding test for the bug patch

Comment 9

[Abdoulaye O. LY]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)

> It looks like you may be able to simulate a restart of a specific tab by
> doing this:
> https://searchfox.org/mozilla-central/rev/
> d03ad8843e3bf2e856126bc53b0475c595e5183b/browser/base/content/test/general/
> browser_restore_isAppTab.js#35-50

This does not work for this specific situation, because the tab isn't actually removed and keeps its state so. The only thing that I feel is really restarted here is the tab browser. So I needed to simulate the restart of the tab by "replacing" it with a new lazy tab restored with the removed tab data of course.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8952577 [details] [diff] [review]
Adding test for the bug patch

Review of attachment 8952577 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! Can you combine your patches and reupload them with the following commit message:
`Bug 1387976 - Persist tab-muted state across restarts of the browser. r?jaws`

Then we can get this landed. Nice job!

::: browser/base/content/test/tabs/browser.ini
@@ +34,4 @@
>  [browser_visibleTabs_bookmarkAllTabs.js]
>  [browser_visibleTabs_contextMenu.js]
>  [browser_open_newtab_start_observer_notification.js]
> +[browser_bug_1387976_restore_lazy_tab_browser_muted_state.js]

Can you add a blank line at the end of the file like it was before?

Comment 11

[Abdoulaye O. LY]

Attachment: Persist tab-muted state across restarts of the browser

Comment 12

[Abdoulaye O. LY]

final upload:
 combined patches
 renamed commit message
 Added a blank line at the end of browser/base/content/test/tabs/browser.ini

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Bug 1392352 moved this code out of tabbrowser.xml and in to browser/base/content/tabbrowser.js. Can you pull the latest code from mozilla-central and update your patch?

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8954625 [details] [diff] [review]
Persist tab-muted state across restarts of the browser

Review of attachment 8954625 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.xml
@@ +2368,4 @@
>                let setter;
>                switch (name) {
>                  case "audioMuted":
> +                  getter = () => aTab.hasAttribute("muted");

Should this be using `SessionStore.getLazyTabValue(aTab, "muted");` ?

Comment 15

[Abdoulaye O. LY]

> ::: browser/base/content/tabbrowser.xml
> @@ +2368,4 @@
>                let setter;
>                switch (name) {
>                  case "audioMuted":
> +                  getter = () => aTab.hasAttribute("muted");

> Should this be using `SessionStore.getLazyTabValue(aTab, "muted");` ?

This can actually work but requires several changes such as:

 1. Adding a "muted" attribute for "tab.__SS_lazyData" object in SessionStore.jsm
 2. defining the method "setLazyTabValue" because the "muted" attribute may change for a lazy tab browser.
 3. call "setLazyTabValue" method when the muted icon is toggled (toggleMuteAudio in tabbrowser.xml)

Personally I find this approach less clean & more complicated. So what do you think ?

Comment 16

[Abdoulaye O. LY]

Attachment: Persist tab-muted state across restarts of the browser

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

If I understand https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/browser/base/content/tabbrowser.js#98-106 correctly, changing the muted state while the browser is lazy will cause the browser to wake up, so I think we need to do what you've described in comment #15.

Comment 18

[Abdoulaye O. LY]

> changing the muted state while the browser is lazy will cause the browser to wake up

No, it won't. The browser will still remain lazy. Actually, only the invocation of methods "browser.mute()" or "browser.unmute()" in toggleMuteAudio methods wake up the browser and fortunately we can avoid this.

Briefly, in both approaches we will have the browser remaining lazy when its muted state changes.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8955961 [details] [diff] [review]
Persist tab-muted state across restarts of the browser

Review of attachment 8955961 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, I tested this out and it works great. I'll get this landed now :)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7958b0ccbe0b
Persist tab-muted state across restarts of the browser. r=jaws

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b708ab55bc72
Fix merge and eslint issues with the landing of the patch on a CLOSED TREE. r=me

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Hey Abdoulaye, the patch that you had attached to the bug had some + characters at the beginning of two lines in tabbrowser.xml. I landed a follow-up patch to remove those (and also fix a spacing issue in the comments).

Did you hand-edit the patch file or have some issues with merging/rebasing while you were working on it? There are no problems, I just want to help you so in the future we won't run in to this again :)

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d91b6a40b32a
Fix a copy/paste error in a comment (should have been mute, not unmute). r=me

Comment 24

[Abdoulaye O. LY]

Oh sorry!!!

> Did you hand-edit the patch file

No, I did not. I always use hg import.

> or have some issues with merging/rebasing while you were working on it?

No, but what I actually did was to add the changes into tabbrowser.js by copying code from tortoise-hg (which is why we have these + caracteres).

I will be more careful next time.

Can I reupload the patch here.

Thank you for your help.

Comment 25

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/7c01c3875cae
Backed out 3 changesets because of merge conflicts on AsyncTabSwitcher.jsm.

Comment 26

[Cosmin Sabou [:CosminS]]

Sorry, had to back this out because of merge conflicts: https://hg.mozilla.org/mozilla-central/rev/7c01c3875cae3d9b8a3fab22c4b07efe20f6d48f

merging browser/base/content/tabbrowser.js
merging browser/base/content/tabbrowser.xml
merging browser/modules/AsyncTabSwitcher.jsm and browser/base/content/tabbrowser.js to browser/modules/AsyncTabSwitcher.jsm
warning: conflicts while merging browser/modules/AsyncTabSwitcher.jsm! (edit, then use 'hg resolve --mark')
170 files updated, 2 files merged, 0 files removed, 1 files unresolved

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Yes, you can reupload the patch. You will need to rebase the patch too.

Comment 28

[Abdoulaye O. LY]

Attachment: Persist tab-muted state across restarts of the browser

Comment 29

[Abdoulaye O. LY]

Patch reuploaded:
 - Conflicts resolved
 - removed "+" caracteres
 - fixed spacing issue on comments

Comment 30

[Abdoulaye O. LY]

Hey Jared, have you seen the last upload ? 
FYI: It's still rebasing without conflict.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de798abb5ac7
Persist tab-muted state across restarts of the browser. r=jaws

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a049e86626d4
Add missing test file for persisting tab-muted state across restarts of the browser. r=jaws

Comment 33

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/de798abb5ac7
https://hg.mozilla.org/mozilla-central/rev/a049e86626d4

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

Thanks for fixing this bug Abdoulaye! I will try to find you another bug you can work on.

Comment 35

[Abdoulaye O. LY]

The pleasure is mine and your help was great! I am looking foward to get a new bug to work on.

Comment 36

[Sajedul Islam]

I have reproduced this bug with Nightly 57.0a1 (2017-08-07) on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Nightly !

Build   ID    20180327105613
User Agent    Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

[bugday-20180321]

Comment 37

[Catalin Sasca, Desktop QA [:csasca]]

I successfully reproduced the bug on Firefox Nightly 57.0a1 (2017-08-06) under Windows 10 (X64).

The bug is no longer reproducible on latest Nightly 61.0a1 (2018-03-27) under Windows 10 (x64), Ubuntu 16.04 (x64) and macOS 10.13.

Thank you Sajedul Islam for helping out with testing.