tab audio-indicator disappears when dragging tab to a new window

VERIFIED FIXED in Firefox 42

Status

()

Core
Audio/Video
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Dolske, Assigned: Ehsan)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
The audio indicator disappears when dragging a tab to a new window.

1) Open https://people.mozilla.org/~dolske/tmp/rickroll.ogg in a tab
2) Observe audio indicator in tab
3) Dance to the groovy beat
4) Drag that tab to a different Firefox window

Expected: tab in the new window should have an audio indicator for the still-playing media

Actual: media still playing but no indicator

[Note that the indicator will reappear if you pause+resume the video.]
(Assignee)

Updated

3 years ago
Assignee: nobody → ehsan
Blocks: 1188860
No longer blocks: 486262
(Assignee)

Comment 1

3 years ago
This is an unfortunate byproduct of the interaction of several things together.  In bug 918634, we changed things so that the pagehide event is dispatched in the content script.  Now, as of bug 449778, we dispatch the pagehide event here, in the middle of a bunch of pageshow events.  However, as of bug 1180535, we use that event to catch the case where we are navigating away from an iframe, for example, and therefore we end up dispatching one "audio-playback" event with "inactive" data, and the icon that bug 1190106 was trying to preserve will get removed!

I guess we need to massage that code to exclude the case where the docshell is being frame-swapped.
Blocks: 1180535
Component: Tabbed Browser → Audio/Video
Product: Firefox → Core
See Also: → bug 449778, bug 918634
(Assignee)

Comment 2

3 years ago
Created attachment 8644339 [details] [diff] [review]
Do not dispatch an audio-playback notification when swapping browsers

We send a pagehide event during swapping docshell frame loaders, and
before this patch we would not be able to differentiate this event with
the one that we send when navigating away from a page, so we would
incorrectly dispatch an audio-playback notification indicating that
audio playback has stopped.  This patch adds a flag to the root docshell
when the frame loader swapping is in progress and disables the above
behavior when that flag is set.
Attachment #8644339 - Flags: review?(bugs)
(Assignee)

Updated

3 years ago
See Also: → bug 1190106

Comment 3

3 years ago
Comment on attachment 8644339 [details] [diff] [review]
Do not dispatch an audio-playback notification when swapping browsers

>+nsDocShell::SetInFrameSwap(bool aInSwap)
>+{
>+  nsRefPtr<nsDocShell> shell = this;
>+  while (true) {
>+    nsRefPtr<nsDocShell> parent = shell->GetParentDocshell();
>+    if (!parent) {
>+      shell->mInFrameSwap = aInSwap;
>+      return;
>+    }
>+    shell = parent.forget();
>+  }
>+}
Why you set the flag on the root docshell? That is somewhat surprising and I don't understand the reason for that.

>   unused << mRemoteBrowser->SendSwappedWithOtherRemoteLoader();
>   unused << aOther->mRemoteBrowser->SendSwappedWithOtherRemoteLoader();
>+
>+  mInSwap = aOther->mInSwap = false;
>+  unused << mRemoteBrowser->SendSetInFrameSwap(false);
>+  unused << aOther->mRemoteBrowser->SendSetInFrameSwap(false);
I don't see how this could work.
SendSwappedWithOtherRemoteLoader is async, so after that child process may load something and you may get a valid pagehide, but
you send SetInFrameSwap after all that.

I think we may want to add a new param to nsDocument::OnPageHide/Show to indicate that it is a fake "pagehide/show", and then use that information here.



>+    // Be careful to ignore this event during a docshell frame swap.
>+    nsIDocument* ownerDoc = OwnerDoc();
>+    if (!ownerDoc) {
>+      return;
>+    }
OwnerDoc() is _never_ null.
Attachment #8644339 - Flags: review?(bugs) → review-
(Assignee)

Comment 4

3 years ago
(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8644339 [details] [diff] [review]
> Do not dispatch an audio-playback notification when swapping browsers
> 
> >+nsDocShell::SetInFrameSwap(bool aInSwap)
> >+{
> >+  nsRefPtr<nsDocShell> shell = this;
> >+  while (true) {
> >+    nsRefPtr<nsDocShell> parent = shell->GetParentDocshell();
> >+    if (!parent) {
> >+      shell->mInFrameSwap = aInSwap;
> >+      return;
> >+    }
> >+    shell = parent.forget();
> >+  }
> >+}
> Why you set the flag on the root docshell? That is somewhat surprising and I
> don't understand the reason for that.

Because this would nicely deal with iframes and top-level documents playing audio with a single code path.  Do you think we should propagate the flag to all docshells?

> >   unused << mRemoteBrowser->SendSwappedWithOtherRemoteLoader();
> >   unused << aOther->mRemoteBrowser->SendSwappedWithOtherRemoteLoader();
> >+
> >+  mInSwap = aOther->mInSwap = false;
> >+  unused << mRemoteBrowser->SendSetInFrameSwap(false);
> >+  unused << aOther->mRemoteBrowser->SendSetInFrameSwap(false);
> I don't see how this could work.
> SendSwappedWithOtherRemoteLoader is async, so after that child process may
> load something and you may get a valid pagehide, but
> you send SetInFrameSwap after all that.

Yeah, I guess that edge case won't work correctly, but in that case the pageshow/hide events that we send through SendSwappedWithOtherRemoteLoader would also be bogus, right?

> I think we may want to add a new param to nsDocument::OnPageHide/Show to
> indicate that it is a fake "pagehide/show", and then use that information
> here.

Do you mean just to not have to send two IPC messages that can run with arbitrary stuff in between, or do you mean instead of the docshell flag?  I sort of see the case for the former, but the latter is way too much work, I think.

> >+    // Be careful to ignore this event during a docshell frame swap.
> >+    nsIDocument* ownerDoc = OwnerDoc();
> >+    if (!ownerDoc) {
> >+      return;
> >+    }
> OwnerDoc() is _never_ null.

Should we remove the null check from other places in that file then?
Flags: needinfo?(bugs)

Comment 5

3 years ago
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #4)
> > Why you set the flag on the root docshell? That is somewhat surprising and I
> > don't understand the reason for that.
> 
> Because this would nicely deal with iframes and top-level documents playing
> audio with a single code path.  Do you think we should propagate the flag to
> all docshells?
Why you need this on root? I mean, InFrameSwap() can then check ancestors - I didn't comment on that one.



> 
> > >   unused << mRemoteBrowser->SendSwappedWithOtherRemoteLoader();
> > >   unused << aOther->mRemoteBrowser->SendSwappedWithOtherRemoteLoader();
> > >+
> > >+  mInSwap = aOther->mInSwap = false;
> > >+  unused << mRemoteBrowser->SendSetInFrameSwap(false);
> > >+  unused << aOther->mRemoteBrowser->SendSetInFrameSwap(false);
> > I don't see how this could work.
> > SendSwappedWithOtherRemoteLoader is async, so after that child process may
> > load something and you may get a valid pagehide, but
> > you send SetInFrameSwap after all that.
> 
> Yeah, I guess that edge case won't work correctly, but in that case the
> pageshow/hide events that we send through SendSwappedWithOtherRemoteLoader
> would also be bogus, right?
I wouldn't say they are bogus. JS can still use pagehide/show to update the state.



> Do you mean just to not have to send two IPC messages that can run with
> arbitrary stuff in between, or do you mean instead of the docshell flag?
both. Swapping should pass the param to 

>  I
> sort of see the case for the former, but the latter is way too much work, I
> think.
How so? But sure, I can live with setting and clearing the docshell flag in RecvSwappedWithOtherRemoteLoader
around the pagehide/show calls.


> 
> Should we remove the null check from other places in that file then?
If there are null checks, yes.
Flags: needinfo?(bugs)
(Assignee)

Comment 6

3 years ago
(In reply to Olli Pettay [:smaug] from comment #5)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #4)
> > > Why you set the flag on the root docshell? That is somewhat surprising and I
> > > don't understand the reason for that.
> > 
> > Because this would nicely deal with iframes and top-level documents playing
> > audio with a single code path.  Do you think we should propagate the flag to
> > all docshells?
> Why you need this on root? I mean, InFrameSwap() can then check ancestors -
> I didn't comment on that one.

OK, that I can do!

> > Do you mean just to not have to send two IPC messages that can run with
> > arbitrary stuff in between, or do you mean instead of the docshell flag?
> both. Swapping should pass the param to 
> 
> >  I
> > sort of see the case for the former, but the latter is way too much work, I
> > think.
> How so? But sure, I can live with setting and clearing the docshell flag in
> RecvSwappedWithOtherRemoteLoader
> around the pagehide/show calls.

The HTMLMediaElement code uses a normal activity observer, so passing this flag all the way down the pipe will need a fair amount of changes that are not even only restricted to HTMLMediaElement.  This is how the call is propagated:

    frame #1: 0x0000000104eb2760 XUL`mozilla::dom::HTMLMediaElement::NotifyOwnerDocumentActivityChanged(this=0x000000012b454000) + 208 at HTMLMediaElement.cpp:4022
    frame #2: 0x0000000103a0bf00 XUL`NotifyActivityChanged(aSupports=0x000000012b454000, aUnused=0x0000000000000000) + 224 at nsDocument.cpp:4561
    frame #3: 0x0000000103a0b9ee XUL`nsIDocument::EnumerateActivityObservers(this=0x000000012d83a000, aEnumerator=0x0000000103a0be20, aData=0x0000000000000000)(nsISupports*, void*), void*) + 174 at nsDocument.cpp:10150
    frame #4: 0x0000000103a2089c XUL`nsDocument::UpdateVisibilityState(this=0x000000012d83a000) + 252 at nsDocument.cpp:12441
    frame #5: 0x0000000103a20c26 XUL`nsDocument::OnPageHide(this=0x000000012d83a000, aPersisted=true, aDispatchStartTarget=0x000000012c898880) + 886 at nsDocument.cpp:9295
    frame #6: 0x000000010379b075 XUL`nsContentUtils::FirePageHideEvent(aItem=0x000000012d86f1a8, aChromeEventHandler=0x000000012c898880) + 181 at nsContentUtils.cpp:7842
    frame #7: 0x0000000103a542eb XUL`nsFrameLoader::SwapWithOtherLoader(this=0x00000001411cfaa0, aOther=0x000000012d90ef20, aFirstToSwap=0x000000014e936390, aSecondToSwap=0x000000012d8cd0d0) + 3947 at nsFrameLoader.cpp:1144
    frame #8: 0x0000000105a99f31 XUL`nsXULElement::SwapFrameLoaders(this=0x00000001411cfa10, aOtherElement=0x000000012d90ee90, rv=0x00007fff5fbf7d28) + 273 at nsXULElement.cpp:1650

I originally wanted to do this and half way through it I saw that it's turning into a mess and decided to add a docshell flag instead.

> > Should we remove the null check from other places in that file then?
> If there are null checks, yes.

OK, I'll do that in a follow-up.
(Assignee)

Comment 7

3 years ago
Created attachment 8644691 [details] [diff] [review]
Do not dispatch an audio-playback notification when swapping browsers

We send a pagehide event during swapping docshell frame loaders, and
before this patch we would not be able to differentiate this event with
the one that we send when navigating away from a page, so we would
incorrectly dispatch an audio-playback notification indicating that
audio playback has stopped.  This patch adds a flag to the root docshell
when the frame loader swapping is in progress and disables the above
behavior when that flag is set.
Attachment #8644339 - Attachment is obsolete: true
Attachment #8644691 - Flags: review?(bugs)
(Assignee)

Comment 8

3 years ago
Created attachment 8644710 [details] [diff] [review]
Do not dispatch an audio-playback notification when swapping browsers

We send a pagehide event during swapping docshell frame loaders, and
before this patch we would not be able to differentiate this event with
the one that we send when navigating away from a page, so we would
incorrectly dispatch an audio-playback notification indicating that
audio playback has stopped.  This patch adds a flag to the root docshell
when the frame loader swapping is in progress and disables the above
behavior when that flag is set.
Attachment #8644691 - Attachment is obsolete: true
Attachment #8644691 - Flags: review?(bugs)
Attachment #8644710 - Flags: review?(bugs)

Comment 9

3 years ago
Comment on attachment 8644710 [details] [diff] [review]
Do not dispatch an audio-playback notification when swapping browsers

>+  void SetInFrameSwap(bool aInSwap) {
{ goes to its own line

>   mInSwap = aOther->mInSwap = true;
>+  ourDocshell->SetInFrameSwap(true);
>+  otherDocshell->SetInFrameSwap(true);
> 
>   // Fire pageshow events on still-loading pages, and then fire pagehide
>   // events.  Note that we do NOT fire these in the normal way, but just fire
>   // them on the chrome event handlers.
>   nsContentUtils::FirePageShowEvent(ourDocshell, ourEventTarget, false);
>   nsContentUtils::FirePageShowEvent(otherDocshell, otherEventTarget, false);
>   nsContentUtils::FirePageHideEvent(ourDocshell, ourEventTarget);
>   nsContentUtils::FirePageHideEvent(otherDocshell, otherEventTarget);
>   
>   nsIFrame* ourFrame = ourContent->GetPrimaryFrame();
>   nsIFrame* otherFrame = otherContent->GetPrimaryFrame();
>   if (!ourFrame || !otherFrame) {
>     mInSwap = aOther->mInSwap = false;
>+    ourDocshell->SetInFrameSwap(false);
>+    otherDocshell->SetInFrameSwap(false);
>     nsContentUtils::FirePageShowEvent(ourDocshell, ourEventTarget, true);
>     nsContentUtils::FirePageShowEvent(otherDocshell, otherEventTarget, true);
>     return NS_ERROR_NOT_IMPLEMENTED;
>   }
> 
>   nsSubDocumentFrame* ourFrameFrame = do_QueryFrame(ourFrame);
>   if (!ourFrameFrame) {
>     mInSwap = aOther->mInSwap = false;
>+    ourDocshell->SetInFrameSwap(false);
>+    otherDocshell->SetInFrameSwap(false);
>     nsContentUtils::FirePageShowEvent(ourDocshell, ourEventTarget, true);
>     nsContentUtils::FirePageShowEvent(otherDocshell, otherEventTarget, true);
>     return NS_ERROR_NOT_IMPLEMENTED;
>   }
> 
>   // OK.  First begin to swap the docshells in the two nsIFrames
>   rv = ourFrameFrame->BeginSwapDocShells(otherFrame);
>   if (NS_FAILED(rv)) {
>     mInSwap = aOther->mInSwap = false;
>+    ourDocshell->SetInFrameSwap(false);
>+    otherDocshell->SetInFrameSwap(false);
>     nsContentUtils::FirePageShowEvent(ourDocshell, ourEventTarget, true);
>     nsContentUtils::FirePageShowEvent(otherDocshell, otherEventTarget, true);
>     return rv;
>   }
> 
>   // Now move the docshells to the right docshell trees.  Note that this
>   // resets their treeowners to null.
>   ourParentItem->RemoveChild(ourDocshell);
>@@ -1253,16 +1261,18 @@ nsFrameLoader::SwapWithOtherLoader(nsFrameLoader* aOther,
> 
>   ourParentDocument->FlushPendingNotifications(Flush_Layout);
>   otherParentDocument->FlushPendingNotifications(Flush_Layout);
> 
>   nsContentUtils::FirePageShowEvent(ourDocshell, ourEventTarget, true);
>   nsContentUtils::FirePageShowEvent(otherDocshell, otherEventTarget, true);
> 
>   mInSwap = aOther->mInSwap = false;
>+  ourDocshell->SetInFrameSwap(false);
>+  otherDocshell->SetInFrameSwap(false);
So, you call SetInFrameSwap(false) here _after_ FirePageShowEvent, but in the error cases _before_.
I think you should always call them after.
(mInSwap = aOther->mInSwap = false; has similar issue)

All this cries for some stack object which calls the right things when something fails. But not about this bug. 



With consistent SetInFrameSwap(false) ordering after FirePageShowEvent calls, r+
Attachment #8644710 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/abd120a5b640
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using Aurora 42.0a2 (buildID: 20150813124640).
Status: RESOLVED → VERIFIED
status-firefox42: fixed → verified
You need to log in before you can comment on or make changes to this bug.