Bug 1871987 - Make `Document::ExecCommand` treat first call by addon as a user input r=smaug

`beforeinput` event shouldn't be fired if it's caused by JS.  However, we
dispatch it when the call is by chrome script or an addon because there is
no user input emulation API for WebExtension and builtin editors make the
change undoable only when JS changes the editor value with
`Document.execCommand`.  Therefore, addons may want to use
`Document.execCommand` for making the change undoable.

On the other hand, nested calls of `Document.execCommand` makes the error
handlings in editor classes too complicated.  Therefore, we don't allow that.
However, this causes that if web apps intercept `beforeinput` events and
prevents the default and calls `document.execCommand`, the first call of
web apps may be first nested call if the `beforeinput` event is caused by
a call of `Document.execCommand` in addon.

Therefore, this patch makes `Document` stores whether `ExecCommand` is called
by content or chrome/addon.  And if the call is improperly nested, keep stopping
handling the command, but allows if and only if the first call is by
chrome/addon.

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

dom/base/Document.cpp

@@ -1385,7 +1385,8 @@ Document::Document(const char* aContentType)
       mPendingMaybeEditingStateChanged(false),
       mHasBeenEditable(false),
       mHasWarnedAboutZoom(false),
-      mIsRunningExecCommand(false),
+      mIsRunningExecCommandByContent(false),
+      mIsRunningExecCommandByChromeOrAddon(false),
       mSetCompleteAfterDOMContentLoaded(false),
       mDidHitCompleteSheetCache(false),
       mUseCountersInitialized(false),
@@ -5366,6 +5367,20 @@ nsresult Document::AutoEditorCommandTarget::GetCommandStateParams(
                               MOZ_KnownLive(targetEditor), nullptr);
 }
 
+Document::AutoRunningExecCommandMarker::AutoRunningExecCommandMarker(
+    Document& aDocument, nsIPrincipal* aPrincipal)
+    : mDocument(aDocument),
+      mTreatAsUserInput(EditorBase::TreatAsUserInput(aPrincipal)),
+      mHasBeenRunningByContent(aDocument.mIsRunningExecCommandByContent),
+      mHasBeenRunningByChromeOrAddon(
+          aDocument.mIsRunningExecCommandByChromeOrAddon) {
+  if (mTreatAsUserInput) {
+    aDocument.mIsRunningExecCommandByChromeOrAddon = true;
+  } else {
+    aDocument.mIsRunningExecCommandByContent = true;
+  }
+}
+
 bool Document::ExecCommand(const nsAString& aHTMLCommandName, bool aShowUI,
                            const nsAString& aValue,
                            nsIPrincipal& aSubjectPrincipal, ErrorResult& aRv) {
@@ -5382,13 +5397,6 @@ 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?
 
@@ -5416,6 +5424,16 @@ bool Document::ExecCommand(const nsAString& aHTMLCommandName, bool aShowUI,
       break;
   }
 
+  AutoRunningExecCommandMarker markRunningExecCommand(*this,
+                                                      &aSubjectPrincipal);
+
+  // 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() &&
+      !markRunningExecCommand.IsSafeToRun()) {
+    return false;
+  }
+
   // Do security check first.
   if (commandData.IsCutOrCopyCommand()) {
     if (!nsContentUtils::IsCutCopyAllowed(this, aSubjectPrincipal)) {
@@ -5433,8 +5451,6 @@ bool Document::ExecCommand(const nsAString& aHTMLCommandName, bool aShowUI,
     }
   }
 
-  AutoRunningExecCommandMarker markRunningExecCommand(*this);
-
   // Next, consider context of command handling which is automatically resolved
   // by order of controllers in `nsCommandManager::GetControllerForCommand()`.
   AutoEditorCommandTarget editCommandTarget(*this, commandData);

dom/base/Document.h

@@ -4334,21 +4334,35 @@ class Document : public nsINode,
     explicit AutoRunningExecCommandMarker(const AutoRunningExecCommandMarker&) =
         delete;
     // Guaranteeing the document's lifetime with `MOZ_CAN_RUN_SCRIPT`.
-    MOZ_CAN_RUN_SCRIPT explicit AutoRunningExecCommandMarker(
+    MOZ_CAN_RUN_SCRIPT AutoRunningExecCommandMarker(Document& aDocument,
-        Document& aDocument)
+                                                    nsIPrincipal* aPrincipal);
-        : mDocument(aDocument),
-          mHasBeenRunning(aDocument.mIsRunningExecCommand) {
-      aDocument.mIsRunningExecCommand = true;
-    }
     ~AutoRunningExecCommandMarker() {
-      if (!mHasBeenRunning) {
+      if (mTreatAsUserInput) {
-        mDocument.mIsRunningExecCommand = false;
+        mDocument.mIsRunningExecCommandByChromeOrAddon =
+            mHasBeenRunningByChromeOrAddon;
+      } else {
+        mDocument.mIsRunningExecCommandByContent = mHasBeenRunningByContent;
       }
     }
 
+    [[nodiscard]] bool IsSafeToRun() const {
+      // We don't allow nested calls of execCommand even if the caller is chrome
+      // script.
+      if (mTreatAsUserInput) {
+        return !mHasBeenRunningByChromeOrAddon && !mHasBeenRunningByContent;
+      }
+      // If current call is by content, we should ignore whether nested with a
+      // call by addon (or chrome script) because the caller wants to emulate
+      // user input for making it undoable.  So, we should treat the first
+      // call as user input.
+      return !mHasBeenRunningByContent;
+    }
+
    private:
     Document& mDocument;
-    bool mHasBeenRunning;
+    bool mTreatAsUserInput;
+    bool mHasBeenRunningByContent;
+    bool mHasBeenRunningByChromeOrAddon;
   };
 
   // Mapping table from HTML command name to internal command.
@@ -4809,8 +4823,12 @@ class Document : public nsINode,
   // also record this as a `CountedUnknownProperty`.
   bool mHasWarnedAboutZoom : 1;
 
-  // While we're handling an execCommand call, set to true.
+  // While we're handling an execCommand call by web app, set
-  bool mIsRunningExecCommand : 1;
+  // to true.
+  bool mIsRunningExecCommandByContent : 1;
+  // While we're handling an execCommand call by an addon (or chrome script),
+  // set to true.
+  bool mIsRunningExecCommandByChromeOrAddon : 1;
 
   // True if we should change the readystate to complete after we fire
   // DOMContentLoaded. This happens when we abort a load and

dom/events/test/browser_beforeinput_by_execCommand_in_contentscript.js

@@ -46,26 +46,116 @@ add_task(async function () {
     true
   );
 
+  /**
+   * Document.execCommand() shouldn't cause `beforeinput`, but it may be used
+   * by addons for emulating user input and make the input undoable on builtin
+   * editors.  Therefore, if and only if it's called by addons, `beforeinput`
+   * should be fired.
+   */
   function runTest() {
-    var editor = content.document.querySelector("[contenteditable]");
+    const editor = content.document.querySelector("[contenteditable]");
     editor.focus();
     content.document.getSelection().selectAllChildren(editor);
-    var beforeinput;
+    let beforeinput;
     editor.addEventListener("beforeinput", aEvent => {
       beforeinput = aEvent;
     });
+    const description = 'Test execCommand("createLink")';
     editor.addEventListener("input", aEvent => {
       if (!beforeinput) {
         sendAsyncMessage("Test:BeforeInputInContentEditable", {
           succeeded: false,
-          message: "No beforeinput event is fired",
+          message: `${description}: No beforeinput event is fired`,
         });
         return;
       }
       sendAsyncMessage("Test:BeforeInputInContentEditable", {
         succeeded:
           editor.innerHTML === '<a href="http://example.com/">abcdef</a>',
-        message: `editor.innerHTML=${editor.innerHTML}`,
+        message: `${description}: editor.innerHTML=${editor.innerHTML}`,
+      });
+    });
+  }
+
+  try {
+    tab.linkedBrowser.messageManager.loadFrameScript(
+      "data:,(" + runTest.toString() + ")();",
+      false
+    );
+
+    let testResult = new Promise(resolve => {
+      let mm = tab.linkedBrowser.messageManager;
+      mm.addMessageListener(
+        "Test:BeforeInputInContentEditable",
+        function onFinish(aMsg) {
+          mm.removeMessageListener(
+            "Test:BeforeInputInContentEditable",
+            onFinish
+          );
+          is(aMsg.data.succeeded, true, aMsg.data.message);
+          resolve();
+        }
+      );
+    });
+    info("Sending Ctrl+K...");
+    await BrowserTestUtils.synthesizeKey(
+      "k",
+      { ctrlKey: true },
+      tab.linkedBrowser
+    );
+    info("Waiting test result...");
+    await testResult;
+  } finally {
+    BrowserTestUtils.removeTab(tab);
+    await extension.unload();
+  }
+});
+
+add_task(async function () {
+  const extension = await installAndStartExtension();
+  const tab = await BrowserTestUtils.openNewForegroundTab(
+    gBrowser,
+    "http://example.com/browser/dom/events/test/file_beforeinput_by_execCommand_in_contentscript.html",
+    true
+  );
+
+  /**
+   * Document.execCommand() from addons should be treated as a user input.
+   * Therefore, it should not block first nested Document.execCommand() call
+   * in a "beforeinput" event listener in the web app.
+   */
+  function runTest() {
+    const editor = content.document.querySelectorAll("[contenteditable]")[1];
+    editor.focus();
+    content.document.getSelection().selectAllChildren(editor);
+    const beforeInputs = [];
+    editor.parentNode.addEventListener(
+      "beforeinput",
+      aEvent => {
+        beforeInputs.push(aEvent);
+      },
+      { capture: true }
+    );
+    const description =
+      'Test web app calls execCommand("insertText") on "beforeinput"';
+    editor.addEventListener("input", aEvent => {
+      if (!beforeInputs.length) {
+        sendAsyncMessage("Test:BeforeInputInContentEditable", {
+          succeeded: false,
+          message: `${description}: No beforeinput event is fired`,
+        });
+        return;
+      }
+      if (beforeInputs.length > 1) {
+        sendAsyncMessage("Test:BeforeInputInContentEditable", {
+          succeeded: false,
+          message: `${description}: Too many beforeinput events are fired`,
+        });
+        return;
+      }
+      sendAsyncMessage("Test:BeforeInputInContentEditable", {
+        succeeded: editor.innerHTML.replace("<br>", "") === "ABCDEF",
+        message: `${description}: editor.innerHTML=${editor.innerHTML}`,
       });
     });
   }
@@ -76,7 +166,6 @@ add_task(async function () {
       false
     );
 
-    let received = false;
     let testResult = new Promise(resolve => {
       let mm = tab.linkedBrowser.messageManager;
       mm.addMessageListener(

dom/events/test/file_beforeinput_by_execCommand_in_contentscript.html

@@ -1,2 +1,9 @@
 <!doctype html>
 <div contenteditable>abcdef</div>
+<div contenteditable>abcdef</div>
+<script>
+document.querySelectorAll("[contenteditable]")[1].addEventListener("beforeinput", event => {
+  document.execCommand("insertText", false, "ABCDEF");
+  event.preventDefault();
+});
+</script>

editor/libeditor/EditorBase.cpp

@@ -6448,16 +6448,20 @@ bool EditorBase::AutoEditActionDataSetter::IsBeforeInputEventEnabled() const {
   if (mEditorBase.IsSuppressingDispatchingInputEvent()) {
     return false;
   }
+  return EditorBase::TreatAsUserInput(mPrincipal);
+}
 
-  // If mPrincipal has set, it means that we're handling an edit action
+// static
-  // which is requested by JS.  If it's not chrome script, we shouldn't
+bool EditorBase::TreatAsUserInput(nsIPrincipal* aPrincipal) {
+  // If aPrincipal it not nullptr, it means that the caller is handling an edit
+  // action which is requested by JS.  If it's not chrome script, we shouldn't
   // dispatch "beforeinput" event.
-  if (mPrincipal && !mPrincipal->IsSystemPrincipal()) {
+  if (aPrincipal && !aPrincipal->IsSystemPrincipal()) {
     // But if it's content script of an addon, `execCommand` calls are a
     // part of browser's default action from point of view of web apps.
     // Therefore, we should dispatch `beforeinput` event.
     // https://github.com/w3c/input-events/issues/91
-    if (!mPrincipal->GetIsAddonOrExpandedAddonPrincipal()) {
+    if (!aPrincipal->GetIsAddonOrExpandedAddonPrincipal()) {
       return false;
     }
   }

editor/libeditor/EditorBase.h

@@ -196,6 +196,12 @@ class EditorBase : public nsIEditor,
     return false;
   }
 
+  /**
+   * This checks whether the call with aPrincipal should or should not be
+   * treated as user input.
+   */
+  [[nodiscard]] static bool TreatAsUserInput(nsIPrincipal* aPrincipal);
+
   PresShell* GetPresShell() const;
   nsPresContext* GetPresContext() const;
   already_AddRefed<nsCaret> GetCaret() const;