Add three unit tests to check `_removeFrame` callback

`_removeFrame` must be called in these three scenarios:

- `enable-annotation` attribute is removed from iframe
- `src` attribute is modified in the iframe
- iframe is deleted

When the iframe is deleted there are two possible paths for the execution of
the `_removeFrame`:

1. First and faster execution of `_removeFrame`: `iframe.remove()`
   triggers the `unload` event which calls the `_removeFrame`

2. Second and delayed execution of `_removeFrame`: `iframe.remove()`
   triggers the `MutationObserver` (debounced by 40 ms). This could
   cause the `_removeFrame` to be fired if the first path would not
   remove the iframe from the list of `_handledFrames`.

I moved the addition and deletion of the iframes to `_handledFrames` as
earlier as possible in the `_addFrame` and `_removeFrame` methods to
avoid racing conditions. A consequence of this is that the `_addFrame`
is executed only once per iframe. If it fails (for example, because the
iframe is from a different origin) it is not constantly retried.

src/annotator/frame-observer.js

@@ -39,13 +39,13 @@ export default class FrameObserver {
    * @param {HTMLIFrameElement} frame
    */
   async _addFrame(frame) {
+    this._handledFrames.add(frame);
     if (isAccessible(frame)) {
       await onDocumentReady(frame);
       const frameWindow = /** @type {Window} */ (frame.contentWindow);
       frameWindow.addEventListener('unload', () => {
         this._removeFrame(frame);
       });
-      this._handledFrames.add(frame);
       this._onFrameAdded(frame);
     } else {
       // Could warn here that frame was not cross origin accessible
@@ -56,8 +56,8 @@ export default class FrameObserver {
    * @param {HTMLIFrameElement} frame
    */
   _removeFrame(frame) {
-    this._onFrameRemoved(frame);
     this._handledFrames.delete(frame);
+    this._onFrameRemoved(frame);
   }
 
   _discoverFrames() {

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

@@ -20,6 +20,12 @@ describe('FrameObserver', () => {
     return frame;
   }
 
+  function waitForIFrameUnload(frame) {
+    return new Promise(resolve =>
+      frame.contentWindow.addEventListener('unload', resolve)
+    );
+  }
+
   beforeEach(() => {
     container = document.createElement('div');
     document.body.appendChild(container);
@@ -67,6 +73,57 @@ describe('FrameObserver', () => {
     assert.notCalled(onFrameAdded);
   });
 
+  it('removal of the annotatable iframe triggers onFrameRemoved', done => {
+    sinon.stub(frameObserver, '_removeFrame').callThrough();
+    const frame = createAnnotatableIFrame();
+
+    waitForFrameObserver()
+      .then(() => {
+        assert.calledOnce(onFrameAdded);
+        assert.calledWith(onFrameAdded, frame);
+      })
+      .then(() => {
+        frame.remove();
+      });
+
+    waitForIFrameUnload(frame)
+      .then(() => waitForFrameObserver())
+      .then(() => {
+        assert.calledOnce(frameObserver._removeFrame);
+        assert.calledOnce(onFrameRemoved);
+        assert.calledWith(onFrameRemoved, frame);
+      })
+      .then(done);
+  });
+
+  it('removal of the `enable-annotation` attribute triggers onFrameRemoved', async () => {
+    const frame = createAnnotatableIFrame();
+    await waitForFrameObserver();
+
+    assert.calledOnce(onFrameAdded);
+    assert.calledWith(onFrameAdded, frame);
+
+    frame.removeAttribute('enable-annotation');
+    await waitForFrameObserver();
+
+    assert.calledOnce(onFrameRemoved);
+    assert.calledWith(onFrameRemoved, frame);
+  });
+
+  it('changing the `src` attribute triggers onFrameRemoved', async () => {
+    const frame = createAnnotatableIFrame();
+    await waitForFrameObserver();
+
+    assert.calledOnce(onFrameAdded);
+    assert.calledWith(onFrameAdded, frame);
+
+    frame.setAttribute('src', document.location);
+    await waitForIFrameUnload(frame);
+
+    assert.calledOnce(onFrameRemoved);
+    assert.calledWith(onFrameRemoved, frame);
+  });
+
   // This test doesn't work. Surprisingly, `isAccessible` returns `true` even
   // thought the iframe is from a different domain. My suspicion is that the
   // iframe is accessed at a time where the loading has not yet started and it