[tbb-commits] [Git][tpo/applications/tor-browser][tor-browser-115.1.0esr-13.0-1] 4 commits: Bug 1821884 - Ensure consistent state for fullscreen/pointerlock warnings; r=Gijs

ma1 (@ma1) git at gitlab.torproject.org
Tue Aug 1 09:47:51 UTC 2023



ma1 pushed to branch tor-browser-115.1.0esr-13.0-1 at The Tor Project / Applications / Tor Browser


Commits:
a9279174 by Edgar Chen at 2023-07-31T23:49:06+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
- - - - -
9f2eaedb by Edgar Chen at 2023-07-31T23:49:06+02:00
Bug 1821884 - Reshow initial fullscreen notification; r=Gijs

Depends on D177790

Differential Revision: https://phabricator.services.mozilla.com/D178339
- - - - -
30d19aa0 by Eitan Isaacson at 2023-07-31T23:49:07+02:00
Bug 1819160 - Map Android ids to doc/accessible id pairs. r=Jamie

Differential Revision: https://phabricator.services.mozilla.com/D179737
- - - - -
efee8978 by Jon Coppeard at 2023-07-31T23:49:07+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_cast<dom::BrowserParent*>(remoteDoc->Manager())
+        static_cast<dom::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::NativeWeakPtr<widget::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/c4e8cd0f05884220aaf13fad10b1ac16d561a2a6...efee89783a7d79442a260275e93f39fce228f8c8

-- 
View it on GitLab: https://gitlab.torproject.org/tpo/applications/tor-browser/-/compare/c4e8cd0f05884220aaf13fad10b1ac16d561a2a6...efee89783a7d79442a260275e93f39fce228f8c8
You're receiving this email because of your account on gitlab.torproject.org.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.torproject.org/pipermail/tbb-commits/attachments/20230801/f43062b0/attachment-0001.htm>


More information about the tbb-commits mailing list