[fix] Dequeue messages when committing

src/static/js/collab_client.js

@@ -97,6 +97,10 @@ const getCollabClient = (ace2editor, serverVars, initialUserInfo, options, _pad)
         setChannelState('DISCONNECTED', 'slowcommit');
       } else if (state === 'COMMITTING' && msgQueue.length === 0 && (t - lastCommitTime) > 5000) {
         callbacks.onConnectionTrouble('SLOW');
+      } else if (msgQueue.length > 0) {
+        // in slow or bad network conditions, there might be enqueued messages
+        // while the state is still COMMITTING
+        dequeueMessages();
       } else {
         // run again in a few seconds, to detect a disconnect
         setTimeout(wrapRecordingErrors('setTimeout(handleUserChanges)', handleUserChanges), 3000);
@@ -114,31 +118,7 @@ const getCollabClient = (ace2editor, serverVars, initialUserInfo, options, _pad)
 
     // apply msgQueue changeset.
     if (msgQueue.length !== 0) {
-      let msg;
+      dequeueMessages();
-      while ((msg = msgQueue.shift())) {
-        const newRev = msg.newRev;
-        rev = newRev;
-        if (msg.type === 'ACCEPT_COMMIT') {
-          editor.applyPreparedChangesetToBase();
-          setStateIdle();
-          callCatchingErrors('onInternalAction', () => {
-            callbacks.onInternalAction('commitAcceptedByServer');
-          });
-          callCatchingErrors('onConnectionTrouble', () => {
-            callbacks.onConnectionTrouble('OK');
-          });
-          handleUserChanges();
-        } else if (msg.type === 'NEW_CHANGES') {
-          const changeset = msg.changeset;
-          const author = (msg.author || '');
-          const apool = msg.apool;
-
-          editor.applyChangesToBase(changeset, author, apool);
-        }
-      }
-      if (isPendingRevision) {
-        setIsPendingRevision(false);
-      }
     }
 
     let sentMessage = false;
@@ -170,6 +150,42 @@ const getCollabClient = (ace2editor, serverVars, initialUserInfo, options, _pad)
     }
   };
 
+  const acceptCommit = () => {
+    editor.applyPreparedChangesetToBase();
+    setStateIdle();
+    callCatchingErrors('onInternalAction', () => {
+      callbacks.onInternalAction('commitAcceptedByServer');
+    });
+    callCatchingErrors('onConnectionTrouble', () => {
+      callbacks.onConnectionTrouble('OK');
+    });
+    handleUserChanges();
+  };
+
+  const enqueueMessage = (msg) => {
+    msgQueue.push(msg);
+  };
+
+  const dequeueMessages = () => {
+    let msg;
+    while ((msg = msgQueue.shift())) {
+      const newRev = msg.newRev;
+      rev = newRev;
+      if (msg.type === 'ACCEPT_COMMIT') {
+        acceptCommit();
+      } else if (msg.type === 'NEW_CHANGES') {
+        const changeset = msg.changeset;
+        const author = (msg.author || '');
+        const apool = msg.apool;
+
+        editor.applyChangesToBase(changeset, author, apool);
+      }
+    }
+    if (isPendingRevision) {
+      setIsPendingRevision(false);
+    }
+  };
+
   const setUpSocket = () => {
     setChannelState('CONNECTED');
     doDeferredActions();
@@ -226,7 +242,7 @@ const getCollabClient = (ace2editor, serverVars, initialUserInfo, options, _pad)
           // setChannelState("DISCONNECTED", "badmessage_newchanges");
           return;
         }
-        msgQueue.push(msg);
+        enqueueMessage(msg);
         return;
       }
 
@@ -247,7 +263,7 @@ const getCollabClient = (ace2editor, serverVars, initialUserInfo, options, _pad)
           // setChannelState("DISCONNECTED", "badmessage_acceptcommit");
           return;
         }
-        msgQueue.push(msg);
+        enqueueMessage(msg);
         return;
       }
 
@@ -257,15 +273,7 @@ const getCollabClient = (ace2editor, serverVars, initialUserInfo, options, _pad)
         return;
       }
       rev = newRev;
-      editor.applyPreparedChangesetToBase();
+      acceptCommit();
-      setStateIdle();
-      callCatchingErrors('onInternalAction', () => {
-        callbacks.onInternalAction('commitAcceptedByServer');
-      });
-      callCatchingErrors('onConnectionTrouble', () => {
-        callbacks.onConnectionTrouble('OK');
-      });
-      handleUserChanges();
     } else if (msg.type === 'CLIENT_RECONNECT') {
       // Server sends a CLIENT_RECONNECT message when there is a client reconnect.
       // Server also returns all pending revisions along with this CLIENT_RECONNECT message
@@ -289,7 +297,7 @@ const getCollabClient = (ace2editor, serverVars, initialUserInfo, options, _pad)
           return;
         }
         msg.type = 'NEW_CHANGES';
-        msgQueue.push(msg);
+        enqueueMessage(msg);
         return;
       }
 

src/tests/frontend/helper/multipleUsers.js

@@ -0,0 +1,182 @@
+'use strict';
+
+helper.multipleUsers = {
+  thisUser: null,
+  otherUser: null,
+};
+
+// open the same pad on the same frame (does not allow concurrent editions to pad)
+helper.multipleUsers.loadSamePadAsAnotherUser = (done, tokenForOtherUser) => {
+  // change user
+  const token = tokenForOtherUser || _createTokenForAnotherUser();
+  _removeExistingTokensFromCookie();
+  _setTokenOnCookie(token);
+
+  // reload pad
+  const padId = helper.padChrome$.window.clientVars.padId;
+  helper.newPad(done, padId);
+};
+
+// open the same pad on different frames (allows concurrent editions to pad)
+helper.multipleUsers.openSamePadOnWithAnotherUser = (done) => {
+  const self = helper.multipleUsers;
+
+  // do some cleanup, in case any of the tests failed on the previous run
+  const currentToken = _createTokenForCurrentUser();
+  const otherToken = _createTokenForAnotherUser();
+  _removeExistingTokensFromCookie();
+
+  self.thisUser = {
+    $frame: $('#iframe-container iframe'),
+    token: currentToken,
+    // we'll switch between pads, need to store current values of helper.pad*
+    // to be able to restore those values later
+    padChrome$: helper.padChrome$,
+    padOuter$: helper.padOuter$,
+    padInner$: helper.padInner$,
+  };
+
+  self.otherUser = {
+    token: otherToken,
+  };
+
+  // need to perform as the other user, otherwise we'll get the userdup error message
+  self.performAsOtherUser(_createFrameForOtherUser, done);
+};
+
+helper.multipleUsers.performAsOtherUser = (action, done) => {
+  const self = helper.multipleUsers;
+
+  self.startActingLikeOtherUser();
+  action(() => {
+    // go back to initial state when we're done
+    self.startActingLikeThisUser();
+    done();
+  });
+};
+
+helper.multipleUsers.closePadForOtherUser = () => {
+  const self = helper.multipleUsers;
+
+  self.thisUser.$frame.attr('style', ''); // make the default ocopy the full height
+  self.otherUser.$frame.remove();
+};
+
+helper.multipleUsers.startActingLikeOtherUser = () => {
+  const self = helper.multipleUsers;
+  _startActingLike(self.otherUser);
+};
+
+helper.multipleUsers.startActingLikeThisUser = () => {
+  const self = helper.multipleUsers;
+  _startActingLike(self.thisUser);
+};
+
+// adapted form helper.js on Etherpad code
+const _getFrameJQuery = (codesToLoad, $iframe) => {
+  const win = $iframe[0].contentWindow;
+  const doc = win.document;
+
+  for (let i = 0; i < codesToLoad.length; i++) {
+    win.eval(codesToLoad[i]);
+  }
+
+  win.$.window = win;
+  win.$.document = doc;
+
+  return win.$;
+};
+
+const _loadJQueryCodeForOtherFrame = (done) => {
+  const self = helper.multipleUsers;
+
+  $.get('/static/js/jquery.js').done((code) => {
+    // make sure we don't override existing jquery
+    const jQueryCode = `if(typeof $ === "undefined") {\n${code}\n}`;
+    $.get('/tests/frontend/lib/sendkeys.js').done((sendkeysCode) => {
+      const codesToLoad = [jQueryCode, sendkeysCode];
+
+      self.otherUser.padChrome$ = _getFrameJQuery(codesToLoad, self.otherUser.$frame);
+      self.otherUser.padOuter$ = _getFrameJQuery(codesToLoad, self.otherUser.padChrome$('iframe[name="ace_outer"]'));
+      self.otherUser.padInner$ = _getFrameJQuery(codesToLoad, self.otherUser.padOuter$('iframe[name="ace_inner"]'));
+
+      // update helper vars now that they are available
+      helper.padChrome$ = self.otherUser.padChrome$;
+      helper.padOuter$ = self.otherUser.padOuter$;
+      helper.padInner$ = self.otherUser.padInner$;
+
+      done();
+    });
+  });
+};
+
+const _createFrameForOtherUser = (done) => {
+  const self = helper.multipleUsers;
+
+  // create the iframe
+  const padUrl = self.thisUser.$frame.attr('src');
+  self.otherUser.$frame = $(`<iframe id="other_pad" src="${padUrl}"></iframe>`);
+
+  // place one iframe (visually) below the other
+  self.thisUser.$frame.attr('style', 'height: 50%');
+  self.otherUser.$frame.attr('style', 'height: 50%; top: 50%');
+  self.otherUser.$frame.insertAfter(self.thisUser.$frame);
+
+  // wait for other pad to load
+  self.otherUser.$frame.one('load', () => {
+    const $editorLoadingMessage = self.otherUser.$frame.contents().find('#editorloadingbox');
+    const $errorMessageModal = self.thisUser.$frame.contents().find('#connectivity .userdup');
+
+    helper.waitFor(() => {
+      const finishedLoadingOtherFrame = !$editorLoadingMessage.is(':visible');
+      // make sure we don't get the userdup by mistake
+      const didNotDetectUserDup = !$errorMessageModal.is(':visible');
+
+      return finishedLoadingOtherFrame && didNotDetectUserDup;
+    }, 50000).done(() => {
+      // need to get values for self.otherUser.pad* vars
+      _loadJQueryCodeForOtherFrame(done);
+    });
+  });
+};
+
+const _getDocumentWithCookie = () => (
+  helper.padChrome$
+    ? helper.padChrome$.document
+    : helper.multipleUsers.thisUser.$frame.get(0).contentDocument
+);
+
+const _setTokenOnCookie = (token) => {
+  _getDocumentWithCookie().cookie = `token=${token};secure`;
+};
+
+const _getTokenFromCookie = () => {
+  const fullCookie = _getDocumentWithCookie().cookie;
+  return fullCookie.replace(/.*token=([^;]*).*/, '$1').trim();
+};
+
+const _createTokenForCurrentUser = () => (
+  _getTokenFromCookie().replace(/-other_user.*/g, '')
+);
+
+const _createTokenForAnotherUser = () => {
+  const currentToken = _createTokenForCurrentUser();
+  return `${currentToken}-other_user${helper.randomString(4)}`;
+};
+
+const _startActingLike = (user) => {
+  // update helper references, so other methods will act as if the main frame
+  // was the one we're using from now on
+  helper.padChrome$ = user.padChrome$;
+  helper.padOuter$ = user.padOuter$;
+  helper.padInner$ = user.padInner$;
+
+  _setTokenOnCookie(user.token);
+};
+
+const _removeExistingTokensFromCookie = () => {
+  // Expire cookie, to make sure it is removed by the browser.
+  // See https://developer.mozilla.org/en-US/docs/Web/API/Document/cookie#Example_4_Reset_the_previous_cookie
+  _getDocumentWithCookie().cookie = 'token=foo;expires=Thu, 01 Jan 1970 00:00:00 GMT; path=/p';
+  _getDocumentWithCookie().cookie = 'token=foo;expires=Thu, 01 Jan 1970 00:00:00 GMT; path=/';
+};

src/tests/frontend/index.html

@@ -20,6 +20,7 @@
     <script src="helper.js"></script>
     <script src="helper/methods.js"></script>
     <script src="helper/ui.js"></script>
+    <script src="helper/multipleUsers.js"></script>
 
     <script src="specs_list.js"></script>
     <script src="runner.js"></script>

src/tests/frontend/specs/collab_client.js

@@ -0,0 +1,130 @@
+'use strict';
+
+describe('Messages in the COLLABROOM', function () {
+  const newTextOfUser1 = 'text created by user 1';
+  const newTextOfUser2 = 'text created by user 2';
+
+  before(function (done) {
+    helper.newPad(() => {
+      helper.multipleUsers.openSamePadOnWithAnotherUser(done);
+    });
+    this.timeout(60000);
+  });
+
+  context('when user 1 is with slow or bad network conditions', function () {
+    before(function (done) {
+      helper.multipleUsers.startActingLikeThisUser();
+      simulateSlowOrBadNetworkConditions();
+      done();
+    });
+
+    context('and user 1 is in international composition', function () {
+      before(function (done) {
+        replaceLineText(1, newTextOfUser1, () => {
+          simulateInternationalCompositionStartEvent();
+          done();
+        });
+      });
+
+      context('and user 2 edits the text', function () {
+        before(function (done) {
+          this.timeout(4000);
+          helper.multipleUsers.startActingLikeOtherUser();
+          replaceLineText(2, newTextOfUser2, () => {
+            helper.multipleUsers.startActingLikeThisUser();
+            done();
+          });
+        });
+
+        context('and user 1 ends the international composition', function () {
+          before(function (done) {
+            simulateInternationalCompositionEndEvent();
+            done();
+          });
+
+          context('and both users add more editions', function () {
+            before(function (done) {
+              this.timeout(10000);
+
+              helper.multipleUsers.startActingLikeOtherUser();
+              replaceLineText(4, newTextOfUser2, () => {
+                helper.multipleUsers.startActingLikeThisUser();
+                replaceLineText(3, newTextOfUser1, done);
+              });
+            });
+
+            it('user 1 has all editions of user 2', function (done) {
+              this.timeout(5000);
+              helper.multipleUsers.startActingLikeThisUser();
+              helper.waitFor(() => {
+                const inner$ = helper.padInner$;
+                const expectedLines = [2, 4];
+                return expectedLines.every((line) => (
+                  inner$('div').eq(line).text() === newTextOfUser2)
+                );
+              }, 4000).done(done);
+            });
+
+            it('user 2 has all editions of user 1', function (done) {
+              this.timeout(5000);
+              helper.multipleUsers.startActingLikeOtherUser();
+              helper.waitFor(() => {
+                const inner$ = helper.padInner$;
+                const expectedLines = [1, 3];
+                return expectedLines.every((line) => (
+                  inner$('div').eq(line).text() === newTextOfUser1)
+                );
+              }, 4000).done(done);
+            });
+          });
+        });
+      });
+    });
+  });
+
+  const triggerEvent = (eventName) => {
+    const event = new helper.padInner$.Event(eventName);
+    helper.padInner$('#innerdocbody').trigger(event);
+  };
+
+  const simulateInternationalCompositionStartEvent = () => {
+    triggerEvent('compositionstart');
+  };
+
+  const simulateInternationalCompositionEndEvent = () => {
+    triggerEvent('compositionend');
+  };
+
+  const simulateSlowOrBadNetworkConditions = () => {
+    // to simulate slow or bad network conditions (packet loss), we delay the
+    // sending of messages through the socket.
+    const originalFunction = helper.padChrome$.window.pad.socket.json.send;
+    const mockedFunction = function (...args) {
+      const context = this;
+      setTimeout(() => {
+        originalFunction.apply(context, args);
+      }, 4000);
+    };
+    mockedFunction.bind(helper.padChrome$.window.pad.socket);
+    helper.padChrome$.window.pad.socket.json.send = mockedFunction;
+  };
+
+  const replaceLineText = (lineNumber, newText, done) => {
+    const inner$ = helper.padInner$;
+
+    // get the line element
+    const $line = inner$('div').eq(lineNumber);
+
+    // simulate key presses to delete content
+    $line.sendkeys('{selectall}'); // select all
+    $line.sendkeys('{del}'); // clear the first line
+    $line.sendkeys(newText); // insert the string
+
+    helper.waitFor(() => (
+      inner$('div').eq(lineNumber).text() === newText
+    ), 2000).done(() => {
+      // give some time to receive NEW_CHANGES message
+      setTimeout(done, 2000);
+    });
+  };
+});