Fix issues that could cause unexpected onDocumentReady firing

Fix various issues that could cause the callback to `onDocumentReady` to
fire after the subscription had been canceled.

 - `pollOnUnload` was called twice for the initial document, once at the
   top level of the function and once by the first call to
   `checkForDocumentChange`. This resulted into "unload" listeners being
   added for the same window.

 - The initial async `checkForDocumentChange` call would still fire if
   the subscription was immediately canceled

 - If the current window's "unload" event fired after the subscription
   was canceled, polling would be restarted, effectively re-subscribing
   to document changes.

Author: robertknight

src/annotator/frame-observer.js

@@ -215,9 +215,12 @@ export function onDocumentReady(frame, callback, { pollInterval = 10 } = {}) {
     pollOnUnload();
   };
 
+  let canceled = false;
   pollForDocumentChange = () => {
     cancelPoll();
-    pollTimer = setInterval(checkForDocumentChange, pollInterval);
+    if (!canceled) {
+      pollTimer = setInterval(checkForDocumentChange, pollInterval);
+    }
   };
 
   // Set up observers for signals that the document either has changed or will
@@ -226,16 +229,20 @@ export function onDocumentReady(frame, callback, { pollInterval = 10 } = {}) {
   //  - Polling after the current document is about to be unloaded. This allows
   //    us to detect the new document quickly, but may not fire in some
   //    situations (exact circumstances unclear, but eg. MDN warns about this).
+  //
+  //    This is set up in the first call to `checkForDocumentChange`.
+  //
   //  - The iframe's "load" event. This is guaranteed to fire but only after the
   //    new document is fully loaded.
-  pollOnUnload();
   frame.addEventListener('load', checkForDocumentChange);
 
   // Notify caller about the current document. This fires asynchronously so that
   // the caller will have received the unsubscribe callback first.
-  setTimeout(() => checkForDocumentChange(), 0);
+  const initialCheckTimer = setTimeout(checkForDocumentChange, 0);
 
   return () => {
+    canceled = true;
+    clearTimeout(initialCheckTimer);
     cancelPoll();
     frame.removeEventListener('load', checkForDocumentChange);
   };

src/annotator/test/frame-observer-test.js

@@ -228,7 +228,7 @@ describe('annotator/frame-observer', () => {
       assert.equal(error.message, 'Frame is cross-origin');
     });
 
-    it('returns a callback that stops polling', async () => {
+    it('stops polling when subscription is canceled', async () => {
       const callback = sinon.stub();
       const frame = createFrame(sameOriginURL);
       await waitForEvent(frame, 'load');
@@ -242,6 +242,45 @@ describe('annotator/frame-observer', () => {
 
       assert.calledOnce(callback);
     });
+
+    it('does not start polling if "unload" event is received after subscription is canceled', async () => {
+      const clock = sinon.useFakeTimers();
+      try {
+        const callback = sinon.stub();
+        const frame = createFrame(sameOriginURL);
+        await waitForEvent(frame, 'load');
+
+        const unsubscribe = onDocumentReady(frame, callback);
+        clock.tick();
+        assert.calledOnce(callback);
+
+        const contentWindow = frame.contentWindow;
+        unsubscribe();
+        contentWindow.dispatchEvent(new Event('unload'));
+
+        frame.src = sameOriginURL + '?v2';
+        await waitForEvent(frame, 'load');
+        clock.tick(50); // Wait for any active polling to trigger
+
+        assert.calledOnce(callback);
+      } finally {
+        clock.restore();
+      }
+    });
+
+    it('does not invoke callback if subscription is immediately canceled', async () => {
+      const callback = sinon.stub();
+      const frame = createFrame(sameOriginURL);
+      await waitForEvent(frame, 'load');
+
+      const unsubscribe = onDocumentReady(frame, callback);
+      unsubscribe();
+
+      frame.src = sameOriginURL + '?v2';
+      await waitForEvent(frame, 'load');
+
+      assert.notCalled(callback);
+    });
   });
 
   describe('onNextDocumentReady', () => {