Bug 1727514 - media playback should stop in the bfcache, r=peterv
authorOlli Pettay <Olli.Pettay@helsinki.fi>
Mon, 13 Sep 2021 12:40:18 +0000
changeset 591740 2d49d73f38aacf9b5b0b211574b60152f413d0a3
parent 591739 0bf41da9f46737d2c351a609e0ccbf4f3d7bcc21
child 591741 95d231577e5f30aa16f6ee42cb7691c3d63f781a
push id38786
push usermlaza@mozilla.com
push dateMon, 13 Sep 2021 21:32:24 +0000
treeherdermozilla-central@b50ef8e31c4c [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 SetContainer handling is similar to what DocumentViewer does for the old bfcache implementation. (The old implementation hides it quite well). The changes to HTMLMediaElement are needed to ensure page can enter bfcache. Removed erroneous MOZ_ASSERT in nsPresContext, it is ok to trigger that code path in the new implementation. And the Run() method of the relevant nsIRunnable already deals with that case. 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
layout/base/nsPresContext.cpp
--- 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) {
+        // Container needs to be cleared after mIsInBFCache is set to true.
+        doc->SetContainer(nullptr);
+      }
+    });
   }
 }
 
 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->SetContainer(this);
       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
@@ -6928,18 +6928,21 @@ bool Document::RemoveFromBFCacheSync() {
     // before nsIContentViewer.show() has been called, the previous page doesn't
     // yet have nsIBFCacheEntry. However, the previous page isn't the current
     // active document anymore.
     DisallowBFCaching();
     removed = true;
   }
 
   if (mozilla::SessionHistoryInParent() && XRE_IsContentProcess()) {
-    if (BrowsingContext* bc = GetBrowsingContext()) {
-      if (bc->IsInBFCache()) {
+    // Note, Document::GetBrowsingContext() returns null when the document is in
+    // the bfcache.
+    if (nsPIDOMWindowInner* innerWindow = GetInnerWindow()) {
+      BrowsingContext* bc = innerWindow->GetBrowsingContext();
+      if (bc && bc->IsInBFCache()) {
         ContentChild* cc = ContentChild::GetSingleton();
         // IPC is asynchronous but the caller is supposed to check the return
         // value. The reason for 'Sync' in the method name is that the old
         // implementation may run scripts. There is Async variant in
         // the old session history implementation for the cases where
         // synchronous operation isn't safe.
         cc->SendRemoveFromBFCache(bc->Top());
         removed = true;
--- a/dom/base/Document.h
+++ b/dom/base/Document.h
@@ -3645,16 +3645,18 @@ class Document : public nsINode,
 
   // Return true if there is transient user gesture activation and it hasn't yet
   // timed out.
   bool HasValidTransientUserGestureActivation() const;
 
   // Return true.
   bool ConsumeTransientUserGestureActivation();
 
+  // Note, GetBrowsingContext() returns null when the document is in
+  // the bfcache.
   BrowsingContext* GetBrowsingContext() const;
 
   // This document is a WebExtension page, it might be a background page, a
   // popup, a visible tab, a visible iframe ...e.t.c.
   bool IsExtensionPage() const;
 
   bool HasScriptsBlockedBySandbox();
 
--- 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>
--- a/layout/base/nsPresContext.cpp
+++ b/layout/base/nsPresContext.cpp
@@ -2073,20 +2073,17 @@ class DelayedFireDOMPaintEvent : public 
   DelayedFireDOMPaintEvent(
       nsPresContext* aPresContext, nsTArray<nsRect>&& aList,
       TransactionId aTransactionId,
       const mozilla::TimeStamp& aTimeStamp = mozilla::TimeStamp())
       : mozilla::Runnable("DelayedFireDOMPaintEvent"),
         mPresContext(aPresContext),
         mTransactionId(aTransactionId),
         mTimeStamp(aTimeStamp),
-        mList(std::move(aList)) {
-    MOZ_ASSERT(mPresContext->GetContainerWeak(),
-               "DOMPaintEvent requested for a detached pres context");
-  }
+        mList(std::move(aList)) {}
   NS_IMETHOD Run() override {
     // The pres context might have been detached during the delay -
     // that's fine, just don't fire the event.
     if (mPresContext->GetContainerWeak()) {
       mPresContext->FireDOMPaintEvent(&mList, mTransactionId, mTimeStamp);
     }
     return NS_OK;
   }