Bug 1801391 - Avoid handling multiple widget fullscreen at the same time; r=smaug

We had some protection in https://searchfox.org/mozilla-central/rev/2fc2ccf960c2f7c419262ac7215715c5235948db/dom/base/nsGlobalWindowOuter.cpp#4259-4265,
but it isn't enough, it is still possible that fullscreen state get confused if
there are multiple request happens at a short time.

This patch tries to improve how we handle multiple request, we don't need to
track each request, but need to ensure widget matches with the latest fullscreen
state specified.

Differential Revision: https://phabricator.services.mozilla.com/D163311

dom/base/nsGlobalWindowOuter.cpp

@@ -1296,8 +1296,7 @@ static JSObject* NewOuterWindowProxy(JSContext* cx,
 
 nsGlobalWindowOuter::nsGlobalWindowOuter(uint64_t aWindowID)
     : nsPIDOMWindowOuter(aWindowID),
-      mFullscreen(false),
-      mFullscreenMode(false),
+      mFullscreenHasChangedDuringProcessing(false),
       mForceFullScreenInWidget(false),
       mIsClosed(false),
       mInClose(false),
@@ -4247,15 +4246,6 @@ FullscreenTransitionTask::Run() {
   } else if (stage == eToggleFullscreen) {
     PROFILER_MARKER_UNTYPED("Fullscreen toggle start", DOM);
     mFullscreenChangeStartTime = TimeStamp::Now();
-    if (MOZ_UNLIKELY(mWindow->mFullscreen != mFullscreen)) {
-      // This could happen in theory if several fullscreen requests in
-      // different direction happen continuously in a short time. We
-      // need to ensure the fullscreen state matches our target here,
-      // otherwise the widget would change the window state as if we
-      // toggle for Fullscreen Mode instead of Fullscreen API.
-      NS_WARNING("The fullscreen state of the window does not match");
-      mWindow->mFullscreen = mFullscreen;
-    }
     // Toggle the fullscreen state on the widget
     if (!mWindow->SetWidgetFullscreen(FullscreenReason::ForFullscreenAPI,
                                       mFullscreen, mWidget)) {
@@ -4366,6 +4356,44 @@ static bool MakeWidgetFullscreen(nsGlobalWindowOuter* aWindow,
   return true;
 }
 
+nsresult nsGlobalWindowOuter::ProcessWidgetFullscreenRequest(
+    FullscreenReason aReason, bool aFullscreen) {
+  mInProcessFullscreenRequest.emplace(aReason, aFullscreen);
+
+  // Prevent chrome documents which are still loading from resizing
+  // the window after we set fullscreen mode.
+  nsCOMPtr<nsIBaseWindow> treeOwnerAsWin = GetTreeOwnerWindow();
+  nsCOMPtr<nsIAppWindow> appWin(do_GetInterface(treeOwnerAsWin));
+  if (aFullscreen && appWin) {
+    appWin->SetIntrinsicallySized(false);
+  }
+
+  // Sometimes we don't want the top-level widget to actually go fullscreen:
+  // - in the B2G desktop client, we don't want the emulated screen dimensions
+  //   to appear to increase when entering fullscreen mode; we just want the
+  //   content to fill the entire client area of the emulator window.
+  // - in FxR Desktop, we don't want fullscreen to take over the monitor, but
+  //   instead we want fullscreen to fill the FxR window in the the headset.
+  if (!Preferences::GetBool("full-screen-api.ignore-widgets", false) &&
+      !mForceFullScreenInWidget) {
+    if (MakeWidgetFullscreen(this, aReason, aFullscreen)) {
+      // The rest of code for switching fullscreen is in nsGlobalWindowOuter::
+      // FinishFullscreenChange() which will be called after sizemodechange
+      // event is dispatched.
+      return NS_OK;
+    }
+  }
+
+#if defined(NIGHTLY_BUILD) && defined(XP_WIN)
+  if (FxRWindowManager::GetInstance()->IsFxRWindow(mWindowID)) {
+    mozilla::gfx::VRShMem shmem(nullptr, true /*aRequiresMutex*/);
+    shmem.SendFullscreenState(mWindowID, aFullscreen);
+  }
+#endif  // NIGHTLY_BUILD && XP_WIN
+  FinishFullscreenChange(aFullscreen);
+  return NS_OK;
+}
+
 nsresult nsGlobalWindowOuter::SetFullscreenInternal(FullscreenReason aReason,
                                                     bool aFullscreen) {
   MOZ_ASSERT(nsContentUtils::IsSafeToRunScript(),
@@ -4402,8 +4430,27 @@ nsresult nsGlobalWindowOuter::SetFullscreenInternal(FullscreenReason aReason,
   if (mDocShell->ItemType() != nsIDocShellTreeItem::typeChrome)
     return NS_ERROR_FAILURE;
 
-  // If we are already in full screen mode, just return.
-  if (mFullscreen == aFullscreen) {
+  // FullscreenReason::ForForceExitFullscreen can only be used with exiting
+  // fullscreen
+  MOZ_ASSERT_IF(
+      mFullscreen.isSome(),
+      mFullscreen.value() != FullscreenReason::ForForceExitFullscreen);
+
+  // If we are already in full screen mode, just return, we don't care about the
+  // reason here, because,
+  // - If we are in fullscreen mode due to browser fullscreen mode, requesting
+  //   DOM fullscreen does not change anything.
+  // - If we are in fullscreen mode due to DOM fullscreen, requesting browser
+  //   fullscreen should not change anything, either. Note that we should not
+  //   update reason to ForFullscreenMode, otherwise the subsequent DOM
+  //   fullscreen exit will be ignored and user will be confused. And ideally
+  //   this should never happen as `window.fullscreen` returns `true` for DOM
+  //   fullscreen as well.
+  if (mFullscreen.isSome() == aFullscreen) {
+    // How come we get browser fullscreen request while we are already in DOM
+    // fullscreen?
+    MOZ_ASSERT_IF(aFullscreen && aReason == FullscreenReason::ForFullscreenMode,
+                  mFullscreen.value() != FullscreenReason::ForFullscreenAPI);
     return NS_OK;
   }
 
@@ -4411,61 +4458,45 @@ nsresult nsGlobalWindowOuter::SetFullscreenInternal(FullscreenReason aReason,
   // consequential calls to this method, those calls will be skipped
   // at the condition above.
   if (aReason == FullscreenReason::ForFullscreenMode) {
-    if (!aFullscreen && !mFullscreenMode) {
+    if (!aFullscreen && mFullscreen &&
+        mFullscreen.value() == FullscreenReason::ForFullscreenAPI) {
       // If we are exiting fullscreen mode, but we actually didn't
-      // entered fullscreen mode, the fullscreen state was only for
+      // entered browser fullscreen mode, the fullscreen state was only for
       // the Fullscreen API. Change the reason here so that we can
       // perform transition for it.
       aReason = FullscreenReason::ForFullscreenAPI;
-    } else {
-      mFullscreenMode = aFullscreen;
     }
   } else {
     // If we are exiting from DOM fullscreen while we initially make
-    // the window fullscreen because of fullscreen mode, don't restore
+    // the window fullscreen because of browser fullscreen mode, don't restore
     // the window. But we still need to exit the DOM fullscreen state.
-    if (!aFullscreen && mFullscreenMode) {
-      FinishDOMFullscreenChange(mDoc, false);
+    if (!aFullscreen && mFullscreen &&
+        mFullscreen.value() == FullscreenReason::ForFullscreenMode) {
+      // If there is a in-process fullscreen request, FinishDOMFullscreenChange
+      // will be called when the request is finished.
+      if (!mInProcessFullscreenRequest.isSome()) {
+        FinishDOMFullscreenChange(mDoc, false);
+      }
       return NS_OK;
     }
   }
 
-  // Prevent chrome documents which are still loading from resizing
-  // the window after we set fullscreen mode.
-  nsCOMPtr<nsIBaseWindow> treeOwnerAsWin = GetTreeOwnerWindow();
-  nsCOMPtr<nsIAppWindow> appWin(do_GetInterface(treeOwnerAsWin));
-  if (aFullscreen && appWin) {
-    appWin->SetIntrinsicallySized(false);
-  }
-
   // Set this before so if widget sends an event indicating its
   // gone full screen, the state trap above works.
-  mFullscreen = aFullscreen;
-
-  // Sometimes we don't want the top-level widget to actually go fullscreen:
-  // - in the B2G desktop client, we don't want the emulated screen dimensions
-  //   to appear to increase when entering fullscreen mode; we just want the
-  //   content to fill the entire client area of the emulator window.
-  // - in FxR Desktop, we don't want fullscreen to take over the monitor, but
-  //   instead we want fullscreen to fill the FxR window in the the headset.
-  if (!Preferences::GetBool("full-screen-api.ignore-widgets", false) &&
-      !mForceFullScreenInWidget) {
-    if (MakeWidgetFullscreen(this, aReason, aFullscreen)) {
-      // The rest of code for switching fullscreen is in nsGlobalWindowOuter::
-      // FinishFullscreenChange() which will be called after sizemodechange
-      // event is dispatched.
-      return NS_OK;
-    }
+  if (aFullscreen) {
+    mFullscreen.emplace(aReason);
+  } else {
+    mFullscreen.reset();
   }
 
-#if defined(NIGHTLY_BUILD) && defined(XP_WIN)
-  if (FxRWindowManager::GetInstance()->IsFxRWindow(mWindowID)) {
-    mozilla::gfx::VRShMem shmem(nullptr, true /*aRequiresMutex*/);
-    shmem.SendFullscreenState(mWindowID, aFullscreen);
+  // If we are in process of fullscreen request, only keep the latest fullscreen
+  // state, we will sync up later while the processing request is finished.
+  if (mInProcessFullscreenRequest.isSome()) {
+    mFullscreenHasChangedDuringProcessing = true;
+    return NS_OK;
   }
-#endif  // NIGHTLY_BUILD && XP_WIN
-  FinishFullscreenChange(aFullscreen);
-  return NS_OK;
+
+  return ProcessWidgetFullscreenRequest(aReason, aFullscreen);
 }
 
 // Support a per-window, dynamic equivalent of enabling
@@ -4507,6 +4538,21 @@ bool nsGlobalWindowOuter::SetWidgetFullscreen(FullscreenReason aReason,
 
 /* virtual */
 void nsGlobalWindowOuter::FullscreenWillChange(bool aIsFullscreen) {
+  if (!mInProcessFullscreenRequest.isSome()) {
+    // If there is no in-process fullscreen request, the fullscreen state change
+    // is triggered from the OS directly, e.g. user use built-in window button
+    // to enter/exit fullscreen on macOS.
+    MOZ_ASSERT(mFullscreen.isSome() != aIsFullscreen,
+               "FullscreenWillChange should not be notified if the fullscreen "
+               "state isn't changed");
+    mInProcessFullscreenRequest.emplace(FullscreenReason::ForFullscreenMode,
+                                        aIsFullscreen);
+    if (aIsFullscreen) {
+      mFullscreen.emplace(FullscreenReason::ForFullscreenMode);
+    } else {
+      mFullscreen.reset();
+    }
+  }
   if (aIsFullscreen) {
     DispatchCustomEvent(u"willenterfullscreen"_ns, ChromeOnlyDispatch::eYes);
   } else {
@@ -4516,23 +4562,23 @@ void nsGlobalWindowOuter::FullscreenWillChange(bool aIsFullscreen) {
 
 /* virtual */
 void nsGlobalWindowOuter::FinishFullscreenChange(bool aIsFullscreen) {
-  if (aIsFullscreen != mFullscreen) {
+  mozilla::Maybe<FullscreenRequest> currentInProcessRequest =
+      std::move(mInProcessFullscreenRequest);
+  if (!mFullscreenHasChangedDuringProcessing &&
+      aIsFullscreen != mFullscreen.isSome()) {
     NS_WARNING("Failed to toggle fullscreen state of the widget");
     // We failed to make the widget enter fullscreen.
     // Stop further changes and restore the state.
     if (!aIsFullscreen) {
-      mFullscreen = false;
-      mFullscreenMode = false;
+      mFullscreen.reset();
     } else {
 #ifndef XP_MACOSX
       MOZ_ASSERT_UNREACHABLE("Failed to exit fullscreen?");
 #endif
-      mFullscreen = true;
-      // At least on macOS, we may reach here because the system fails
-      // to let us exit the system fullscreen mode. In that case, we may
-      // have already exited DOM fullscreen before, so set fullscreen
-      // mode to true here so that it has a saner state.
-      mFullscreenMode = true;
+      // Restore fullscreen state with FullscreenReason::ForFullscreenAPI reason
+      // in order to make subsequent DOM fullscreen exit request can exit
+      // browser fullscreen mode.
+      mFullscreen.emplace(FullscreenReason::ForFullscreenAPI);
     }
     return;
   }
@@ -4541,7 +4587,7 @@ void nsGlobalWindowOuter::FinishFullscreenChange(bool aIsFullscreen) {
   // of the document before dispatching the "fullscreen" event, so
   // that the chrome can distinguish between browser fullscreen mode
   // and DOM fullscreen.
-  FinishDOMFullscreenChange(mDoc, mFullscreen);
+  FinishDOMFullscreenChange(mDoc, aIsFullscreen);
 
   // dispatch a "fullscreen" DOM event so that XUL apps can
   // respond visually if we are kicked into full screen mode
@@ -4556,6 +4602,24 @@ void nsGlobalWindowOuter::FinishFullscreenChange(bool aIsFullscreen) {
       mChromeFields.mFullscreenPresShell = nullptr;
     }
   }
+
+  // If fullscreen state has changed during processing fullscreen request, we
+  // need to ensure widget matches our latest fullscreen state here.
+  if (mFullscreenHasChangedDuringProcessing) {
+    mFullscreenHasChangedDuringProcessing = false;
+    // Widget doesn't care about the reason that makes it entering/exiting
+    // fullscreen, so here we just need to ensure the fullscreen state is
+    // matched.
+    if (aIsFullscreen != mFullscreen.isSome()) {
+      // If we end up need to exit fullscreen, use the same reason that brings
+      // us into fullscreen mode, so that we will perform the same fullscreen
+      // transistion effect for exiting.
+      ProcessWidgetFullscreenRequest(
+          mFullscreen.isSome() ? mFullscreen.value()
+                               : currentInProcessRequest.value().mReason,
+          mFullscreen.isSome());
+    }
+  }
 }
 
 /* virtual */
@@ -4592,7 +4656,7 @@ void nsGlobalWindowOuter::MacFullscreenMenubarOverlapChanged(
 }
 
 bool nsGlobalWindowOuter::Fullscreen() const {
-  NS_ENSURE_TRUE(mDocShell, mFullscreen);
+  NS_ENSURE_TRUE(mDocShell, mFullscreen.isSome());
 
   // Get the fullscreen value of the root window, to always have the value
   // accurate, even when called from content.
@@ -4601,7 +4665,7 @@ bool nsGlobalWindowOuter::Fullscreen() const {
   if (rootItem == mDocShell) {
     if (!XRE_IsContentProcess()) {
       // We are the root window. Return our internal value.
-      return mFullscreen;
+      return mFullscreen.isSome();
     }
     if (nsCOMPtr<nsIWidget> widget = GetNearestWidget()) {
       // We are in content process, figure out the value from
@@ -4612,7 +4676,7 @@ bool nsGlobalWindowOuter::Fullscreen() const {
   }
 
   nsCOMPtr<nsPIDOMWindowOuter> window = rootItem->GetWindow();
-  NS_ENSURE_TRUE(window, mFullscreen);
+  NS_ENSURE_TRUE(window, mFullscreen.isSome());
 
   return nsGlobalWindowOuter::Cast(window)->Fullscreen();
 }

dom/base/nsGlobalWindowOuter.h

@@ -1056,8 +1056,34 @@ class nsGlobalWindowOuter final : public mozilla::dom::EventTarget,
       mozilla::TaskCategory aCategory) override;
 
  protected:
-  bool mFullscreen : 1;
-  bool mFullscreenMode : 1;
+  nsresult ProcessWidgetFullscreenRequest(FullscreenReason aReason,
+                                          bool aFullscreen);
+
+  // Indicates whether browser window should be in fullscreen mode and the
+  // reason, e.g. browser fullscreen mode or DOM fullscreen API, which should
+  // never be ForForceExitFullscreen. Nothing if browser window should not be in
+  // fullscreen mode.
+  mozilla::Maybe<FullscreenReason> mFullscreen;
+
+  // Indicates whether new fullscreen request have been made when previous
+  // fullscreen request is still in-process.
+  bool mFullscreenHasChangedDuringProcessing : 1;
+
+  using FullscreenRequest = struct FullscreenRequest {
+    FullscreenRequest(FullscreenReason aReason, bool aFullscreen)
+        : mReason(aReason), mFullscreen(aFullscreen) {
+      MOZ_ASSERT(
+          mReason != FullscreenReason::ForForceExitFullscreen || !mFullscreen,
+          "FullscreenReason::ForForceExitFullscreen can only be used with "
+          "exiting fullscreen");
+    }
+    FullscreenReason mReason;
+    bool mFullscreen : 1;
+  };
+  // The current in-process fullscreen request. Nothing if there is no
+  // in-process request.
+  mozilla::Maybe<FullscreenRequest> mInProcessFullscreenRequest;
+
   bool mForceFullScreenInWidget : 1;
   bool mIsClosed : 1;
   bool mInClose : 1;

dom/base/test/fullscreen/browser_fullscreen-document-mutation-navigation.js

@@ -58,13 +58,16 @@ async function startTests(testFun, name) {
           // process, so it could be possible that the widget or the chrome
           // document goes into fullscreen mode again, but they should end up
           // leaving fullscreen mode again.
-          if (window.fullScreen) {
+          if (
+            window.fullScreen ||
+            document.documentElement.hasAttribute("inFullscreen")
+          ) {
             info("widget is still in fullscreen, wait again");
             await waitWidgetFullscreenEvent(window, false, true);
           }
           if (document.documentElement.hasAttribute("inDOMFullscreen")) {
             info("chrome document is still in fullscreen, wait again");
-            await waitDOMFullscreenEvent(document, false, true);
+            await waitForFullScreenObserver(document, false, true);
           }
 
           // Ensure the browser exits fullscreen state.

testing/web-platform/meta/fullscreen/__dir__.ini

@@ -0,0 +1 @@
+prefs: [full-screen-api.transition-duration.enter:0 0, full-screen-api.transition-duration.leave:0 0]

testing/web-platform/meta/fullscreen/api/element-request-fullscreen-timing.html.ini

@@ -1,9 +1,4 @@
 [element-request-fullscreen-timing.html]
   expected:
     if (os == "android") and fission: [OK, TIMEOUT]
-  [Timing of fullscreenchange and resize events]
-    expected:
-      if (os == "win") and debug: [PASS, FAIL]
-      if (os == "mac") and not debug: [FAIL, PASS]
-      if os == "linux": PASS
-      FAIL
+

widget/gtk/nsWindow.cpp

@@ -5228,10 +5228,14 @@ void nsWindow::OnWindowStateEvent(GtkWidget* aWidget,
 
   if (mWidgetListener) {
     if (mSizeMode != oldSizeMode) {
-      mWidgetListener->SizeModeChanged(mSizeMode);
       if (mSizeMode == nsSizeMode_Fullscreen ||
           oldSizeMode == nsSizeMode_Fullscreen) {
-        mWidgetListener->FullscreenChanged(mSizeMode == nsSizeMode_Fullscreen);
+        bool isFullscreen = mSizeMode == nsSizeMode_Fullscreen;
+        mWidgetListener->FullscreenWillChange(isFullscreen);
+        mWidgetListener->SizeModeChanged(mSizeMode);
+        mWidgetListener->FullscreenChanged(isFullscreen);
+      } else {
+        mWidgetListener->SizeModeChanged(mSizeMode);
       }
     }
   }
@@ -7349,10 +7353,6 @@ nsresult nsWindow::MakeFullScreen(bool aFullScreen) {
   }
 
   const bool wasFullscreen = mSizeMode == nsSizeMode_Fullscreen;
-  if (aFullScreen != wasFullscreen && mWidgetListener) {
-    mWidgetListener->FullscreenWillChange(aFullScreen);
-  }
-
   if (aFullScreen) {
     if (!wasFullscreen) {
       mLastSizeModeBeforeFullscreen = mSizeMode;

xpfe/appshell/AppWindow.cpp

@@ -2897,9 +2897,8 @@ void AppWindow::SizeModeChanged(nsSizeMode aSizeMode) {
   if (ourWindow) {
     // Ensure that the fullscreen state is synchronized between
     // the widget and the outer window object.
-    if (aSizeMode == nsSizeMode_Fullscreen) {
-      ourWindow->SetFullScreen(true);
-    } else if (aSizeMode != nsSizeMode_Minimized) {
+    if (aSizeMode != nsSizeMode_Fullscreen &&
+        aSizeMode != nsSizeMode_Minimized) {
       if (ourWindow->GetFullScreen()) {
         // The first SetFullscreenInternal call below ensures that we do
         // not trigger any fullscreen transition even if the window was