Fix originData favicon path not being stored (#150)

* Fix originData favicon path not being stored

This bug was introduced in #141.
This PR made it so that favicon fetching did not block storing
originData. This resulted in the originData object being updated after
it had already been stored, meaning the updated originData object was
never stored.

Fixing this issue requires introducing a new function
`storeOriginFavicon`which fetches the favicon, and once/if the fetch
resolves, updates and stores the originData safely using a lock.

* Update src/data/storage.js

Co-authored-by: Sharon Wang <[email protected]>

* Update src/data/storage.js

Co-authored-by: Sharon Wang <[email protected]>

* Update src/data/storage.js

Co-authored-by: Sharon Wang <[email protected]>

* Update src/data/storage.js

Co-authored-by: Sharon Wang <[email protected]>

* Update src/data/storage.js

Co-authored-by: Sharon Wang <[email protected]>

* Update src/data/storage.js

Co-authored-by: Sharon Wang <[email protected]>

* Address PR comments

* Fix nits

Co-authored-by: Sharon Wang <[email protected]>

Author: vezwork

src/data/AkitaOriginData.js

@@ -110,11 +110,11 @@ class AkitaOriginData {
 	}
 
 	/**
-	 * Store the favicon source.
+	 * Set the favicon source.
 	 *
 	 * @param {String} faviconPath The url of the origin's favicon.
 	 */
-	storeOriginFavicon(faviconPath) {
+	setOriginFavicon(faviconPath) {
 		this.faviconSource = faviconPath;
 	}
 

src/data/storage.js

@@ -8,6 +8,7 @@ const AKITA_DATA_TYPE = {
 	'TIME_SPENT': 2,
 	'ORIGIN_FAVICON': 3
 };
+const VALID_AKITA_DATA_TYPE_VALUES = Object.values(AKITA_DATA_TYPE);
 
 const ORIGIN_NAME_LIST_KEY = 'originList';
 const ORIGIN_STATS_KEY = 'originStats';
@@ -83,7 +84,9 @@ async function storeDataIntoAkitaFormatNonMonetized(data, typeOfData) {
  * @param {Boolean} isMonetizedData Whether or not the data being stored is for a monetized origin.
  */
 async function updateAkitaData(originData, data, typeOfData, isMonetizedData) {
-	// TODO: ensure typeOfData is one of AKITA_DATA_TYPE
+	if (!VALID_AKITA_DATA_TYPE_VALUES.includes(typeOfData)) {
+		throw "invalid typeOfData passed to updateAkitaData";
+	}
 
 	// Get existing originStats or create it if it doesn't already exist
 	let originStats = await loadOriginStats();
@@ -107,16 +110,48 @@ async function updateAkitaData(originData, data, typeOfData, isMonetizedData) {
 			updateTimeSpent(originData, originStats, data, isMonetizedData);
 			break;
 		case AKITA_DATA_TYPE.ORIGIN_FAVICON: // Only for a monetized origin
-			updateOriginFavicon(originData, data);
+			storeOriginFavicon(originData.origin, data);
 			break;
-		default:
-			// console.log("invalid data type provided");
 	}
 
 	// Overwrite or create origin stats in storage
 	await storeOriginStats(originStats);
 }
 
+/**
+ * Given an origin and an absolute or relative path to a favicon, this function:
+ *   - fetches the favicon to check if it is a valid favicon,
+ *     - if the fetch is successful, the favicon is valid, so the origin's orginData favicon is set
+ *     to the given favicon path,
+ *     - if the fetch is unsuccessful, the origin's originData favicon is set to "",
+ *   - finally, the updated originData is stored.
+ *
+ * Note: this function's implementation assumes that the specified origin's data has been created
+ * and stored prior to the critical section.
+ *
+ * @param {String} origin The origin to set a favicon for.
+ * @param {String} faviconPath The absolute or relative path to the origin's favicon.
+ */
+ async function storeOriginFavicon(origin, faviconPath) {
+	const absoluteFaviconPath = pathToAbsolutePath(faviconPath, origin);
+	const isFaviconValid = await isFetchStatusOk(absoluteFaviconPath);
+
+	// Once/If the favicon resolves, store the originData with the favicon included
+	const resolveLock = await acquireStoreLock();
+
+	// Get and update existing data for this origin
+	let originData = await loadOriginData(origin);
+	if (isFaviconValid) {
+		originData.setOriginFavicon(absoluteFaviconPath);
+	} else {
+		originData.setOriginFavicon("");
+	}
+
+	// Update the data for this origin in storage
+	await storeOriginData(origin, originData);
+	resolveLock();
+}
+
 
 /***********************************************************
  * Locking Functions
@@ -197,46 +232,44 @@ function updateTimeSpent(originData, originStats, recentTimeSpent, isMonetizedTi
 }
 
 /**
- * Store the path to the origin's favicon in the origin data. If a relative path
- * is provided, construct the absolute path. Attempt to fetch the favicon to check
- * if it is a valid path. If fetch response is 200 OK, the path is valid, so store
- * the url of the favicon.
+ * Makes an absolute path from an input path which is either relative or absolute. If the input path
+ * is absolute, the input path is returned unchanged. If the input path is relative, an absolute
+ * path on the given origin is made with the input path.
  *
- * @param {AkitaOriginData} originData The origin data to update.
- * @param {String} faviconData The absolute or relative path to the site's favicon.
+ * @param {String} path A path to convert to an absolute path, or check that it is already an absolute path.
+ * @param {String} origin If the path is relative, this param is he origin which the path is
+ * relative to. Not used if the path is absolute.
+ * @returns {String} The input path as an absolute path.
  */
-async function updateOriginFavicon(originData, faviconData) {
-	let faviconPath = null;
-	let origin = originData.origin;
-
+function pathToAbsolutePath(path, origin) {
 	// Regex pattern for the start of the path
 	const pathStartPattern = /^(https?:\/\/)(www\.)?/i; // i = ignore case (case insensitive)
 
-	if (pathStartPattern.test(faviconData)) {
+	if (pathStartPattern.test(path)) {
 		// The favicon path is an absolute path
-		faviconPath = faviconData;
+		return path;
 	} else {
 		// The favicon path is relative to the origin
 		if ((origin.charAt(origin.length) !== '/')
-			&& (faviconData.charAt(0) !== '/')
+			&& (path.charAt(0) !== '/')
 		) {
-			faviconPath = origin + "/" + faviconData;
+			return origin + "/" + path;
 		} else {
-			faviconPath = origin + faviconData;
+			return origin + path;
 		}
 	}
+}
 
-	if (faviconPath) {
-		let response = await fetch(faviconPath);
+/**
+ * Fetches a url and resolves to true if the response has status 200 OK.
+ *
+ * @param {String} url The url to be requested by the browser.
+ * @returns {Promise<Boolean>} true if the request response has status 200 OK, false otherwise.
+ */
+async function isFetchStatusOk(url) {
+	let response = await fetch(url);
 
-		if (response) {
-			if (200 === response.status) {
-				originData.storeOriginFavicon(faviconPath);
-			} else {
-				originData.storeOriginFavicon("");
-			}
-		}
-	}
+	return (200 === response?.status);
 }
 
 /***********************************************************