Bug 1611374 - Disallow nested `Document.execCommand()` calls in Nightly and early Beta r=smaug
authorMasayuki Nakano <masayuki@d-toybox.com>
Sat, 15 Feb 2020 19:17:00 +0000
changeset 514390 eaf48f8ba83ff25e9933aa325b842b0da0845bdc
parent 514389 79cc8a9b30e8343205ac0f58f35446509ac6b287
child 514391 9fbafe41107ef0055c90d02ad7d6d8fdae08749e
push id107557
push usermasayuki@d-toybox.com
push dateTue, 18 Feb 2020 10:48:11 +0000
treeherderautoland@eaf48f8ba83f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1611374
milestone75.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1611374 - Disallow nested `Document.execCommand()` calls in Nightly 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
dom/base/Document.cpp
dom/base/Document.h
editor/libeditor/crashtests/crashtests.list
editor/libeditor/tests/test_bug1425997.html
modules/libpref/init/StaticPrefList.yaml
--- a/dom/base/Document.cpp
+++ b/dom/base/Document.cpp
@@ -1314,16 +1314,17 @@ Document::Document(const char* aContentT
       mLoadEventFiring(false),
       mSkipLoadEventAfterClose(false),
       mDisableCookieAccess(false),
       mDisableDocWrite(false),
       mTooDeepWriteRecursion(false),
       mPendingMaybeEditingStateChanged(false),
       mHasBeenEditable(false),
       mHasWarnedAboutZoom(false),
+      mIsRunningExecCommand(false),
       mPendingFullscreenRequests(0),
       mXMLDeclarationBits(0),
       mOnloadBlockCount(0),
       mAsyncOnloadBlockCount(0),
       mWriteLevel(0),
       mContentEditableCount(0),
       mEditingState(EditingState::eOff),
       mCompatMode(eCompatibility_FullStandards),
@@ -4689,16 +4690,23 @@ bool Document::ExecCommand(const nsAStri
     return false;
   }
 
   // if they are requesting UI from us, let's fail since we have no UI
   if (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?
 
   nsAutoString adjustedValue;
   InternalCommandData commandData =
       ConvertToInternalCommand(aHTMLCommandName, aValue, &adjustedValue);
   if (commandData.mCommand == Command::DoNothing) {
     return false;
@@ -4785,16 +4793,18 @@ bool Document::ExecCommand(const nsAStri
         // Return false if editor specific commands is disabled (bug 760052).
         return false;
       }
       // Otherwise, we should redirect it to focused descendant with DocShell.
       editorCommand = nullptr;
     }
   }
 
+  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) {
     MOZ_ASSERT(!commandData.IsAvailableOnlyWhenEditable());
 
     // Special case clipboard write commands like Command::Cut and
     // Command::Copy.  For such commands, we need the behaviour from
     // nsWindowRoot::GetControllers() which is to look at the focused element,
--- a/dom/base/Document.h
+++ b/dom/base/Document.h
@@ -4058,16 +4058,44 @@ class Document : public nsINode,
    *                            instance registered in
    *                            sInternalCommandDataHashtable.
    */
   static InternalCommandData ConvertToInternalCommand(
       const nsAString& aHTMLCommandName,
       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;
   static InternalCommandDataHashtable* sInternalCommandDataHashtable;
 
   mutable std::bitset<eDeprecatedOperationCount> mDeprecationWarnedAbout;
   mutable std::bitset<eDocumentWarningCount> mDocWarningWarnedAbout;
 
@@ -4564,16 +4592,19 @@ class Document : public nsINode,
   bool mHasBeenEditable : 1;
 
   // Whether we've warned about the CSS zoom property.
   //
   // We don't use the general deprecated operation mechanism for this because we
   // 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;
 
   // Currently active onload blockers.
   uint32_t mOnloadBlockCount;
 
   // Onload blockers which haven't been activated yet.
--- a/editor/libeditor/crashtests/crashtests.list
+++ b/editor/libeditor/crashtests/crashtests.list
@@ -86,34 +86,34 @@ load 1383747.html
 load 1383755.html
 load 1383763.html
 load 1384161.html
 load 1388075.html
 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
 load 1415231.html
 load 1423767.html
 needs-focus load 1423776.html
 needs-focus load 1424450.html
 load 1425091.html
 load 1426709.html
 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
 load 1547897.html
 load 1547898.html
 load 1556799.html
 load 1574544.html
 load 1596516.html
--- a/editor/libeditor/tests/test_bug1425997.html
+++ b/editor/libeditor/tests/test_bug1425997.html
@@ -21,29 +21,31 @@ https://bugzilla.mozilla.org/show_bug.cg
 </div>
 
 <pre id="test">
 
 <script class="testbody" type="application/javascript">
 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");
   function onCharacterDataModified() {
     // Until removing all NBSPs which were inserted by the editor,
     // emulates Backspace key with "delete" command.
     // When this test is created, the behavior is:
     //   after 1st delete: "\n<!-- -->&nbsp;&nbsp;\n"
     //   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 });
   editor.focus();
   selection.selectAllChildren(document.getElementById("inline"));
   document.execCommand("insertHTML", false, "text");
   // This expected result is just same as the result of Chrome.
   // If the spec says this is wrong, feel free to change this result.
--- a/modules/libpref/init/StaticPrefList.yaml
+++ b/modules/libpref/init/StaticPrefList.yaml
@@ -146,18 +146,20 @@
 #ifdef MOZILLA_OFFICIAL
 #define IS_NOT_MOZILLA_OFFICIAL false
 #else
 #define IS_NOT_MOZILLA_OFFICIAL true
 #endif
 
 #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
 #define IS_ANDROID true
 #define IS_NOT_ANDROID false
 #else
 #define IS_ANDROID false
 #define IS_NOT_ANDROID true
@@ -1442,16 +1444,23 @@
   value: 1000
   mirror: always
 
 - name: dom.disable_open_during_load
   type: bool
   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@
   mirror: always
 
 - name: dom.element.transform-getters.enabled
   type: bool
   value: false