Commit 64533dc4 authored by Eduardo Sanz García's avatar Eduardo Sanz García Committed by Eduardo

Fix race condition when annotations are deleted before anchoring

`deleteAnnotation` RPC event can be triggered before `loadAnnotations`.
This is a race condition in which annotations are asked to be deleted in
the `sidebar` frame before the `guest` frame finishes to anchor the
annotations.

B
This problem is avoided by maintaining one additional set which tracks
the annotations that should be in the page. This can be updated 1) when
an annotation is received, just before it is anchored and 2) when an
annotation is detached. After anchoring completes, the logic can check
if the annotation should still be present in the page and skip saving
the anchor if not.

Solution from here:
https://github.com/hypothesis/client/pull/4007#discussion_r763971360
parent 3b77ad6c
......@@ -168,6 +168,12 @@ export default class Guest {
*/
this.anchors = [];
/**
* Tags of annotations that are currently anchored or being anchored in
* the guest.
*/
this._annotations = /** @type {Set<string>} */ (new Set());
// Set the frame identifier if it's available.
// The "top" guest instance will have this as null since it's in a top frame not a sub frame
/** @type {string|null} */
......@@ -528,11 +534,19 @@ export default class Guest {
// Remove existing anchors for this annotation.
this.detach(annotation.$tag, false /* notify */);
this._annotations.add(annotation.$tag);
// Resolve selectors to ranges and insert highlights.
if (!annotation.target) {
annotation.target = [];
}
const anchors = await Promise.all(annotation.target.map(locate));
// If the annotation was removed while anchoring, don't save the anchors.
if (!this._annotations.has(annotation.$tag)) {
return [];
}
for (let anchor of anchors) {
highlight(anchor);
}
......@@ -559,6 +573,8 @@ export default class Guest {
* @param {boolean} [notify] - For internal use. Whether to emit an `anchorsChanged` notification
*/
detach(tag, notify = true) {
this._annotations.delete(tag);
/** @type {Anchor[]} */
const anchors = [];
for (let anchor of this.anchors) {
......
......@@ -51,6 +51,14 @@ describe('Guest', () => {
return guest;
};
const emitSidebarEvent = (event, ...args) => {
for (let [evt, fn] of fakeBridge.on.args) {
if (event === evt) {
fn(...args);
}
}
};
beforeEach(() => {
guests = [];
highlighter = {
......@@ -195,14 +203,6 @@ describe('Guest', () => {
});
describe('events from sidebar frame', () => {
const emitSidebarEvent = (event, ...args) => {
for (let [evt, fn] of fakeBridge.on.args) {
if (event === evt) {
fn(...args);
}
}
};
describe('on "focusAnnotations" event', () => {
it('focuses any annotations with a matching tag', () => {
const highlight0 = document.createElement('span');
......@@ -1128,6 +1128,21 @@ describe('Guest', () => {
true
);
});
it('prevents saving of annotations that are deleted while been anchored', async () => {
const guest = createGuest();
const annotation = {
$tag: 'tag1',
target: [{ selector: [{ type: 'TextQuoteSelector', exact: 'hello' }] }],
};
fakeIntegration.anchor.resolves(range);
emitSidebarEvent('loadAnnotations', [annotation]);
emitSidebarEvent('deleteAnnotation', annotation.$tag);
await delay(0);
assert.lengthOf(guest.anchors, 0);
});
});
describe('#detach', () => {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment