Bug 1727514 - media playback should stop in the bfcache, r=peterv
authorOlli Pettay <Olli.Pettay@helsinki.fi>
Mon, 20 Sep 2021 08:49:22 +0000
changeset 592450 80a83d69906fc0ee1b01006b600c7955418b9f06
parent 592449 27cb0dad158d132ed4e803c4f6383a38737240ed
child 592451 60edafe5dcc3510cf8df8fae05addd1f81e3622b
push id38806
push userccozmuta@mozilla.com
push dateMon, 20 Sep 2021 16:24:09 +0000
treeherdermozilla-central@bf14cb5f9518 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspeterv
bugs1727514
milestone94.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1727514 - media playback should stop in the bfcache, r=peterv This version doesn't change SetContainer handling, since it seems to be tricky for the top level page. So only activity change notification is fired and IsActive() is updated. The comment about IsActive() was wrong even with the old bfcache implementation. (I did check that it returned false when the page was in bfcache and many of the activity observers rely on that) The changes to HTMLMediaElement are needed to ensure page can enter bfcache.. Differential Revision: https://phabricator.services.mozilla.com/D124684
docshell/base/BrowsingContext.cpp
docshell/base/nsDocShell.cpp
dom/base/Document.cpp
dom/base/Document.h
dom/html/HTMLMediaElement.cpp
dom/media/test/file_playback_and_bfcache.html
dom/media/test/mochitest.ini
dom/media/test/test_playback_and_bfcache.html
--- a/docshell/base/BrowsingContext.cpp
+++ b/docshell/base/BrowsingContext.cpp
@@ -2837,18 +2837,24 @@ void BrowsingContext::DidSet(FieldIndex<
     nsCOMPtr<nsIDocShell> shell = aContext->GetDocShell();
     if (shell) {
       static_cast<nsDocShell*>(shell.get())
           ->FirePageHideShowNonRecursive(!isInBFCache);
     }
   });
 
   if (isInBFCache) {
-    PreOrderWalk(
-        [&](BrowsingContext* aContext) { aContext->mIsInBFCache = true; });
+    PreOrderWalk([&](BrowsingContext* aContext) {
+      aContext->mIsInBFCache = true;
+      Document* doc = aContext->GetDocument();
+      if (doc) {
+        // Notifying needs to happen after mIsInBFCache is set to true.
+        doc->NotifyActivityChanged();
+      }
+    });
   }
 }
 
 void BrowsingContext::SetCustomPlatform(const nsAString& aPlatform,
                                         ErrorResult& aRv) {
   Top()->SetPlatformOverride(aPlatform, aRv);
 }
 
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@@ -1156,16 +1156,17 @@ void nsDocShell::FirePageHideShowNonRecu
   nsCOMPtr<nsIContentViewer> contentViewer(mContentViewer);
   if (aShow) {
     contentViewer->SetIsHidden(false);
     mRefreshURIList = std::move(mBFCachedRefreshURIList);
     RefreshURIFromQueue();
     mFiredUnloadEvent = false;
     RefPtr<Document> doc = contentViewer->GetDocument();
     if (doc) {
+      doc->NotifyActivityChanged();
       RefPtr<nsGlobalWindowInner> inner =
           mScriptGlobal ? mScriptGlobal->GetCurrentInnerWindowInternal()
                         : nullptr;
       if (mBrowsingContext->IsTop()) {
         doc->NotifyPossibleTitleChange(false);
         if (inner) {
           // Now that we have found the inner window of the page restored
           // from the history, we have to make sure that
--- a/dom/base/Document.cpp
+++ b/dom/base/Document.cpp
@@ -7400,17 +7400,17 @@ bool Document::ContainsMSEContent() {
       }
     }
   };
 
   EnumerateActivityObservers(check);
   return containsMSE;
 }
 
-static void NotifyActivityChanged(nsISupports* aSupports) {
+static void NotifyActivityChangedCallback(nsISupports* aSupports) {
   nsCOMPtr<nsIContent> content(do_QueryInterface(aSupports));
   if (auto mediaElem = HTMLMediaElement::FromNodeOrNull(content)) {
     mediaElem->NotifyOwnerDocumentActivityChanged();
   }
   nsCOMPtr<nsIObjectLoadingContent> objectLoadingContent(
       do_QueryInterface(aSupports));
   if (objectLoadingContent) {
     nsObjectLoadingContent* olc =
@@ -7426,16 +7426,20 @@ static void NotifyActivityChanged(nsISup
         do_QueryInterface(aSupports));
     if (imageLoadingContent) {
       auto ilc = static_cast<nsImageLoadingContent*>(imageLoadingContent.get());
       ilc->NotifyOwnerDocumentActivityChanged();
     }
   }
 }
 
+void Document::NotifyActivityChanged() {
+  EnumerateActivityObservers(NotifyActivityChangedCallback);
+}
+
 bool Document::IsTopLevelWindowInactive() const {
   if (BrowsingContext* bc = GetBrowsingContext()) {
     return !bc->GetIsActiveBrowserWindow();
   }
 
   return false;
 }
 
@@ -7444,17 +7448,17 @@ void Document::SetContainer(nsDocShell* 
     mDocumentContainer = aContainer;
   } else {
     mDocumentContainer = WeakPtr<nsDocShell>();
   }
 
   mInChromeDocShell =
       aContainer && aContainer->GetBrowsingContext()->IsChrome();
 
-  EnumerateActivityObservers(NotifyActivityChanged);
+  NotifyActivityChanged();
 
   // IsTopLevelWindowInactive depends on the docshell, so
   // update the cached value now that it's available.
   UpdateDocumentStates(NS_DOCUMENT_STATE_WINDOW_INACTIVE, false);
   if (!aContainer) {
     return;
   }
 
@@ -11268,17 +11272,17 @@ void Document::Destroy() {
 }
 
 void Document::RemovedFromDocShell() {
   mEditingState = EditingState::eOff;
 
   if (mRemovedFromDocShell) return;
 
   mRemovedFromDocShell = true;
-  EnumerateActivityObservers(NotifyActivityChanged);
+  NotifyActivityChanged();
 
   for (nsIContent* child = GetFirstChild(); child;
        child = child->GetNextSibling()) {
     child->SaveSubtreeState();
   }
 
   nsIDocShell* docShell = GetDocShell();
   if (docShell) {
@@ -11538,17 +11542,17 @@ void Document::OnPageShow(bool aPersiste
     // Set mIsShowing before firing events, in case those event handlers
     // move us around.
     mIsShowing = true;
     mVisible = true;
 
     UpdateVisibilityState();
   }
 
-  EnumerateActivityObservers(NotifyActivityChanged);
+  NotifyActivityChanged();
 
   auto notifyExternal = [aPersisted](Document& aExternalResource) {
     aExternalResource.OnPageShow(aPersisted, nullptr);
     return CallState::Continue;
   };
   EnumerateExternalResources(notifyExternal);
 
   if (mAnimationController) {
@@ -11662,17 +11666,17 @@ void Document::OnPageHide(bool aPersiste
     UpdateVisibilityState();
   }
 
   auto notifyExternal = [aPersisted](Document& aExternalResource) {
     aExternalResource.OnPageHide(aPersisted, nullptr);
     return CallState::Continue;
   };
   EnumerateExternalResources(notifyExternal);
-  EnumerateActivityObservers(NotifyActivityChanged);
+  NotifyActivityChanged();
 
   ClearPendingFullscreenRequests(this);
   if (GetUnretargetedFullScreenElement()) {
     // If this document was fullscreen, we should exit fullscreen in this
     // doctree branch. This ensures that if the user navigates while in
     // fullscreen mode we don't leave its still visible ancestor documents
     // in fullscreen mode. So exit fullscreen in the document's fullscreen
     // root document, as this will exit fullscreen in all the root's
@@ -12498,16 +12502,21 @@ void Document::SetSuppressedEventListene
   mSuppressedEventListener = aListener;
   auto setOnSubDocs = [&](Document& aDocument) {
     aDocument.SetSuppressedEventListener(aListener);
     return CallState::Continue;
   };
   EnumerateSubDocuments(setOnSubDocs);
 }
 
+bool Document::IsActive() const {
+  return mDocumentContainer && !mRemovedFromDocShell && GetBrowsingContext() &&
+         !GetBrowsingContext()->IsInBFCache();
+}
+
 nsISupports* Document::GetCurrentContentSink() {
   return mParser ? mParser->GetContentSink() : nullptr;
 }
 
 Document* Document::GetTemplateContentsOwner() {
   if (!mTemplateContentsOwner) {
     bool hasHadScriptObject = true;
     nsIScriptGlobalObject* scriptObject =
@@ -14916,17 +14925,17 @@ void Document::UpdateVisibilityState(Dis
   dom::VisibilityState oldState = mVisibilityState;
   mVisibilityState = ComputeVisibilityState();
   if (oldState != mVisibilityState) {
     if (aDispatchEvent == DispatchVisibilityChange::Yes) {
       nsContentUtils::DispatchTrustedEvent(this, ToSupports(this),
                                            u"visibilitychange"_ns,
                                            CanBubble::eYes, Cancelable::eNo);
     }
-    EnumerateActivityObservers(NotifyActivityChanged);
+    NotifyActivityChanged();
     if (mVisibilityState == dom::VisibilityState::Visible) {
       MaybeActiveMediaComponents();
     }
   }
 }
 
 VisibilityState Document::ComputeVisibilityState() const {
   // We have to check a few pieces of information here:
@@ -15561,17 +15570,17 @@ void Document::IncLazyLoadImageReachView
   if (aLoading) {
     ++mLazyLoadImageReachViewportLoading;
   } else {
     ++mLazyLoadImageReachViewportLoaded;
   }
 }
 
 void Document::NotifyLayerManagerRecreated() {
-  EnumerateActivityObservers(NotifyActivityChanged);
+  NotifyActivityChanged();
   EnumerateSubDocuments([](Document& aSubDoc) {
     aSubDoc.NotifyLayerManagerRecreated();
     return CallState::Continue;
   });
 }
 
 XPathEvaluator* Document::XPathEvaluator() {
   if (!mXPathEvaluator) {
--- a/dom/base/Document.h
+++ b/dom/base/Document.h
@@ -2649,21 +2649,21 @@ class Document : public nsINode,
   void SetSuppressedEventListener(EventListener* aListener);
 
   EventListener* GetSuppressedEventListener() {
     return mSuppressedEventListener;
   }
 
   /**
    * Return true when this document is active, i.e., an active document
-   * in a content viewer.  Note that this will return true for bfcached
-   * documents, so this does NOT match the "active document" concept in
-   * the WHATWG spec - see IsCurrentActiveDocument.
-   */
-  bool IsActive() const { return mDocumentContainer && !mRemovedFromDocShell; }
+   * in a content viewer and not in the bfcache.
+   * This does NOT match the "active document" concept in the WHATWG spec -
+   * see IsCurrentActiveDocument.
+   */
+  bool IsActive() const;
 
   /**
    * Return true if this is the current active document for its
    * docshell. Note that a docshell may have multiple active documents
    * due to the bfcache -- this should be used when you need to
    * differentiate the *current* active document from any active
    * documents.
    */
@@ -2696,16 +2696,18 @@ class Document : public nsINode,
    * nsIDocumentActivity or HTMLMEdiaElement.
    */
   void RegisterActivityObserver(nsISupports* aSupports);
   bool UnregisterActivityObserver(nsISupports* aSupports);
   // Enumerate all the observers in mActivityObservers by the aEnumerator.
   using ActivityObserverEnumerator = FunctionRef<void(nsISupports*)>;
   void EnumerateActivityObservers(ActivityObserverEnumerator aEnumerator);
 
+  void NotifyActivityChanged();
+
   // Indicates whether mAnimationController has been (lazily) initialized.
   // If this returns true, we're promising that GetAnimationController()
   // will have a non-null return value.
   bool HasAnimationController() { return !!mAnimationController; }
 
   // Getter for this document's SMIL Animation Controller. Performs lazy
   // initialization, if this document supports animation and if
   // mAnimationController isn't yet initialized.
--- a/dom/html/HTMLMediaElement.cpp
+++ b/dom/html/HTMLMediaElement.cpp
@@ -2561,30 +2561,30 @@ void HTMLMediaElement::SelectResource() 
     MediaResult rv = NewURIFromString(src, getter_AddRefs(uri));
     if (NS_SUCCEEDED(rv)) {
       LOG(LogLevel::Debug, ("%p Trying load from src=%s", this,
                             NS_ConvertUTF16toUTF8(src).get()));
       NS_ASSERTION(
           !mIsLoadingFromSourceChildren,
           "Should think we're not loading from source children by default");
 
-      if (!mMediaSource) {
-        OwnerDoc()->AddMediaElementWithMSE();
-      }
-
       RemoveMediaElementFromURITable();
       if (!mSrcMediaSource) {
         mLoadingSrc = uri;
       } else {
         mLoadingSrc = nullptr;
       }
       mLoadingSrcTriggeringPrincipal = mSrcAttrTriggeringPrincipal;
       DDLOG(DDLogCategory::Property, "loading_src",
             nsCString(NS_ConvertUTF16toUTF8(src)));
+      bool hadMediaSource = !!mMediaSource;
       mMediaSource = mSrcMediaSource;
+      if (mMediaSource && !hadMediaSource) {
+        OwnerDoc()->AddMediaElementWithMSE();
+      }
       DDLINKCHILD("mediasource", mMediaSource.get());
       UpdatePreloadAction();
       if (mPreloadAction == HTMLMediaElement::PRELOAD_NONE && !mMediaSource) {
         // preload:none media, suspend the load here before we make any
         // network requests.
         SuspendLoad();
         return;
       }
@@ -2828,26 +2828,26 @@ void HTMLMediaElement::LoadFromSourceChi
     NewURIFromString(src, getter_AddRefs(uri));
     if (!uri) {
       AutoTArray<nsString, 1> params = {src};
       ReportLoadError("MediaLoadInvalidURI", params);
       DealWithFailedElement(child);
       return;
     }
 
-    if (!mMediaSource) {
-      OwnerDoc()->AddMediaElementWithMSE();
-    }
-
     RemoveMediaElementFromURITable();
     mLoadingSrc = uri;
     mLoadingSrcTriggeringPrincipal = childSrc->GetSrcTriggeringPrincipal();
     DDLOG(DDLogCategory::Property, "loading_src",
           nsCString(NS_ConvertUTF16toUTF8(src)));
+    bool hadMediaSource = !!mMediaSource;
     mMediaSource = childSrc->GetSrcMediaSource();
+    if (mMediaSource && !hadMediaSource) {
+      OwnerDoc()->AddMediaElementWithMSE();
+    }
     DDLINKCHILD("mediasource", mMediaSource.get());
     NS_ASSERTION(mNetworkState == NETWORK_LOADING,
                  "Network state should be loading");
 
     if (mPreloadAction == HTMLMediaElement::PRELOAD_NONE && !mMediaSource) {
       // preload:none media, suspend the load here before we make any
       // network requests.
       SuspendLoad();
new file mode 100644
--- /dev/null
+++ b/dom/media/test/file_playback_and_bfcache.html
@@ -0,0 +1,57 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <script>
+  function init() {
+    if (location.search == "") {
+      let bc1 = new BroadcastChannel("bc1");
+      bc1.onmessage = function(e) {
+        if (e.data == "loadNext") {
+          location.href = location.href + "?page2";
+        } else if (e.data == "forward") {
+          bc1.close();
+          history.forward();
+        }
+      };
+      window.onpageshow = function() {
+        bc1.postMessage("pageshow");
+      };
+    } else {
+      document.body.innerHTML = "<video controls src='owl.mp3' autoplay>";
+      let bc2 = new BroadcastChannel("bc2");
+      bc2.onmessage = function(e) {
+        if (e.data == "back") {
+          history.back();
+        } else if (e.data == "statistics") {
+          bc2.postMessage({ currentTime: document.body.firstChild.currentTime,
+                          duration: document.body.firstChild.duration });
+          bc2.close();
+          window.close();
+        }
+      }
+      window.onpageshow = function(e) {
+        bc2.postMessage({ event: "pageshow", persisted: e.persisted});
+        if (!e.persisted) {
+          // The initial statistics is sent once we know the duration and
+          // have loaded all the data.
+          let mediaElement = document.body.firstChild;
+          mediaElement.onpause = function() {
+            mediaElement.onpause = null;
+            mediaElement.currentTime = 0;
+            mediaElement.onplay = function() {
+              setTimeout(function() {
+                bc2.postMessage({ currentTime: mediaElement.currentTime,
+                                  duration: mediaElement.duration });
+                }, 500);
+            }
+            mediaElement.play();
+          }
+        }
+      };
+    }
+  }
+  </script>
+</head>
+<body onload="init()">
+</body>
+</html>
--- a/dom/media/test/mochitest.ini
+++ b/dom/media/test/mochitest.ini
@@ -1098,16 +1098,18 @@ tags=promise-play
 tags=promise-play
 [test_play_twice.html]
 skip-if = appname == "seamonkey" #  Seamonkey: Bug 598252, bug 1307337, bug 1143695
 # If encountering intermittents in test_playback.html please consider disabling
 # the individual faulting file via `manifest.js` as disabling the whole test on
 # a platform removes a lot of coverage.
 [test_playback.html]
 skip-if = toolkit == 'android' # bug 1316177
+[test_playback_and_bfcache.html]
+support-files = file_playback_and_bfcache.html
 [test_playback_errors.html]
 [test_playback_rate.html]
 [test_playback_rate_playpause.html]
 [test_playback_reactivate.html]
 [test_played.html]
 skip-if = toolkit == 'android' && is_emulator # Times out on android-em, Bug 1613946
 [test_preload_actions.html]
 [test_preload_attribute.html]
new file mode 100644
--- /dev/null
+++ b/dom/media/test/test_playback_and_bfcache.html
@@ -0,0 +1,72 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Test media playback and bfcache</title>
+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" href="/tests/SimpleTest/test.css"/>
+  <script>
+    SimpleTest.requestFlakyTimeout("Need some timer to wait for the audio to play");
+    SimpleTest.waitForExplicitFinish();
+    var duration = 0;
+
+    // The test opens a page and another page with a media element is loaded.
+    // The media element plays an audio file and starts again and sends
+    // statistics about it and then history.back() is called. The test waits
+    // for 1s + duration of the audio file and goes forward. The audio playback
+    // shouldn't have progressed while the page was in the bfcache.
+    function test() {
+      let bc1 = new BroadcastChannel("bc1");
+      let pageshow1Count = 0;
+      bc1.onmessage = function(e) {
+        if (e.data == "pageshow") {
+          ++pageshow1Count;
+          info("Page 1 pageshow " + pageshow1Count);
+          if (pageshow1Count == 1) {
+            bc1.postMessage("loadNext");
+          } else if (pageshow1Count == 2) {
+            setTimeout(function() {
+                bc1.postMessage("forward");
+                bc1.close();
+              }, (Math.round(duration) + 1) * 1000);
+          }
+        }
+      };
+
+      let bc2 = new BroadcastChannel("bc2");
+      let pageshow2Count = 0;
+      let statisticsCount = 0;
+      bc2.onmessage = function(e) {
+        if (e.data.event == "pageshow") {
+          ++pageshow2Count;
+          info("Page 2 pageshow " + pageshow2Count);
+          if (pageshow2Count == 2) {
+            ok(e.data.persisted, "Should have persisted the page.");
+            bc2.postMessage("statistics");
+          }
+        } else {
+          ++statisticsCount;
+          if (statisticsCount == 1) {
+            duration = e.data.duration;
+            bc2.postMessage("back");
+          } else {
+            is(statisticsCount, 2, "Should got two play events.");
+            ok(e.data.currentTime < e.data.duration,
+               "Should have stopped the playback while the page was in bfcache." +
+               "currentTime: " + e.data.currentTime + " duration: " + e.data.duration);
+            bc2.close();
+            SimpleTest.finish();
+          }
+        }
+      };
+
+      window.open("file_playback_and_bfcache.html", "", "noopener");
+    }
+  </script>
+</head>
+<body onload="test()">
+<p id="display"></p>
+<div id="content" style="display: none"></div>
+<pre id="test"></pre>
+</body>
+</html>