ma1 pushed to branch base-browser-115.1.0esr-13.0-1 at The Tor Project / Applications / Tor Browser
Commits: 45b89a93 by Edgar Chen at 2023-08-01T17:39:54+02:00 Bug 1821884 - Ensure consistent state for fullscreen/pointerlock warnings; r=Gijs
Fullscreen/PointerLock warnings are initialized with hidden="true", but change to hidden="" after being shown and hidden again. I think this started happening when we began using HTML elements instead of XUL as they handle hidden attribute differently.
Differential Revision: https://phabricator.services.mozilla.com/D177790 - - - - - 1b031e60 by Edgar Chen at 2023-08-01T17:39:54+02:00 Bug 1821884 - Reshow initial fullscreen notification; r=Gijs
Depends on D177790
Differential Revision: https://phabricator.services.mozilla.com/D178339 - - - - - 33af6b34 by Eitan Isaacson at 2023-08-01T17:39:55+02:00 Bug 1819160 - Map Android ids to doc/accessible id pairs. r=Jamie
Differential Revision: https://phabricator.services.mozilla.com/D179737 - - - - - 65183e25 by Jon Coppeard at 2023-08-01T17:39:55+02:00 Bug 1828024 - Require the helper thread lock in the GC helper thread count getter r=sfink
This makes us take a lock to read this state (we already lock when writing it).
Also it adds a release assert in case something goes wrong with the thread count calculations, as a crash is preferable to the potential deadlock.
Differential Revision: https://phabricator.services.mozilla.com/D181257 - - - - -
12 changed files:
- accessible/android/SessionAccessibility.cpp - accessible/android/SessionAccessibility.h - accessible/ipc/DocAccessibleParent.cpp - accessible/ipc/DocAccessibleParent.h - accessible/ipc/moz.build - browser/base/content/browser-fullScreenAndPointerLock.js - browser/base/content/fullscreen-and-pointerlock.inc.xhtml - browser/base/content/test/fullscreen/browser_fullscreen_warning.js - dom/tests/browser/browser_pointerlock_warning.js - js/src/gc/GC.cpp - js/src/gc/ParallelMarking.cpp - js/src/vm/HelperThreadState.h
Changes:
===================================== accessible/android/SessionAccessibility.cpp ===================================== @@ -269,12 +269,9 @@ RefPtr<SessionAccessibility> SessionAccessibility::GetInstanceFor( return GetInstanceFor(doc->GetPresShell()); } } else { - DocAccessibleParent* remoteDoc = aAccessible->AsRemote()->Document(); - if (remoteDoc->mSessionAccessibility) { - return remoteDoc->mSessionAccessibility; - } dom::CanonicalBrowsingContext* cbc = - static_castdom::BrowserParent*(remoteDoc->Manager()) + static_castdom::BrowserParent*( + aAccessible->AsRemote()->Document()->Manager()) ->GetBrowsingContext() ->Top(); dom::BrowserParent* bp = cbc->GetBrowserParent(); @@ -285,10 +282,7 @@ RefPtr<SessionAccessibility> SessionAccessibility::GetInstanceFor( if (auto element = bp->GetOwnerElement()) { if (auto doc = element->OwnerDoc()) { if (nsPresContext* presContext = doc->GetPresContext()) { - RefPtr<SessionAccessibility> sessionAcc = - GetInstanceFor(presContext->PresShell()); - remoteDoc->mSessionAccessibility = sessionAcc; - return sessionAcc; + return GetInstanceFor(presContext->PresShell()); } } else { MOZ_ASSERT_UNREACHABLE( @@ -684,14 +678,7 @@ void SessionAccessibility::PopulateNodeInfo( }
Accessible* SessionAccessibility::GetAccessibleByID(int32_t aID) const { - Accessible* accessible = mIDToAccessibleMap.Get(aID); - if (accessible && accessible->IsLocal() && - accessible->AsLocal()->IsDefunct()) { - MOZ_ASSERT_UNREACHABLE("Registered accessible is defunct!"); - return nullptr; - } - - return accessible; + return mIDToAccessibleMap.Get(aID); }
#ifdef DEBUG @@ -705,6 +692,58 @@ static bool IsDetachedDoc(Accessible* aAccessible) { } #endif
+SessionAccessibility::IDMappingEntry::IDMappingEntry(Accessible* aAccessible) + : mInternalID(0) { + *this = aAccessible; +} + +SessionAccessibility::IDMappingEntry& +SessionAccessibility::IDMappingEntry::operator=(Accessible* aAccessible) { + mInternalID = aAccessible->ID(); + MOZ_ASSERT(!(mInternalID & IS_REMOTE), "First bit is used in accessible ID!"); + if (aAccessible->IsRemote()) { + mInternalID |= IS_REMOTE; + } + + Accessible* docAcc = nsAccUtils::DocumentFor(aAccessible); + MOZ_ASSERT(docAcc); + if (docAcc) { + MOZ_ASSERT(docAcc->IsRemote() == aAccessible->IsRemote()); + if (docAcc->IsRemote()) { + mDoc = docAcc->AsRemote()->AsDoc(); + } else { + mDoc = docAcc->AsLocal(); + } + } + + return *this; +} + +SessionAccessibility::IDMappingEntry::operator Accessible*() const { + if (mInternalID == 0) { + return static_cast<LocalAccessible*>(mDoc.get()); + } + + if (mInternalID == IS_REMOTE) { + return static_cast<DocAccessibleParent*>(mDoc.get()); + } + + if (mInternalID & IS_REMOTE) { + return static_cast<DocAccessibleParent*>(mDoc.get()) + ->GetAccessible(mInternalID & ~IS_REMOTE); + } + + Accessible* accessible = + static_cast<LocalAccessible*>(mDoc.get()) + ->AsDoc() + ->GetAccessibleByUniqueID(reinterpret_cast<void*>(mInternalID)); + // If the accessible is retrievable from the DocAccessible, it can't be + // defunct. + MOZ_ASSERT(!accessible->AsLocal()->IsDefunct()); + + return accessible; +} + void SessionAccessibility::RegisterAccessible(Accessible* aAccessible) { if (IPCAccessibilityActive()) { // Don't register accessible in content process. @@ -766,7 +805,6 @@ void SessionAccessibility::UnregisterAccessible(Accessible* aAccessible) { }
RefPtr<SessionAccessibility> sessionAcc = GetInstanceFor(aAccessible); - MOZ_ASSERT(sessionAcc, "Need SessionAccessibility to unregister Accessible!"); if (sessionAcc) { Accessible* registeredAcc = sessionAcc->mIDToAccessibleMap.Get(virtualViewID);
===================================== accessible/android/SessionAccessibility.h ===================================== @@ -110,10 +110,34 @@ class SessionAccessibility final jni::NativeWeakPtrwidget::GeckoViewSupport mWindow; // Parent only java::SessionAccessibility::NativeProvider::GlobalRef mSessionAccessibility;
+ class IDMappingEntry { + public: + explicit IDMappingEntry(Accessible* aAccessible); + + IDMappingEntry& operator=(Accessible* aAccessible); + + operator Accessible*() const; + + private: + // A strong reference to a DocAccessible or DocAccessibleParent. They don't + // share any useful base class except nsISupports, so we use that. + // When we retrieve the document from this reference we cast it to + // LocalAccessible in the DocAccessible case because DocAccessible has + // multiple inheritance paths for nsISupports. + RefPtr<nsISupports> mDoc; + // The ID of the accessible as used in the internal doc mapping. + // We rely on this ID being pointer derived and therefore divisible by two + // so we can use the first bit to mark if it is remote or not. + uint64_t mInternalID; + + static const uintptr_t IS_REMOTE = 0x1; + }; + /* * This provides a mapping from 32 bit id to accessible objects. */ - nsTHashMap<nsUint32HashKey, Accessible*> mIDToAccessibleMap; + nsBaseHashtable<nsUint32HashKey, IDMappingEntry, Accessible*> + mIDToAccessibleMap; };
} // namespace a11y
===================================== accessible/ipc/DocAccessibleParent.cpp ===================================== @@ -29,7 +29,6 @@ #endif
#if defined(ANDROID) -# include "mozilla/a11y/SessionAccessibility.h" # define ACQUIRE_ANDROID_LOCK \ MonitorAutoLock mal(nsAccessibilityService::GetAndroidMonitor()); #else
===================================== accessible/ipc/DocAccessibleParent.h ===================================== @@ -29,10 +29,6 @@ class xpcAccessibleGeneric; class DocAccessiblePlatformExtParent; #endif
-#ifdef ANDROID -class SessionAccessibility; -#endif - /* * These objects live in the main process and comunicate with and represent * an accessible document in a content process. @@ -348,10 +344,6 @@ class DocAccessibleParent : public RemoteAccessible,
size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) override;
-#ifdef ANDROID - RefPtr<SessionAccessibility> mSessionAccessibility; -#endif - private: ~DocAccessibleParent();
===================================== accessible/ipc/moz.build ===================================== @@ -24,11 +24,6 @@ else: LOCAL_INCLUDES += [ "/accessible/mac", ] - elif CONFIG["MOZ_WIDGET_TOOLKIT"] == "android": - LOCAL_INCLUDES += [ - "/accessible/android", - "/widget/android", - ] else: LOCAL_INCLUDES += [ "/accessible/other",
===================================== browser/base/content/browser-fullScreenAndPointerLock.js ===================================== @@ -62,9 +62,14 @@ var PointerlockFsWarning = { this._element = document.getElementById(elementId); // Setup event listeners this._element.addEventListener("transitionend", this); + this._element.addEventListener("transitioncancel", this); window.addEventListener("mousemove", this, true); + window.addEventListener("activate", this); + window.addEventListener("deactivate", this); // The timeout to hide the warning box after a while. this._timeoutHide = new this.Timeout(() => { + window.removeEventListener("activate", this); + window.removeEventListener("deactivate", this); this._state = "hidden"; }, timeout); // The timeout to show the warning box when the pointer is at the top @@ -116,11 +121,10 @@ var PointerlockFsWarning = { return; }
- // Explicitly set the last state to hidden to avoid the warning - // box being hidden immediately because of mousemove. - this._state = "onscreen"; - this._lastState = "hidden"; - this._timeoutHide.start(); + if (Services.focus.activeWindow == window) { + this._state = "onscreen"; + this._timeoutHide.start(); + } },
/** @@ -148,7 +152,10 @@ var PointerlockFsWarning = { this._element.hidden = true; // Remove all event listeners this._element.removeEventListener("transitionend", this); + this._element.removeEventListener("transitioncancel", this); window.removeEventListener("mousemove", this, true); + window.removeEventListener("activate", this); + window.removeEventListener("deactivate", this); // Clear fields this._element = null; this._timeoutHide = null; @@ -186,7 +193,7 @@ var PointerlockFsWarning = { } if (newState != "hidden") { if (currentState != "hidden") { - this._element.setAttribute(newState, true); + this._element.setAttribute(newState, ""); } else { // When the previous state is hidden, the display was none, // thus no box was constructed. We need to wait for the new @@ -197,7 +204,7 @@ var PointerlockFsWarning = { requestAnimationFrame(() => { requestAnimationFrame(() => { if (this._element) { - this._element.setAttribute(newState, true); + this._element.setAttribute(newState, ""); } }); }); @@ -217,7 +224,7 @@ var PointerlockFsWarning = { } else if (this._timeoutShow.delay >= 0) { this._timeoutShow.start(); } - } else { + } else if (state != "onscreen") { let elemRect = this._element.getBoundingClientRect(); if (state == "hiding" && this._lastState != "hidden") { // If we are on the hiding transition, and the pointer @@ -239,12 +246,23 @@ var PointerlockFsWarning = { } break; } - case "transitionend": { + case "transitionend": + case "transitioncancel": { if (this._state == "hiding") { this._element.hidden = true; } break; } + case "activate": { + this._state = "onscreen"; + this._timeoutHide.start(); + break; + } + case "deactivate": { + this._state = "hidden"; + this._timeoutHide.cancel(); + break; + } } }, };
===================================== browser/base/content/fullscreen-and-pointerlock.inc.xhtml ===================================== @@ -3,7 +3,7 @@ # file, You can obtain one at http://mozilla.org/MPL/2.0/.
<html:div id="fullscreen-and-pointerlock-wrapper"> - <html:div id="fullscreen-warning" class="pointerlockfswarning" hidden="true"> + <html:div id="fullscreen-warning" class="pointerlockfswarning" hidden=""> <html:div class="pointerlockfswarning-domain-text"> <html:span class="pointerlockfswarning-domain" data-l10n-name="domain"/> </html:div> @@ -20,7 +20,7 @@ </html:button> </html:div>
- <html:div id="pointerlock-warning" class="pointerlockfswarning" hidden="true"> + <html:div id="pointerlock-warning" class="pointerlockfswarning" hidden=""> <html:div class="pointerlockfswarning-domain-text"> <html:span class="pointerlockfswarning-domain" data-l10n-name="domain"/> </html:div>
===================================== browser/base/content/test/fullscreen/browser_fullscreen_warning.js ===================================== @@ -3,14 +3,35 @@
"use strict";
-add_task(async function test_fullscreen_display_none() { +function checkWarningState(aWarningElement, aExpectedState, aMsg) { + ["hidden", "ontop", "onscreen"].forEach(state => { + is( + aWarningElement.hasAttribute(state), + state == aExpectedState, + `${aMsg} - check ${state} attribute.` + ); + }); +} + +async function waitForWarningState(aWarningElement, aExpectedState) { + await BrowserTestUtils.waitForAttribute(aExpectedState, aWarningElement, ""); + checkWarningState( + aWarningElement, + aExpectedState, + `Wait for ${aExpectedState} state` + ); +} + +add_setup(async function init() { await SpecialPowers.pushPrefEnv({ set: [ ["full-screen-api.enabled", true], ["full-screen-api.allow-trusted-requests-only", false], ], }); +});
+add_task(async function test_fullscreen_display_none() { await BrowserTestUtils.withNewTab( { gBrowser, @@ -30,11 +51,13 @@ add_task(async function test_fullscreen_display_none() { }, async function (browser) { let warning = document.getElementById("fullscreen-warning"); - let warningShownPromise = BrowserTestUtils.waitForAttribute( - "onscreen", + checkWarningState( warning, - "true" + "hidden", + "Should not show full screen warning initially" ); + + let warningShownPromise = waitForWarningState(warning, "onscreen"); // Enter fullscreen await SpecialPowers.spawn(browser, [], async () => { let frame = content.document.querySelector("iframe"); @@ -54,39 +77,33 @@ add_task(async function test_fullscreen_display_none() { ); document.getElementById("fullscreen-exit-button").click(); await exitFullscreenPromise; + + checkWarningState( + warning, + "hidden", + "Should hide fullscreen warning after exiting fullscreen" + ); } ); });
add_task(async function test_fullscreen_pointerlock_conflict() { - await SpecialPowers.pushPrefEnv({ - set: [ - ["full-screen-api.enabled", true], - ["full-screen-api.allow-trusted-requests-only", false], - ], - }); - await BrowserTestUtils.withNewTab("https://example.com", async browser => { let fsWarning = document.getElementById("fullscreen-warning"); let plWarning = document.getElementById("pointerlock-warning");
- is( - fsWarning.getAttribute("onscreen"), - null, - "Should not show full screen warning initially." - ); - is( - plWarning.getAttribute("onscreen"), - null, - "Should not show pointer lock warning initially." - ); - - let fsWarningShownPromise = BrowserTestUtils.waitForAttribute( - "onscreen", + checkWarningState( fsWarning, - "true" + "hidden", + "Should not show full screen warning initially" + ); + checkWarningState( + plWarning, + "hidden", + "Should not show pointer lock warning initially" );
+ let fsWarningShownPromise = waitForWarningState(fsWarning, "onscreen"); info("Entering full screen and pointer lock."); await SpecialPowers.spawn(browser, [], async () => { await content.document.body.requestFullscreen(); @@ -94,15 +111,10 @@ add_task(async function test_fullscreen_pointerlock_conflict() { });
await fsWarningShownPromise; - is( - fsWarning.getAttribute("onscreen"), - "true", - "Should show full screen warning." - ); - is( - plWarning.getAttribute("onscreen"), - null, - "Should not show pointer lock warning." + checkWarningState( + plWarning, + "hidden", + "Should not show pointer lock warning" );
info("Exiting pointerlock"); @@ -110,18 +122,19 @@ add_task(async function test_fullscreen_pointerlock_conflict() { await content.document.exitPointerLock(); });
- is( - fsWarning.getAttribute("onscreen"), - "true", - "Should still show full screen warning." + checkWarningState( + fsWarning, + "onscreen", + "Should still show full screen warning" ); - is( - plWarning.getAttribute("onscreen"), - null, - "Should not show pointer lock warning." + checkWarningState( + plWarning, + "hidden", + "Should not show pointer lock warning" );
// Cleanup + info("Exiting fullscreen"); await document.exitFullscreen(); }); });
===================================== dom/tests/browser/browser_pointerlock_warning.js ===================================== @@ -15,6 +15,25 @@ const FRAME_TEST_URL = encodeURI(BODY_URL) + '"></iframe></body>';
+function checkWarningState(aWarningElement, aExpectedState, aMsg) { + ["hidden", "ontop", "onscreen"].forEach(state => { + is( + aWarningElement.hasAttribute(state), + state == aExpectedState, + `${aMsg} - check ${state} attribute.` + ); + }); +} + +async function waitForWarningState(aWarningElement, aExpectedState) { + await BrowserTestUtils.waitForAttribute(aExpectedState, aWarningElement, ""); + checkWarningState( + aWarningElement, + aExpectedState, + `Wait for ${aExpectedState} state` + ); +} + // Make sure the pointerlock warning is shown and exited with the escape key add_task(async function show_pointerlock_warning_escape() { let urls = [TEST_URL, FRAME_TEST_URL]; @@ -24,11 +43,7 @@ add_task(async function show_pointerlock_warning_escape() { let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, url);
let warning = document.getElementById("pointerlock-warning"); - let warningShownPromise = BrowserTestUtils.waitForAttribute( - "onscreen", - warning, - "true" - ); + let warningShownPromise = waitForWarningState(warning, "onscreen");
let expectedWarningText;
@@ -49,11 +64,7 @@ add_task(async function show_pointerlock_warning_escape() {
ok(true, "Pointerlock warning shown");
- let warningHiddenPromise = BrowserTestUtils.waitForAttribute( - "hidden", - warning, - "" - ); + let warningHiddenPromise = waitForWarningState(warning, "hidden");
await BrowserTestUtils.waitForCondition( () => warning.innerText == expectedWarningText,
===================================== js/src/gc/GC.cpp ===================================== @@ -1331,6 +1331,11 @@ void GCRuntime::assertNoMarkingWork() const { } #endif
+static size_t GetGCParallelThreadCount() { + AutoLockHelperThreadState lock; + return HelperThreadState().getGCParallelThreadCount(lock); +} + bool GCRuntime::updateMarkersVector() { MOZ_ASSERT(helperThreadCount >= 1, "There must always be at least one mark task"); @@ -1339,8 +1344,8 @@ bool GCRuntime::updateMarkersVector() {
// Limit worker count to number of GC parallel tasks that can run // concurrently, otherwise one thread can deadlock waiting on another. - size_t targetCount = std::min(markingWorkerCount(), - HelperThreadState().getGCParallelThreadCount()); + size_t targetCount = + std::min(markingWorkerCount(), GetGCParallelThreadCount());
if (markers.length() > targetCount) { return markers.resize(targetCount);
===================================== js/src/gc/ParallelMarking.cpp ===================================== @@ -103,6 +103,10 @@ bool ParallelMarker::markOneColor(MarkColor color, SliceBudget& sliceBudget) { { AutoLockHelperThreadState lock;
+ // There should always be enough parallel tasks to run our marking work. + MOZ_RELEASE_ASSERT(HelperThreadState().getGCParallelThreadCount(lock) >= + workerCount()); + for (size_t i = 0; i < workerCount(); i++) { gc->startTask(*tasks[i], lock); }
===================================== js/src/vm/HelperThreadState.h ===================================== @@ -333,9 +333,11 @@ class GlobalHelperThreadState {
GCParallelTaskList& gcParallelWorklist() { return gcParallelWorklist_; }
- size_t getGCParallelThreadCount() const { return gcParallelThreadCount; } + size_t getGCParallelThreadCount(const AutoLockHelperThreadState& lock) const { + return gcParallelThreadCount; + } void setGCParallelThreadCount(size_t count, - const AutoLockHelperThreadState&) { + const AutoLockHelperThreadState& lock) { MOZ_ASSERT(count >= 1); MOZ_ASSERT(count <= threadCount); gcParallelThreadCount = count;
View it on GitLab: https://gitlab.torproject.org/tpo/applications/tor-browser/-/compare/c847c74...