Don't zombify tabs that are playing audio

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

unspecified
Firefox 48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
It's super annoying to have the sound suddenly stopped because the tab in which you were listening to music with gets zombified.
(Assignee)

Comment 1

3 years ago
Created attachment 8726253 [details] [diff] [review]
Don't zombify tabs that are playing audio

Margaret, I haven't tested this yet, I'm not sure what triggers zombification
and all, but does that look reasonnable ?

Also, should I and how would I write a test for this ? I'm unfamiliar with how
front-end testing works in Fennec.
Attachment #8726253 - Flags: feedback?(margaret.leibovic)
(Assignee)

Updated

3 years ago
Assignee: nobody → padenot
Status: NEW → ASSIGNED

Comment 2

3 years ago
Comment on attachment 8726253 [details] [diff] [review]
Don't zombify tabs that are playing audio

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

Thanks for the patch! We don't have great test coverage on Fennec, but if you want to take a stab at it, I don't think it would be that hard to make a new chrome mochitest for this:
https://wiki.mozilla.org/Mobile/Fennec/Android/Testing#mochitest_.28plain_and_chrome.29

Our current tests live in here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/chrome/

We do already have some tests that include media:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/chrome/test_video_discovery.html?force=1

You could check to see if these have audio, and if they do make a new test that loads one in a new tab and plays it. You could test calling expireLruTab directly, although it looks like you would need to do some set-up to make sure that it doesn't return early.

Given the current testing situation, a test isn't strictly required for this fix, but it would be nice :)

::: mobile/android/chrome/content/browser.js
@@ +3511,5 @@
>        };
>        Messaging.sendRequest(message);
>  
>        this.overscrollController = new OverscrollController(this);
> +      this.playingAudio = false;

I think it would be more appropriate to declare this in the Tab constructor:
http://hg.mozilla.org/mozilla-central/annotate/bdde3fedb45b/mobile/android/chrome/content/browser.js#l3307

@@ +4178,4 @@
>          Messaging.sendRequest({
>            type: "Tab:AudioPlayingChange",
>            tabID: this.id,
> +          isAudioPlaying: this.playingAudio

This message is just used to update the state in Java, you don't need to include this unless you're using this data for the native Android UI.

@@ +7311,5 @@
>      // Find the least recently used non-zombie tab.
>      for (let i = 0; i < tabs.length; i++) {
> +      if (tabs[i] == selected ||
> +          tabs[i].browser.__SS_restore ||
> +          tabs[i].playingAudio) {

This seems reasonable to me, but kats should also take a look.
Attachment #8726253 - Flags: feedback?(margaret.leibovic)
Attachment #8726253 - Flags: feedback?(bugmail.mozilla)
Attachment #8726253 - Flags: feedback+
Comment on attachment 8726253 [details] [diff] [review]
Don't zombify tabs that are playing audio

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

This looks good to me!
Attachment #8726253 - Flags: feedback?(bugmail.mozilla) → feedback+
(Assignee)

Comment 4

3 years ago
(In reply to :Margaret Leibovic from comment #2)
> @@ +4178,4 @@
> >          Messaging.sendRequest({
> >            type: "Tab:AudioPlayingChange",
> >            tabID: this.id,
> > +          isAudioPlaying: this.playingAudio
> 
> This message is just used to update the state in Java, you don't need to
> include this unless you're using this data for the native Android UI.

This is already present, I'm just moving the actual computation of the state to factor it with setting the new attribute I'm using.
(Assignee)

Comment 5

3 years ago
Created attachment 8726808 [details] [diff] [review]
Don't zombify tabs that are playing audio

Comments addressed, I've looked into adding a test, but I'm really not familiar
with all that so I'll pass.
Attachment #8726808 - Flags: review?(margaret.leibovic)
(Assignee)

Updated

3 years ago
Attachment #8726253 - Attachment is obsolete: true

Comment 6

3 years ago
(In reply to Paul Adenot (:padenot) from comment #4)
> (In reply to :Margaret Leibovic from comment #2)
> > @@ +4178,4 @@
> > >          Messaging.sendRequest({
> > >            type: "Tab:AudioPlayingChange",
> > >            tabID: this.id,
> > > +          isAudioPlaying: this.playingAudio
> > 
> > This message is just used to update the state in Java, you don't need to
> > include this unless you're using this data for the native Android UI.
> 
> This is already present, I'm just moving the actual computation of the state
> to factor it with setting the new attribute I'm using.

Oops, must have looked at that too quickly!

Updated

3 years ago
Attachment #8726808 - Flags: review?(margaret.leibovic) → review+

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f637f149610b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Updated

3 years ago
Blocks: 1260113
You need to log in before you can comment on or make changes to this bug.