[tor-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 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;