From 98a75931b04759dc8ca31a565a8a36a2784bca07 Mon Sep 17 00:00:00 2001
From: fletcherhaz <fletcher.haz@gmail.com>
Date: Sun, 27 Dec 2020 01:31:30 -0700
Subject: [PATCH] Fix double saving snapshots (#1937)

* Fix double saving snapshots

https://forums.zotero.org/discussion/86796/duplicated-snapshots

I was able to replicate it by adding a 5 second delay here:
https://github.com/zotero/zotero/blob/a72ae1481631dcefe603ef9ff3a1a9f7f5c747d8/chrome/content/zotero/xpcom/translation/translate_item.js#L196

This was caused from a race condition and (a72ae14) did not fully
solve the problem. Now this is tested and fixed.
---
 .../xpcom/connector/server_connector.js       |  21 ++-
 test/tests/server_connectorTest.js            | 162 ++++++++++++++++++
 2 files changed, 175 insertions(+), 8 deletions(-)

diff --git a/chrome/content/zotero/xpcom/connector/server_connector.js b/chrome/content/zotero/xpcom/connector/server_connector.js
index a4b1cddf54..7910ef406b 100644
--- a/chrome/content/zotero/xpcom/connector/server_connector.js
+++ b/chrome/content/zotero/xpcom/connector/server_connector.js
@@ -270,8 +270,6 @@ Zotero.Server.Connector.SaveSession.prototype.update = async function (targetID,
 				for (let item of this._items) {
 					await item.eraseTx();
 				}
-				// Remove pending attachments (will be recreated by calling `save...` below)
-				this.pendingAttachments = [];
 				let actionUC = Zotero.Utilities.capitalize(this._action);
 				// saveItems has a different signature with the session as the first argument
 				let params = [targetID, this._requestData];
@@ -855,7 +853,7 @@ Zotero.Server.Connector.SaveItems.prototype = {
 			cookieSandbox,
 			proxy
 		});
-		// This is a bit tricky. When saving items, the call back`onTopLevelItemsDone` will
+		// This is a bit tricky. When saving items, the callback `onTopLevelItemsDone` will
 		// return the HTTP request to the connector. Then it may spend some time fetching
 		// PDFs. In the meantime, the connector will create a snapshot and send it along to
 		// the `saveSingleFile` endpoint, which quickly adds the data to the session and
@@ -863,22 +861,27 @@ Zotero.Server.Connector.SaveItems.prototype = {
 		// the session switches libraries and we need to save again). So the pending
 		// attachments exist and have already been saved by the time this `saveItems`
 		// promise resolves and we continue executing. So we save the number of existing
-		// attachments before that so prevent double saving.
-		let hasPendingAttachments;
+		// attachments before that to prevent double saving.
+		let hadPendingAttachments = session.pendingAttachments.length > 0;
+		if (hadPendingAttachments) {
+			// If we have pending attachments then we are saving again by switching to
+			// a `filesEditable` library. So we clear the pendingAttachments since they
+			// get added again right below here in `saveItems`
+			session.pendingAttachments = [];
+		}
 		let items = await itemSaver.saveItems(
 			data.items,
 			function (attachment, progress, error) {
 				session.onProgress(attachment, progress, error);
 			},
 			(...args) => {
-				hasPendingAttachments = session.pendingAttachments.length > 0;
 				if (onTopLevelItemsDone) onTopLevelItemsDone(...args);
 			},
 			function (parentItemID, attachment) {
 				session.pendingAttachments.push([parentItemID, attachment]);
 			}
 		);
-		if (hasPendingAttachments) {
+		if (hadPendingAttachments) {
 			// If the session has snapshotContent already (from switching to a `filesEditable` library
 			// then we can save `pendingAttachments` now
 			if (data.snapshotContent) {
@@ -1220,7 +1223,9 @@ Zotero.Server.Connector.SaveSnapshot.prototype = {
 			// pending attachment
 			else if (data.hasOwnProperty('singleFile')) {
 				let session = Zotero.Server.Connector.SessionManager.get(data.sessionID);
-				session.pendingAttachments.push([itemID, { title: data.title, url: data.url }]);
+				session.pendingAttachments = [
+					[itemID, { title: data.title, url: data.url }]
+				];
 			}
 			else if (library.filesEditable) {
 				// Old connector will not use SingleFile so importFromURL now
diff --git a/test/tests/server_connectorTest.js b/test/tests/server_connectorTest.js
index 4a9bcd8534..a2b541ecb3 100644
--- a/test/tests/server_connectorTest.js
+++ b/test/tests/server_connectorTest.js
@@ -1118,6 +1118,168 @@ describe("Connector Server", function () {
 			let contents = await Zotero.File.getContentsAsync(path);
 			assert.match(contents, /^<html style><!--\n Page saved with SingleFile \n url:/);
 		});
+
+		it("should handle race condition with /saveItems", async function () {
+			let collection = await createDataObject('collection');
+			await waitForItemsLoad(win);
+
+			let pdfURL = testServerPath + '/pdf';
+			let nonOADOI = '10.2222/bcde';
+
+			// Promise for item saving
+			let parentIDs, attachmentIDs1, attachmentIDs2;
+			let promise = waitForItemEvent('add').then(function (ids) {
+				parentIDs = ids;
+				return waitForItemEvent('add').then(function (ids) {
+					attachmentIDs1 = ids;
+					return waitForItemEvent('add').then(function (ids) {
+						attachmentIDs2 = ids;
+					});
+				});
+			});
+
+			// Promise for snapshot having been saved
+			let singleFileResolve;
+			let singleFileDone = new Zotero.Promise(function (resolve, reject) {
+				singleFileResolve = resolve;
+			});
+
+			// Special handler to delay writing of file response for 5 seconds to allow
+			// `saveSingleFile` request to finish first before getting PDF
+			httpd.registerPathHandler(
+				'/pdf',
+				{
+					handle: async function (request, response) {
+						response.setStatusLine(null, 200, "OK");
+						let file = Zotero.File.pathToFile(OS.Path.join(getTestDataDirectory().path, 'test.pdf'));
+						response.processAsync();
+						// Delay the PDF processing (simulates a long network request) so that
+						// the SingleFile request below completes first.
+						await singleFileDone;
+						httpd._handler._writeFileResponse(request, file, response, 0, file.fileSize);
+					}
+				}
+			);
+
+			// Setup our `saveItems` and payload and call connector server
+			let title = Zotero.Utilities.randomString();
+			let sessionID = Zotero.Utilities.randomString();
+			let payload = {
+				sessionID,
+				items: [
+					{
+						itemType: 'journalArticle',
+						title: title,
+						DOI: nonOADOI,
+						attachments: [
+							{
+								title: "PDF",
+								url: pdfURL,
+								mimeType: 'application/pdf'
+							},
+							{
+								title: "Snapshot",
+								url: `${testServerPath}/attachment`,
+								mimeType: "text/html",
+								singleFile: true
+							}
+						]
+					}
+				],
+				uri: "http://example.com"
+			};
+
+			let req = await Zotero.HTTP.request(
+				'POST',
+				connectorServerPath + "/connector/saveItems",
+				{
+					headers: {
+						"Content-Type": "application/json"
+					},
+					body: JSON.stringify(payload)
+				}
+			);
+			assert.equal(req.status, 201);
+
+			// Now setup and call our `saveSingleFile` to save snapshot attachment
+			let testDataDirectory = getTestDataDirectory().path;
+			let indexPath = OS.Path.join(testDataDirectory, 'snapshot', 'index.html');
+
+			let body = JSON.stringify(Object.assign(payload, {
+				snapshotContent: await Zotero.File.getContentsAsync(indexPath)
+			}));
+
+			req = await Zotero.HTTP.request(
+				'POST',
+				connectorServerPath + "/connector/saveSingleFile",
+				{
+					headers: {
+						"Content-Type": "application/json"
+					},
+					body
+				}
+			);
+			assert.equal(req.status, 201);
+
+			// Trigger PDF saving to complete now that SingleFile is done.
+			singleFileResolve();
+
+			// Await all item saves
+			await promise;
+
+			// Once the PDF is saved, if the bug exists, the snapshot will saved again.
+			// Once that is completed, then the session will be marked as done so we
+			// wait for that to occur here. Then we can proceed to ensure we have the
+			// proper number of items.
+			let savingDone = false;
+			while (!savingDone) {
+				// eslint-disable-next-line no-await-in-loop
+				req = await Zotero.HTTP.request(
+					'POST',
+					connectorServerPath + "/connector/sessionProgress",
+					{
+						headers: {
+							"Content-Type": "application/json"
+						},
+						body: JSON.stringify({ sessionID })
+					}
+				);
+				savingDone = JSON.parse(req.response).done;
+				if (!savingDone) {
+					// eslint-disable-next-line no-await-in-loop
+					await Zotero.Promise.delay(1000);
+				}
+			}
+
+			// Check parent item
+			assert.lengthOf(parentIDs, 1);
+			let item = Zotero.Items.get(parentIDs[0]);
+			assert.equal(Zotero.ItemTypes.getName(item.itemTypeID), 'journalArticle');
+			assert.isTrue(collection.hasItem(item.id));
+
+			// Ensure we only have one snapshot and one PDF - this is the critical test
+			assert.equal(item.numChildren(), 2);
+
+			// Snapshot is saved first
+			assert.lengthOf(attachmentIDs1, 1);
+			item = Zotero.Items.get(attachmentIDs1[0]);
+			assert.isTrue(item.isImportedAttachment());
+			assert.equal(item.getField('title'), 'Snapshot');
+
+			// Double check snapshot html file has content
+			let attachmentDirectory = Zotero.Attachments.getStorageDirectory(item).path;
+			let path = OS.Path.join(attachmentDirectory, 'attachment.html');
+			assert.isTrue(await OS.File.exists(path));
+			let contents = await Zotero.File.getContentsAsync(path);
+			let expectedContents = await Zotero.File.getContentsAsync(indexPath);
+			assert.equal(contents, expectedContents);
+
+			// Then PDF is saved second
+			assert.lengthOf(attachmentIDs2, 1);
+			item = Zotero.Items.get(attachmentIDs2[0]);
+			assert.isTrue(item.isImportedAttachment());
+			assert.equal(item.getField('title'), 'PDF');
+		});
 	});
 
 	describe("/connector/saveSnapshot", function () {