Bug 1611374 - Disallow nested Document.execCommand() calls in Night…

…ly and early Beta r=smaug

Chrome does not allow nested `Document.execCommand()` calls:
https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/editing/commands/document_exec_command.cc;l=75;drc=301e5d079a1b4c29c5b17574d0470e6db7370acc

On the other hand, Safari (and Firefox) allows it.  However, it's worthwhile to
follow Chrome's behavior.

This patch makes `Document::ExecCommand()` return `false` when it's called
while running another `Document::ExecCommand()` call on Nightly and early Beta.
This is exactly same behavior, and we should watch broken web apps reports
for a while before riding this on the train.

And this patch sets the pref to `true` when all crash tests under
`editor/libeditor/crashtests` which depend on nested calls of `execCommand` run
since same things may be reproducible with other DOM APIs.

Differential Revision: https://phabricator.services.mozilla.com/D62815

--HG--
extra : moz-landing-system : lando

dom/base/Document.cpp

@@ -1319,6 +1319,7 @@ Document::Document(const char* aContentType)
       mPendingMaybeEditingStateChanged(false),
       mHasBeenEditable(false),
       mHasWarnedAboutZoom(false),
+      mIsRunningExecCommand(false),
       mPendingFullscreenRequests(0),
       mXMLDeclarationBits(0),
       mOnloadBlockCount(0),
@@ -4694,6 +4695,13 @@ bool Document::ExecCommand(const nsAString& aHTMLCommandName, bool aShowUI,
     return false;
   }
 
+  // If we're running an execCommand, we should just return false.
+  // https://github.com/w3c/editing/issues/200#issuecomment-575241816
+  if (!StaticPrefs::dom_document_exec_command_nested_calls_allowed() &&
+      mIsRunningExecCommand) {
+    return false;
+  }
+
   //  for optional parameters see dom/src/base/nsHistory.cpp: HistoryImpl::Go()
   //  this might add some ugly JS dependencies?
 
@@ -4790,6 +4798,8 @@ bool Document::ExecCommand(const nsAString& aHTMLCommandName, bool aShowUI,
     }
   }
 
+  AutoRunningExecCommandMarker markRunningExecCommand(*this);
+
   // If we cannot use EditorCommand instance directly, we need to handle the
   // command with traditional path (i.e., with DocShell or nsCommandManager).
   if (!editorCommand) {

dom/base/Document.h

@@ -4063,6 +4063,34 @@ class Document : public nsINode,
       const nsAString& aValue = EmptyString(),
       nsAString* aAdjustedValue = nullptr);
 
+  /**
+   * AutoRunningExecCommandMarker is AutoRestorer for mIsRunningExecCommand.
+   * Since it's a bit field, not a bool member, therefore, we cannot use
+   * AutoRestorer for it.
+   */
+  class MOZ_STACK_CLASS AutoRunningExecCommandMarker final {
+   public:
+    AutoRunningExecCommandMarker() = delete;
+    explicit AutoRunningExecCommandMarker(const AutoRunningExecCommandMarker&) =
+        delete;
+    // Guaranteeing the document's lifetime with `MOZ_CAN_RUN_SCRIPT`.
+    MOZ_CAN_RUN_SCRIPT explicit AutoRunningExecCommandMarker(
+        Document& aDocument)
+        : mDocument(aDocument),
+          mHasBeenRunning(aDocument.mIsRunningExecCommand) {
+      aDocument.mIsRunningExecCommand = true;
+    }
+    ~AutoRunningExecCommandMarker() {
+      if (!mHasBeenRunning) {
+        mDocument.mIsRunningExecCommand = false;
+      }
+    }
+
+   private:
+    Document& mDocument;
+    bool mHasBeenRunning;
+  };
+
   // Mapping table from HTML command name to internal command.
   typedef nsDataHashtable<nsStringCaseInsensitiveHashKey, InternalCommandData>
       InternalCommandDataHashtable;
@@ -4569,6 +4597,9 @@ class Document : public nsINode,
   // also record this as a `CountedUnknownProperty`.
   bool mHasWarnedAboutZoom : 1;
 
+  // While we're handling an execCommand call, set to true.
+  bool mIsRunningExecCommand : 1;
+
   uint8_t mPendingFullscreenRequests;
 
   uint8_t mXMLDeclarationBits;

editor/libeditor/crashtests/crashtests.list

@@ -91,8 +91,8 @@ load 1393171.html
 needs-focus load 1402196.html
 load 1402469.html
 load 1402526.html
-asserts(1) load 1402904.html
-asserts(1) load 1405747.html
+pref(dom.document.exec_command.nested_calls_allowed,true) asserts(1) load 1402904.html # assertion is that mutation event listener caused by execCommand calls another execCommand
+pref(dom.document.exec_command.nested_calls_allowed,true) asserts(1) load 1405747.html # assertion is that mutation event listener caused by execCommand calls another execCommand
 load 1405897.html
 load 1408170.html
 asserts(0-1) load 1414581.html
@@ -106,9 +106,9 @@ load 1441619.html
 load 1443664.html
 skip-if(Android) needs-focus load 1444630.html
 load 1446451.html
-asserts(0-2) load 1464251.html # assertion is that mutation event listener modifies content
+pref(dom.document.exec_command.nested_calls_allowed,true) asserts(2) load 1464251.html # assertion is that mutation event listener caused by execCommand calls another execCommand
 pref(layout.accessiblecaret.enabled,true) load 1470926.html
-asserts(0-1) load 1474978.html # assertion is that mutation event listener modifies content
+pref(dom.document.exec_command.nested_calls_allowed,true) asserts(1) load 1474978.html # assertion is that mutation event listener caused by execCommand calls another execCommand
 load 1525481.html
 load 1533913.html
 load 1534394.html

editor/libeditor/tests/test_bug1425997.html

@@ -26,7 +26,7 @@
 SimpleTest.waitForExplicitFinish();
 // 3 assertions are recorded due to nested execCommand() but not a problem.
 // They are necessary to detect invalid method call without mutation event listers.
-SimpleTest.expectAssertions(3, 3);
+SimpleTest.expectAssertions(0, 3);
 SimpleTest.waitForFocus(function() {
   let selection = window.getSelection();
   let editor = document.getElementById("editor");
@@ -38,7 +38,9 @@
     //   after 2nd delete: "\n<!-- -->&nbsp;\n"
     //   after 3rd delete: "\n<!-- -->\n"
     while (editor.innerHTML.includes("&nbsp;")) {
-      document.execCommand("delete", false);
+      if (!document.execCommand("delete", false)) {
+        break;
+      }
     }
   }
   editor.addEventListener("DOMCharacterDataModified", onCharacterDataModified, { once: true });

modules/libpref/init/StaticPrefList.yaml

@@ -151,8 +151,10 @@
 
 #ifdef EARLY_BETA_OR_EARLIER
 #define IS_EARLY_BETA_OR_EARLIER true
+#define IS_NOT_EARLY_BETA_OR_EARLIER false
 #else
 #define IS_EARLY_BETA_OR_EARLIER false
+#define IS_NOT_EARLY_BETA_OR_EARLIER true
 #endif
 
 #ifdef ANDROID
@@ -1447,6 +1449,13 @@
   value: false
   mirror: always
 
+# If set this to true, `Document.execCommand` may be performed nestedly.
+# Otherwise, nested calls just return false.
+- name: dom.document.exec_command.nested_calls_allowed
+  type: bool
+  value: @IS_NOT_EARLY_BETA_OR_EARLIER@
+  mirror: always
+
 - name: dom.enable_window_print
   type: bool
   value: @IS_NOT_ANDROID@