Muting tab causes lazy browser premature insertion

RESOLVED FIXED in Firefox 61

Status

()

P1
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: Oriol, Assigned: ablayelyfondou)

Tracking

({regression})

61 Branch
Firefox 61
regression
Points:
---

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 months ago
1. Go to https://demo.bitmovin.com/public/firefox/av1/
2. Play the video and mute the tab
3. Switch to some other tab
4. Make sure automatic session restore is enabled
5. Restart the browser

Result:

[bug 1345098] Lazy browser prematurely inserted via 'mute' property access:
getter@chrome://browser/content/tabbrowser.js:1999:37
toggleMuteAudio@chrome://browser/content/tabbrowser.xml:1932:15
restoreTab@resource:///modules/sessionstore/SessionStore.jsm:3744:7
restoreTabs@resource:///modules/sessionstore/SessionStore.jsm:3686:9
ssi_restoreWindow@resource:///modules/sessionstore/SessionStore.jsm:3503:7
ssi_restoreWindows@resource:///modules/sessionstore/SessionStore.jsm:3627:5
initializeWindow@resource:///modules/sessionstore/SessionStore.jsm:1178:11
onBeforeBrowserWindowShown/<@resource:///modules/sessionstore/SessionStore.jsm:1329:9
promise callback*onBeforeBrowserWindowShown@resource:///modules/sessionstore/SessionStore.jsm:1314:5
ssi_observe@resource:///modules/sessionstore/SessionStore.jsm:765:9
onLoad@chrome://browser/content/browser.js:1326:5
onload@chrome://browser/content/browser.xul:1:1

Premature insertions are not awful for performance, they also cause various kinds of problems like bug 1418009 and bug 1418038.

Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=be7a5a989cb3dbabce6a1bb78aad2d05baa3589a&tochange=7a630a4add1f5c974c95ab829658d49653535c36

I guess it's bug 1387976.
(Reporter)

Comment 1

11 months ago
I meant "not *only* awful".
Abdoulaye, can you look in to this?
Flags: needinfo?(ablayelyfondou)
(Reporter)

Comment 3

11 months ago
I believe this is caused by a merge conflict between bug 1387976 and bug 1443849.

In https://hg.mozilla.org/integration/autoland/rev/de798abb5ac7 from bug 1387976 there was a this.linkedPanel check that avoided lazy browser premature insertions, but this was lost in https://hg.mozilla.org/integration/autoland/rev/7a630a4add1f from bug 1443849.
Blocks: 1443849
(Assignee)

Comment 4

11 months ago
Thank you Oriol for pointing this out. It was indeed a merge conflitct. 

Yes Jared, Please assign me this. I will quickly land a patch for this.
Flags: needinfo?(ablayelyfondou) → needinfo?(jaws)
Assignee: nobody → ablayelyfondou
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
(In reply to Oriol Brufau [:Oriol] from comment #3)
> In https://hg.mozilla.org/integration/autoland/rev/de798abb5ac7 from bug
> 1387976 there was a this.linkedPanel check that avoided lazy browser
> premature insertions, but this was lost in
> https://hg.mozilla.org/integration/autoland/rev/7a630a4add1f from bug
> 1443849.

That's not "from bug 1443849", the sheriffs did that. No idea why this is labeled "part 4" of my patch series which it clearly wasn't. Also not sure why this was a conflict in the first place. My patches didn't even touch toggleMuteAudio. :|
No longer blocks: 1443849
(Assignee)

Comment 6

11 months ago
> Also not sure why this was a conflict in the first place. My patches didn't even touch toggleMuteAudio

By looking closely to the issue, I am a little bit confused too. I can't see any conflicts created from your patches. In addition, all the code from my patch disappeared.
Flags: needinfo?(oriol-bugzilla)
Flags: needinfo?(jaws)
(Reporter)

Comment 7

11 months ago
I have no idea other than https://hg.mozilla.org/integration/autoland/rev/7a630a4add1f says this:

    Bug 1443849 - Part 4: Fix broken tabbrowser.xml after merge conflict. CLOSED TREE

The author is Sebastian Hengst so I guess you can ask him.

But I don't really see how this matters, wouldn't fixing tabbrowser.xml so that it behaves like it was supposed to in bug 1387976 do the trick?
Flags: needinfo?(oriol-bugzilla)
(Assignee)

Comment 8

11 months ago
Created attachment 8959351 [details] [diff] [review]
Reapplied changsets de798abb5ac7 and a049e86626d4 (bug 1387976) for lazy-tab muted state to persist across browser restarts
(Reporter)

Comment 9

11 months ago
Abdoulaye, it seems that most parts of your patch already landed, e.g. see your test:
https://searchfox.org/mozilla-central/source/browser/base/content/test/tabs/browser_bug_1387976_restore_lazy_tab_browser_muted_state.js

So you should use "hg pull" and update, and then attach a patch only with the parts that magically disappeared.
(Assignee)

Comment 10

11 months ago
(In reply to Oriol Brufau [:Oriol] from comment #9)
> Abdoulaye, it seems that most parts of your patch already landed, e.g. see
> your test:
> https://searchfox.org/mozilla-central/source/browser/base/content/test/tabs/
> browser_bug_1387976_restore_lazy_tab_browser_muted_state.js
> 
> So you should use "hg pull" and update, and then attach a patch only with
> the parts that magically disappeared.

Oops I forgot to update after a pull the last time (too used to git :)).

Thanks
(Assignee)

Comment 11

11 months ago
Created attachment 8959903 [details] [diff] [review]
Avoid invoking mute and unmute methods for lazy-browsers to prevent premature insertion
(Reporter)

Comment 12

11 months ago
Comment on attachment 8959903 [details] [diff] [review]
Avoid invoking mute and unmute methods for lazy-browsers to prevent premature insertion

Thanks! I guess :jaws should review this
Attachment #8959903 - Flags: review?(jaws)
(Reporter)

Comment 13

11 months ago
Comment on attachment 8959351 [details] [diff] [review]
Reapplied changsets de798abb5ac7 and a049e86626d4 (bug 1387976) for lazy-tab muted state to persist across browser restarts

and this previous patch is obsolete.
Attachment #8959351 - Attachment is obsolete: true
Flags: needinfo?(jaws)
Keywords: checkin-needed

Comment 14

11 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/006ec77ea08a
Avoid invoking mute and unmute methods for lazy-browsers to prevent premature insertion. r=jaws
Keywords: checkin-needed

Comment 15

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/006ec77ea08a
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
status-firefox59: --- → unaffected
status-firefox-esr52: --- → unaffected

Comment 16

11 months ago
I have reproduced this bug with Nightly 61.0a1 (2018-03-14) 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]
You need to log in before you can comment on or make changes to this bug.