Backed out 2 changesets (bug 1860328) for causing crashes on MarkNeedsDisplayItemRebuild. CLOSED TREE

Backed out changeset b32811ea1c61 (bug 1860328) Backed out changeset caab023e5d2d (bug 1860328)
2024-03-11 17:18:37 +02:00 · 2024-03-11 17:18:37 +02:00 · 9c8ce9aee8
--- a/layout/base/nsCaret.cpp
+++ b/layout/base/nsCaret.cpp
@ -398,10 +398,6 @@ nsIFrame* nsCaret::GetGeometry(const Selection* aSelection, nsRect* aRect) {
 }

 void nsCaret::SchedulePaint(Selection* aSelection) {
-  if (mLastCaretFrame) {
-    mLastCaretFrame->MarkNeedsDisplayItemRebuild();
-  }
-
  Selection* selection;
  if (aSelection) {
    selection = aSelection;
@ -413,16 +409,14 @@ void nsCaret::SchedulePaint(Selection* aSelection) {
  nsIFrame* frame = GetFrameAndOffset(selection, mOverrideContent,
                                      mOverrideOffset, &frameOffset);
  if (!frame) {
-    SetLastCaretFrame(nullptr);
    return;
  }

  if (nsIFrame* cb = GetContainingBlockIfNeeded(frame)) {
-    frame = cb;
+    cb->SchedulePaint();
+  } else {
+    frame->SchedulePaint();
  }
-
-  SetLastCaretFrame(frame);
-  frame->SchedulePaint();
 }

 void nsCaret::SetVisibilityDuringSelection(bool aVisibility) {
@ -561,9 +555,6 @@ void nsCaret::PaintCaret(DrawTarget& aDrawTarget, nsIFrame* aForFrame,
 NS_IMETHODIMP
 nsCaret::NotifySelectionChanged(Document*, Selection* aDomSel, int16_t aReason,
                                int32_t aAmount) {
-  if (mLastCaretFrame) {
-    mLastCaretFrame->MarkNeedsDisplayItemRebuild();
-  }
  // Note that aDomSel, per the comment below may not be the same as our
  // selection, but that's OK since if that is the case, it wouldn't have
  // mattered what IsVisible() returns here, so we just opt for checking
--- a/layout/base/nsCaret.h
+++ b/layout/base/nsCaret.h
@ -12,7 +12,6 @@
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/dom/Selection.h"
 #include "nsCoord.h"
-#include "nsIFrame.h"
 #include "nsISelectionListener.h"
 #include "nsIWeakReferenceUtils.h"
 #include "nsPoint.h"
@ -197,13 +196,6 @@ class nsCaret final : public nsISelectionListener {

  size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;

-  void SetLastCaretFrame(nsIFrame* aLastCaretFrame) {
-    if (mLastCaretFrame && mLastCaretFrame != aLastCaretFrame) {
-      mLastCaretFrame->MarkNeedsDisplayItemRebuild();
-    }
-    mLastCaretFrame = aLastCaretFrame;
-  }
-
 protected:
  static void CaretBlinkCallback(nsITimer* aTimer, void* aClosure);

@ -287,11 +279,6 @@ class nsCaret final : public nsISelectionListener {
   * it's in non-user-modifiable content.
   */
  bool mIgnoreUserModify;
-
-  /**
-   * mLastCaretFrame is the last caret frame that was scheduled to paint.
-   */
-  WeakFrame mLastCaretFrame;
 };

 #endif  // nsCaret_h__
--- a/layout/generic/crashtests/1878805.html
+++ b/layout/generic/crashtests/1878805.html
@ -1,30 +0,0 @@
-<html class="reftest-wait">
-<style>
-* { translate: 0px }
-</style>
-<script>
-addEventListener("MozReftestInvalidate", () => {
-  o.setAttribute("volume", "-68")
-  document.designMode = "on"
-  document.execCommand("selectAll", false)
-  document.execCommand("insertText", false, "AAA")
-  document.documentElement.classList.remove("reftest-wait");
-})
-function go() {
-  window.find("A")
-  let a = new MutationObserver(() => {
-    document.designMode = "off"
-    window.requestAnimationFrame(() => {
-      window.parent.document.designMode = "on"
-      document.execCommand("indent", false)
-      document.designMode = "off"
-    })
-    document.onselectionchange = go
-  })
-  a.observe(o, {attributeOldValue: false})
-}
-</script>
-<body>
-<input autofocus="" onfocusin="go()">
-<bgsound id="o">
-</html>
--- a/layout/generic/crashtests/crashtests.list
+++ b/layout/generic/crashtests/crashtests.list
@ -812,4 +812,3 @@ load 1816574.html
 load 1821603.html
 load 1822118.html
 load 1825434.html
-load 1878805.html
--- a/layout/painting/RetainedDisplayListBuilder.cpp
+++ b/layout/painting/RetainedDisplayListBuilder.cpp
@ -1313,6 +1313,23 @@ bool RetainedDisplayListBuilder::ShouldBuildPartial(
  return true;
 }

+void RetainedDisplayListBuilder::InvalidateCaretFramesIfNeeded() {
+  if (mPreviousCaret == mBuilder.GetCaretFrame()) {
+    // The current caret frame is the same as the previous one.
+    return;
+  }
+
+  if (mPreviousCaret) {
+    mPreviousCaret->MarkNeedsDisplayItemRebuild();
+  }
+
+  if (mBuilder.GetCaretFrame()) {
+    mBuilder.GetCaretFrame()->MarkNeedsDisplayItemRebuild();
+  }
+
+  mPreviousCaret = mBuilder.GetCaretFrame();
+}
+
 class AutoClearFramePropsArray {
 public:
  explicit AutoClearFramePropsArray(size_t aCapacity) : mFrames(aCapacity) {}
@ -1568,6 +1585,8 @@ PartialUpdateResult RetainedDisplayListBuilder::AttemptPartialUpdate(
    MarkFramesWithItemsAndImagesModified(&mList);
  }

+  InvalidateCaretFramesIfNeeded();
+
  // We set the override dirty regions during ComputeRebuildRegion or in
  // DisplayPortUtils::InvalidateForDisplayPortChange. The display port change
  // also marks the frame modified, so those regions are cleared here as well.
--- a/layout/painting/RetainedDisplayListBuilder.h
+++ b/layout/painting/RetainedDisplayListBuilder.h
@ -272,6 +272,7 @@ class RetainedDisplayListBuilder {

  nsDisplayListBuilder mBuilder;
  RetainedDisplayList mList;
+  WeakFrame mPreviousCaret;
  RetainedDisplayListMetrics mMetrics;
  RetainedDisplayListData mData;
 };
--- a/layout/painting/nsDisplayList.cpp
+++ b/layout/painting/nsDisplayList.cpp
@ -650,6 +650,7 @@ nsDisplayListBuilder::nsDisplayListBuilder(nsIFrame* aReferenceFrame,
      mCurrentContainerASR(nullptr),
      mCurrentFrame(aReferenceFrame),
      mCurrentReferenceFrame(aReferenceFrame),
+      mCaretFrame(nullptr),
      mScrollInfoItemsForHoisting(nullptr),
      mFirstClipChainToDestroy(nullptr),
      mTableBackgroundSet(nullptr),
@ -717,6 +718,21 @@ nsDisplayListBuilder::nsDisplayListBuilder(nsIFrame* aReferenceFrame,
      mRetainingDisplayList && StaticPrefs::layout_display_list_retain_sc();
 }

+static PresShell* GetFocusedPresShell() {
+  nsPIDOMWindowOuter* focusedWnd =
+      nsFocusManager::GetFocusManager()->GetFocusedWindow();
+  if (!focusedWnd) {
+    return nullptr;
+  }
+
+  nsCOMPtr<nsIDocShell> focusedDocShell = focusedWnd->GetDocShell();
+  if (!focusedDocShell) {
+    return nullptr;
+  }
+
+  return focusedDocShell->GetPresShell();
+}
+
 void nsDisplayListBuilder::BeginFrame() {
  nsCSSRendering::BeginFrameTreesLocked();

@ -726,6 +742,26 @@ void nsDisplayListBuilder::BeginFrame() {
  mInTransform = false;
  mInFilter = false;
  mSyncDecodeImages = false;
+
+  if (!mBuildCaret) {
+    return;
+  }
+
+  RefPtr<PresShell> presShell = GetFocusedPresShell();
+  if (presShell) {
+    RefPtr<nsCaret> caret = presShell->GetCaret();
+    mCaretFrame = caret->GetPaintGeometry(&mCaretRect);
+
+    // The focused pres shell may not be in the document that we're
+    // painting, or be in a popup. Check if the display root for
+    // the caret matches the display root that we're painting, and
+    // only use it if it matches.
+    if (mCaretFrame &&
+        nsLayoutUtils::GetDisplayRootFrame(mCaretFrame) !=
+            nsLayoutUtils::GetDisplayRootFrame(mReferenceFrame)) {
+      mCaretFrame = nullptr;
+    }
+  }
 }

 void nsDisplayListBuilder::AddEffectUpdate(dom::RemoteBrowser* aBrowser,
@ -765,6 +801,7 @@ void nsDisplayListBuilder::EndFrame() {
  FreeClipChains();
  FreeTemporaryItems();
  nsCSSRendering::EndFrameTreesLocked();
+  mCaretFrame = nullptr;
 }

 void nsDisplayListBuilder::MarkFrameForDisplay(nsIFrame* aFrame,
@ -1104,35 +1141,11 @@ void nsDisplayListBuilder::EnterPresShell(const nsIFrame* aReferenceFrame,
    return;
  }

-  RefPtr<nsCaret> caret = state->mPresShell->GetCaret();
-  // This code run for each pres shell and caret->GetPaintGeometry
-  // will return nullptr for invisible caret. So only one caret
-  // can be painted at a time.
-  state->mCaretFrame = caret->GetPaintGeometry(&mCaretRect);
-
-  // Check if the display root for the caret matches the display
-  // root that we're painting, and only use it if it matches. Likely
-  // we only need this for popup.
-  if (state->mCaretFrame &&
-      nsLayoutUtils::GetDisplayRootFrame(state->mCaretFrame) !=
-          nsLayoutUtils::GetDisplayRootFrame(aReferenceFrame)) {
-    state->mCaretFrame = nullptr;
-  }
-
  // Caret frames add visual area to their frame, but we don't update the
  // overflow area. Use flags to make sure we build display items for that frame
  // instead.
-  if (state->mCaretFrame) {
-    MOZ_ASSERT(state->mCaretFrame->PresShell() == state->mPresShell);
-    // Generally, nsCaret sets the last caret frame in
-    // nsCaret::SchedulePaint to call MarkNeedsDisplayItemRebuild()
-    // on the frame accordingly, so we shouldn't need do to this manually.
-    // However, it's possible for nsCaret::SchedulePaint fails to find
-    // the caret frame (i.e, selection changes), we end up not calling
-    // MarkNeedsDisplayItemRebuild() on this frame. This is not good,
-    // so we are manually setting the last caret frame here.
-    caret->SetLastCaretFrame(state->mCaretFrame);
-    MarkFrameForDisplay(state->mCaretFrame, aReferenceFrame);
+  if (mCaretFrame && mCaretFrame->PresShell() == state->mPresShell) {
+    MarkFrameForDisplay(mCaretFrame, aReferenceFrame);
  }
 }

@ -4121,8 +4134,8 @@ bool nsDisplayCaret::CreateWebRenderCommands(
  nscolor caretColor;
  nsIFrame* frame =
      mCaret->GetPaintGeometry(&caretRect, &hookRect, &caretColor);
-  if (NS_WARN_IF(!frame) || NS_WARN_IF(frame != mFrame)) {
-    NS_ASSERTION(false, "Caret invalidation bug");
+  MOZ_ASSERT(frame == mFrame, "We're referring different frame");
+  if (!frame) {
    return true;
  }

--- a/layout/painting/nsDisplayList.h
+++ b/layout/painting/nsDisplayList.h
@ -659,7 +659,7 @@ class nsDisplayListBuilder {
   * Get the frame that the caret is supposed to draw in.
   * If the caret is currently invisible, this will be null.
   */
-  nsIFrame* GetCaretFrame() { return CurrentPresShellState()->mCaretFrame; }
+  nsIFrame* GetCaretFrame() { return mCaretFrame; }
  /**
   * Get the rectangle we're supposed to draw the caret into.
   */
@ -1729,7 +1729,6 @@ class nsDisplayListBuilder {
    bool mInsidePointerEventsNoneDoc;
    bool mTouchEventPrefEnabledDoc;
    nsIFrame* mPresShellIgnoreScrollFrame;
-    nsIFrame* mCaretFrame = nullptr;
  };

  PresShellState* CurrentPresShellState() {
@ -1764,6 +1763,7 @@ class nsDisplayListBuilder {
  // The reference frame for mCurrentFrame.
  const nsIFrame* mCurrentReferenceFrame;

+  nsIFrame* mCaretFrame;
  // A temporary list that we append scroll info items to while building
  // display items for the contents of frames with SVG effects.
  // Only non-null when ShouldBuildScrollInfoItemsForHoisting() is true.