From 9b2d8200a8fb60a1b46ffcd0c3d76a0f02a64497 Mon Sep 17 00:00:00 2001 From: Edgar Chen Date: Tue, 20 Dec 2022 20:17:33 +0000 Subject: [PATCH] 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 | 192 ++++++++++++------ dom/base/nsGlobalWindowOuter.h | 30 ++- ...fullscreen-document-mutation-navigation.js | 7 +- .../web-platform/meta/fullscreen/__dir__.ini | 1 + ...element-request-fullscreen-timing.html.ini | 7 +- widget/gtk/nsWindow.cpp | 12 +- xpfe/appshell/AppWindow.cpp | 5 +- 7 files changed, 171 insertions(+), 83 deletions(-) create mode 100644 testing/web-platform/meta/fullscreen/__dir__.ini diff --git a/dom/base/nsGlobalWindowOuter.cpp b/dom/base/nsGlobalWindowOuter.cpp index e168d6bffa5f..e330712345c2 100644 --- a/dom/base/nsGlobalWindowOuter.cpp +++ b/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 treeOwnerAsWin = GetTreeOwnerWindow(); + nsCOMPtr 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 treeOwnerAsWin = GetTreeOwnerWindow(); - nsCOMPtr 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 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 widget = GetNearestWidget()) { // We are in content process, figure out the value from @@ -4612,7 +4676,7 @@ bool nsGlobalWindowOuter::Fullscreen() const { } nsCOMPtr window = rootItem->GetWindow(); - NS_ENSURE_TRUE(window, mFullscreen); + NS_ENSURE_TRUE(window, mFullscreen.isSome()); return nsGlobalWindowOuter::Cast(window)->Fullscreen(); } diff --git a/dom/base/nsGlobalWindowOuter.h b/dom/base/nsGlobalWindowOuter.h index d4a5c1a357d5..b7ebe5b2c04c 100644 --- a/dom/base/nsGlobalWindowOuter.h +++ b/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 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 mInProcessFullscreenRequest; + bool mForceFullScreenInWidget : 1; bool mIsClosed : 1; bool mInClose : 1; diff --git a/dom/base/test/fullscreen/browser_fullscreen-document-mutation-navigation.js b/dom/base/test/fullscreen/browser_fullscreen-document-mutation-navigation.js index dc71c547d30b..67c86580d23d 100644 --- a/dom/base/test/fullscreen/browser_fullscreen-document-mutation-navigation.js +++ b/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. diff --git a/testing/web-platform/meta/fullscreen/__dir__.ini b/testing/web-platform/meta/fullscreen/__dir__.ini new file mode 100644 index 000000000000..a04152c81408 --- /dev/null +++ b/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] diff --git a/testing/web-platform/meta/fullscreen/api/element-request-fullscreen-timing.html.ini b/testing/web-platform/meta/fullscreen/api/element-request-fullscreen-timing.html.ini index eda4d1ff4298..282008ed89f3 100644 --- a/testing/web-platform/meta/fullscreen/api/element-request-fullscreen-timing.html.ini +++ b/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 + diff --git a/widget/gtk/nsWindow.cpp b/widget/gtk/nsWindow.cpp index f398634f850c..70f27f3d552e 100644 --- a/widget/gtk/nsWindow.cpp +++ b/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; diff --git a/xpfe/appshell/AppWindow.cpp b/xpfe/appshell/AppWindow.cpp index c79298cc5b54..b11753e2196f 100644 --- a/xpfe/appshell/AppWindow.cpp +++ b/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