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.]

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.

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.

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.

(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?

(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.

(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.

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.

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.

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+

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using Aurora 42.0a2 (buildID: 20150813124640).