Bug 1543315 - part 20: Mark `PresShell::DidCauseReflow()` as `MOZ_CAN_RUN_SCRIPT` r=smaug draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 04 Dec 2019 16:53:16 +0900
changeset 2514406 817c682bf18ddae6b1978d8867edc5515080e83d
parent 2514405 b9e3b7d03bfb48bfe1fe97c43c09f0bc552f4c25
child 2514407 ee01a7f649cb45564fb2dc12cc57a0d47bd35934
push id459907
push usermasayuki@d-toybox.com
push dateWed, 04 Dec 2019 12:49:29 +0000
treeherdertry@ee01a7f649cb [default view] [failures only]
reviewerssmaug
bugs1543315
milestone73.0a1
Bug 1543315 - part 20: Mark `PresShell::DidCauseReflow()` as `MOZ_CAN_RUN_SCRIPT` r=smaug It removes a script blocker. Therefore, although it depends on the caller whether it causes running script or not. However, we should mark it as `MOZ_CAN_RUN_SCRIPT` for safer code. It's called only by the destructor of `nsAutoCauseReflowNotifier`. Therefore, this patch also marks its constructor as `MOZ_CAN_RUN_SCRIPT` for making each creator method marked as `MOZ_CAN_RUN_SCRIPT` or `MOZ_CAN_RUN_SCRIPT_BOUNDARY`. Most of the creators is mutation listener methods. However, `PresShell` does nothing after destroying `nsAutoCauseReflowNotifier`. Therefore, this patch does not change the callers in MutationObserver.cpp to use `RefPtr<PresShell>` at calling them because changing it may cause performance regression. Perhaps, we should create another methods of `WillCauseReflow()` and `DidCauseReflow()` to avoid unnecessary `MOZ_CAN_RUN_SCRIPT` marking. However, I'm not sure whether most callers may run script or not because of outside of my knowledge.
editor/libeditor/TextEditor.cpp
layout/base/PresShell.cpp
layout/base/PresShell.h
--- a/editor/libeditor/TextEditor.cpp
+++ b/editor/libeditor/TextEditor.cpp
@@ -2300,17 +2300,17 @@ nsresult TextEditor::SetUnmaskRangeInter
   if (aNotify) {
     MOZ_ASSERT(IsEditActionDataAvailable());
 
     RefPtr<Document> document = GetDocument();
     if (NS_WARN_IF(!document)) {
       return NS_ERROR_NOT_INITIALIZED;
     }
     // Notify nsTextFrame of masking range change.
-    if (PresShell* presShell = document->GetObservingPresShell()) {
+    if (RefPtr<PresShell> presShell = document->GetObservingPresShell()) {
       uint32_t valueLength = text->Length();
       CharacterDataChangeInfo changeInfo = {false, 0, valueLength, valueLength,
                                             nullptr};
       presShell->CharacterDataChanged(text, changeInfo);
     }
 
     // Scroll caret into the view since masking or unmasking character may
     // move caret to outside of the view.
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -447,25 +447,26 @@ struct nsCallbackEventRequest {
       }                                                                     \
     }
 #else
 #  define ASSERT_REFLOW_SCHEDULED_STATE() /* nothing */
 #endif
 
 class nsAutoCauseReflowNotifier {
  public:
-  explicit nsAutoCauseReflowNotifier(PresShell* aPresShell)
+  MOZ_CAN_RUN_SCRIPT explicit nsAutoCauseReflowNotifier(PresShell* aPresShell)
       : mPresShell(aPresShell) {
     mPresShell->WillCauseReflow();
   }
-  ~nsAutoCauseReflowNotifier() {
+  MOZ_CAN_RUN_SCRIPT ~nsAutoCauseReflowNotifier() {
     // This check should not be needed. Currently the only place that seem
     // to need it is the code that deals with bug 337586.
     if (!mPresShell->mHaveShutDown) {
-      mPresShell->DidCauseReflow();
+      RefPtr<PresShell> presShell(mPresShell);
+      presShell->DidCauseReflow();
     } else {
       nsContentUtils::RemoveScriptBlocker();
     }
   }
 
   PresShell* mPresShell;
 };
 
@@ -1730,27 +1731,21 @@ nsresult PresShell::Initialize() {
 
   if (Element* root = mDocument->GetRootElement()) {
     {
       nsAutoCauseReflowNotifier reflowNotifier(this);
       // Have the style sheet processor construct frame for the root
       // content object down
       mFrameConstructor->ContentInserted(
           root, nullptr, nsCSSFrameConstructor::InsertionKind::Sync);
-
-      // Something in mFrameConstructor->ContentInserted may have caused
-      // Destroy() to get called, bug 337586.
-      NS_ENSURE_STATE(!mHaveShutDown);
-    }
-
-    // nsAutoCauseReflowNotifier (which sets up a script blocker) going out of
-    // scope may have killed us too
-    NS_ENSURE_STATE(!mHaveShutDown);
-
-    // XBLConstructorRunner might destroy us.
+    }
+    // Something in mFrameConstructor->ContentInserted may have caused
+    // Destroy() to get called, bug 337586.  Or, nsAutoCauseReflowNotifier
+    // (which sets up a script blocker) going out of scope may have killed us
+    // too
     NS_ENSURE_STATE(!mHaveShutDown);
   }
 
   mDocument->TriggerAutoFocus();
 
   NS_ASSERTION(rootFrame, "How did that happen?");
 
   // Note: when the frame was created above it had the NS_FRAME_IS_DIRTY bit
@@ -1959,16 +1954,19 @@ nsresult PresShell::ResizeReflowIgnoreOv
                         : mPresContext->GetVisibleArea().height > aHeight;
 
     if (reflowAgain) {
       mPresContext->SetVisibleArea(nsRect(0, 0, aWidth, aHeight));
       DoReflow(rootFrame, true, nullptr);
     }
   }
 
+  // Now, we may have been destroyed by the destructor of
+  // `nsAutoCauseReflowNotifier`.
+
   DidDoReflow(true);
 
   // the reflow above should've set our bsize if it was NS_UNCONSTRAINEDSIZE,
   // and the isize shouldn't be NS_UNCONSTRAINEDSIZE anyway.
   MOZ_DIAGNOSTIC_ASSERT(
       mPresContext->GetVisibleArea().width != NS_UNCONSTRAINEDSIZE,
       "width should not be NS_UNCONSTRAINEDSIZE after reflow");
   MOZ_DIAGNOSTIC_ASSERT(
@@ -4161,25 +4159,28 @@ void PresShell::DoFlushPendingNotificati
     MOZ_ASSERT(didLayoutFlush == didStyleFlush);
     mLayoutTelemetry.PingReqsPerFlushTelemetry(FlushType::Layout);
   } else if (flushType >= FlushType::Style && didStyleFlush) {
     MOZ_ASSERT(!didLayoutFlush);
     mLayoutTelemetry.PingReqsPerFlushTelemetry(FlushType::Style);
   }
 }
 
-void PresShell::CharacterDataChanged(nsIContent* aContent,
-                                     const CharacterDataChangeInfo& aInfo) {
+MOZ_CAN_RUN_SCRIPT_BOUNDARY void PresShell::CharacterDataChanged(
+    nsIContent* aContent, const CharacterDataChangeInfo& aInfo) {
   MOZ_ASSERT(!mIsDocumentGone, "Unexpected CharacterDataChanged");
   MOZ_ASSERT(aContent->OwnerDoc() == mDocument, "Unexpected document");
 
   nsAutoCauseReflowNotifier crNotifier(this);
 
   mPresContext->RestyleManager()->CharacterDataChanged(aContent, aInfo);
   mFrameConstructor->CharacterDataChanged(aContent, aInfo);
+
+  // Be aware, don't do anything after destroying crNotifier because the caller
+  // may not guarantee the lifetime of us due to performance reason.
 }
 
 MOZ_CAN_RUN_SCRIPT_BOUNDARY void PresShell::ContentStateChanged(
     Document* aDocument, nsIContent* aContent, EventStates aStateMask) {
   MOZ_ASSERT(!mIsDocumentGone, "Unexpected ContentStateChanged");
   MOZ_ASSERT(aDocument == mDocument, "Unexpected aDocument");
 
   if (mDidInitialize) {
@@ -4203,48 +4204,54 @@ void PresShell::DocumentStatesChanged(Ev
 
   if (aStateMask.HasState(NS_DOCUMENT_STATE_WINDOW_INACTIVE)) {
     if (nsIFrame* root = mFrameConstructor->GetRootFrame()) {
       root->SchedulePaint();
     }
   }
 }
 
-void PresShell::AttributeWillChange(Element* aElement, int32_t aNameSpaceID,
-                                    nsAtom* aAttribute, int32_t aModType) {
+MOZ_CAN_RUN_SCRIPT_BOUNDARY void PresShell::AttributeWillChange(
+    Element* aElement, int32_t aNameSpaceID, nsAtom* aAttribute,
+    int32_t aModType) {
   MOZ_ASSERT(!mIsDocumentGone, "Unexpected AttributeWillChange");
   MOZ_ASSERT(aElement->OwnerDoc() == mDocument, "Unexpected document");
 
   // XXXwaterson it might be more elegant to wait until after the
   // initial reflow to begin observing the document. That would
   // squelch any other inappropriate notifications as well.
   if (mDidInitialize) {
     nsAutoCauseReflowNotifier crNotifier(this);
     mPresContext->RestyleManager()->AttributeWillChange(aElement, aNameSpaceID,
                                                         aAttribute, aModType);
   }
-}
-
-void PresShell::AttributeChanged(Element* aElement, int32_t aNameSpaceID,
-                                 nsAtom* aAttribute, int32_t aModType,
-                                 const nsAttrValue* aOldValue) {
+  // Be aware, don't do anything after destroying crNotifier because the caller
+  // may not guarantee the lifetime of us due to performance reason.
+}
+
+MOZ_CAN_RUN_SCRIPT_BOUNDARY void PresShell::AttributeChanged(
+    Element* aElement, int32_t aNameSpaceID, nsAtom* aAttribute,
+    int32_t aModType, const nsAttrValue* aOldValue) {
   MOZ_ASSERT(!mIsDocumentGone, "Unexpected AttributeChanged");
   MOZ_ASSERT(aElement->OwnerDoc() == mDocument, "Unexpected document");
 
   // XXXwaterson it might be more elegant to wait until after the
   // initial reflow to begin observing the document. That would
   // squelch any other inappropriate notifications as well.
   if (mDidInitialize) {
     nsAutoCauseReflowNotifier crNotifier(this);
     mPresContext->RestyleManager()->AttributeChanged(
         aElement, aNameSpaceID, aAttribute, aModType, aOldValue);
   }
-}
-
-void PresShell::ContentAppended(nsIContent* aFirstNewContent) {
+  // Be aware, don't do anything after destroying crNotifier because the caller
+  // may not guarantee the lifetime of us due to performance reason.
+}
+
+MOZ_CAN_RUN_SCRIPT_BOUNDARY void PresShell::ContentAppended(
+    nsIContent* aFirstNewContent) {
   MOZ_ASSERT(!mIsDocumentGone, "Unexpected ContentAppended");
   MOZ_ASSERT(aFirstNewContent->OwnerDoc() == mDocument, "Unexpected document");
 
   // We never call ContentAppended with a document as the container, so we can
   // assert that we have an nsIContent parent.
   MOZ_ASSERT(aFirstNewContent->GetParent());
   MOZ_ASSERT(aFirstNewContent->GetParent()->IsElement() ||
              aFirstNewContent->GetParent()->IsShadowRoot());
@@ -4257,39 +4264,46 @@ void PresShell::ContentAppended(nsIConte
 
   // Call this here so it only happens for real content mutations and
   // not cases when the frame constructor calls its own methods to force
   // frame reconstruction.
   mPresContext->RestyleManager()->ContentAppended(aFirstNewContent);
 
   mFrameConstructor->ContentAppended(
       aFirstNewContent, nsCSSFrameConstructor::InsertionKind::Async);
-}
-
-void PresShell::ContentInserted(nsIContent* aChild) {
+
+  // Be aware, don't do anything after destroying crNotifier because the caller
+  // may not guarantee the lifetime of us due to performance reason.
+}
+
+MOZ_CAN_RUN_SCRIPT_BOUNDARY void PresShell::ContentInserted(
+    nsIContent* aChild) {
   MOZ_ASSERT(!mIsDocumentGone, "Unexpected ContentInserted");
   MOZ_ASSERT(aChild->OwnerDoc() == mDocument, "Unexpected document");
 
   if (!mDidInitialize) {
     return;
   }
 
   nsAutoCauseReflowNotifier crNotifier(this);
 
   // Call this here so it only happens for real content mutations and
   // not cases when the frame constructor calls its own methods to force
   // frame reconstruction.
   mPresContext->RestyleManager()->ContentInserted(aChild);
 
   mFrameConstructor->ContentInserted(
       aChild, nullptr, nsCSSFrameConstructor::InsertionKind::Async);
-}
-
-void PresShell::ContentRemoved(nsIContent* aChild,
-                               nsIContent* aPreviousSibling) {
+
+  // Be aware, don't do anything after destroying crNotifier because the caller
+  // may not guarantee the lifetime of us due to performance reason.
+}
+
+MOZ_CAN_RUN_SCRIPT_BOUNDARY void PresShell::ContentRemoved(
+    nsIContent* aChild, nsIContent* aPreviousSibling) {
   MOZ_ASSERT(!mIsDocumentGone, "Unexpected ContentRemoved");
   MOZ_ASSERT(aChild->OwnerDoc() == mDocument, "Unexpected document");
   nsINode* container = aChild->GetParentNode();
 
   // Notify the ESM that the content has been removed, so that
   // it can clean up any state related to the content.
 
   mPresContext->EventStateManager()->ContentRemoved(mDocument, aChild);
@@ -4318,19 +4332,25 @@ void PresShell::ContentRemoved(nsIConten
   mFrameConstructor->ContentRemoved(aChild, oldNextSibling,
                                     nsCSSFrameConstructor::REMOVE_CONTENT);
 
   // NOTE(emilio): It's important that this goes after the frame constructor
   // stuff, otherwise the frame constructor can't see elements which are
   // display: contents / display: none, because we'd have cleared all the style
   // data from there.
   mPresContext->RestyleManager()->ContentRemoved(aChild, oldNextSibling);
+
+  // Be aware, don't do anything after destroying crNotifier because the caller
+  // may not guarantee the lifetime of us due to performance reason.
 }
 
 void PresShell::NotifyCounterStylesAreDirty() {
+  // TODO: Looks like that nsFrameConstructor::NotifyCounterStylesAreDirty()
+  //       does not run script.  If so, we don't need to block script with
+  //       nsAutoCauseReflowNotifier here.
   nsAutoCauseReflowNotifier reflowNotifier(this);
   mFrameConstructor->NotifyCounterStylesAreDirty();
 }
 
 bool PresShell::FrameIsAncestorOfDirtyRoot(nsIFrame* aFrame) const {
   return mDirtyRoots.FrameIsAncestorOfDirtyRoot(aFrame);
 }
 
--- a/layout/base/PresShell.h
+++ b/layout/base/PresShell.h
@@ -334,17 +334,17 @@ class PresShell final : public nsStubDoc
   /**
    * Perform initialization. Constructs the frame for the root content
    * object and then enqueues a reflow of the frame model.
    *
    * Callers of this method must hold a reference to this shell that
    * is guaranteed to survive through arbitrary script execution.
    * Calling Initialize can execute arbitrary script.
    */
-  nsresult Initialize();
+  MOZ_CAN_RUN_SCRIPT_BOUNDARY nsresult Initialize();
 
   /**
    * Reflow the frame model into a new width and height.  The
    * coordinates for aWidth and aHeight must be in standard nscoord's.
    */
   MOZ_CAN_RUN_SCRIPT nsresult
   ResizeReflow(nscoord aWidth, nscoord aHeight,
                ResizeReflowOptions = ResizeReflowOptions::NoOption);
@@ -474,17 +474,17 @@ class PresShell final : public nsStubDoc
   void PostPendingScrollAnchorSelection(
       layout::ScrollAnchorContainer* aContainer);
   void FlushPendingScrollAnchorSelections();
   void PostPendingScrollAnchorAdjustment(
       layout::ScrollAnchorContainer* aContainer);
 
   void CancelAllPendingReflows();
 
-  void NotifyCounterStylesAreDirty();
+  MOZ_CAN_RUN_SCRIPT_BOUNDARY void NotifyCounterStylesAreDirty();
 
   bool FrameIsAncestorOfDirtyRoot(nsIFrame* aFrame) const;
 
   /**
    * Destroy the frames for aElement, and reconstruct them asynchronously if
    * needed.
    *
    * Note that this may destroy frames for an ancestor instead.
@@ -1751,17 +1751,17 @@ class PresShell final : public nsStubDoc
   void PushCurrentEventInfo(nsIFrame* aFrame, nsIContent* aContent);
   void PopCurrentEventInfo();
   nsIContent* GetCurrentEventContent();
 
   friend class ::nsRefreshDriver;
   friend class ::nsAutoCauseReflowNotifier;
 
   void WillCauseReflow();
-  void DidCauseReflow();
+  MOZ_CAN_RUN_SCRIPT void DidCauseReflow();
 
   void CancelPostedReflowCallbacks();
   void FlushPendingScrollAnchorAdjustments();
 
   void SetPendingVisualScrollUpdate(
       const nsPoint& aVisualViewportOffset,
       FrameMetrics::ScrollOffsetUpdateType aUpdateType);