Repository: microsoft/vscode-edge-devtools
Pull request #381 (2021-05-06 23:24:58)

Fixing a race condition on cdpTargetsProvider.ts
Issue:
![image](https://user-images.githubusercontent.com/43078681/117377142-6861f400-ae87-11eb-8afd-93cbea29ce03.png)
I hit this a couple of time. There is a race condition when the favicon icon is cleared. 

Fix:
- Simplified the code
- There were a couple of issues when we were triggering the deletes & downloads, instead of triggering it on every target that gets launched it triggers only when the target list changes. (most of the refactor is due to this)
- Another issues was  mixing of events and promises when reading directory and when deleting the files. I fixed it using fs.promises instead and took out the events.


Patch:
diff --git a/src/cdpTargetsProvider.ts b/src/cdpTargetsProvider.ts
index 77ea9d0..2dc8102 100644
--- a/src/cdpTargetsProvider.ts
+++ b/src/cdpTargetsProvider.ts
@@ -6,7 +6,7 @@ import TelemetryReporter from 'vscode-extension-telemetry';
 import * as path from 'path';
 import * as fs from 'fs';
 import { CDPTarget } from './cdpTarget';
-import { fixRemoteWebSocket, getListOfTargets, getRemoteEndpointSettings, IRemoteTargetJson, SETTINGS_STORE_NAME } from './utils';
+import { fixRemoteWebSocket, getListOfTargets, getRemoteEndpointSettings, IRemoteTargetJson, isLocalResource, SETTINGS_STORE_NAME } from './utils';
 import { IncomingMessage } from 'http';
 import https = require('https');
 import { setLaunchConfig } from './extension';
@@ -38,37 +38,36 @@ export class CDPTargetsProvider implements vscode.TreeDataProvider<CDPTarget> {
             const { hostname, port, useHttps } = getRemoteEndpointSettings();
             const responseArray = await getListOfTargets(hostname, port, useHttps);
             if (Array.isArray(responseArray)) {
+                await this.clearFaviconResourceDirectory();
                 this.telemetryReporter.sendTelemetryEvent(
                     'view/list',
                     undefined,
                     { targetCount: responseArray.length },
                 );
-                if (responseArray.length) {
-                    await new Promise<void>(resolve => {
-                        let targetsProcessed = 0;
-                        // eslint-disable-next-line @typescript-eslint/no-misused-promises
-                        responseArray.forEach(async (target: IRemoteTargetJson) => {
-                            const actualTarget = fixRemoteWebSocket(hostname, port, target);
-                            if (actualTarget.type === 'page' || actualTarget.type === 'iframe') {
-                                const iconPath = await this.downloadFaviconFromSitePromise(actualTarget.url);
-                                if (iconPath) {
-                                    targets.push(new CDPTarget(actualTarget, '', this.extensionPath, iconPath));
-                                } else {
-                                    targets.push(new CDPTarget(actualTarget, '', this.extensionPath));
-                                }
-                            } else if ((actualTarget.type !== 'service_worker' && actualTarget.type !== 'shared_worker') || willShowWorkers) {
-                                targets.push(new CDPTarget(actualTarget, '', this.extensionPath));
-                            }
-                            targetsProcessed++;
-                            if (targetsProcessed === responseArray.length) {
-                                resolve();
-                            }
-                        });
+                if (responseArray.length > 0) {
+                    const responseIconPromiseArray: Array<Promise<IRemoteTargetJson>> = [];
+                    responseArray.forEach((target: IRemoteTargetJson) => {
+                        const actualTarget = fixRemoteWebSocket(hostname, port, target);
+                        if (actualTarget.type === 'page' || actualTarget.type === 'iframe') {
+                            responseIconPromiseArray.push(this.downloadFaviconFromSitePromise(actualTarget));
+                        } else if ((actualTarget.type !== 'service_worker' && actualTarget.type !== 'shared_worker') || willShowWorkers) {
+                            targets.push(new CDPTarget(actualTarget, '', this.extensionPath));
+                        }
                     });
+
+                    const iconResultsArray = await Promise.all(responseIconPromiseArray);
+                    for (const actualTarget of iconResultsArray) {
+                        if (isLocalResource(actualTarget.faviconUrl)) {
+                            targets.push(new CDPTarget(actualTarget, '', this.extensionPath, actualTarget.faviconUrl));
+                        } else {
+                            targets.push(new CDPTarget(actualTarget, '', this.extensionPath));
+                        }
+                    }
                 }
             } else {
                 this.telemetryReporter.sendTelemetryEvent('view/error/no_json_array');
             }
+
             // Sort the targets by type and then title, but keep 'page' types at the top
             // since those are the ones most likely to be the ones the user wants.
             targets.sort((a: CDPTarget, b: CDPTarget) => {
@@ -93,42 +92,23 @@ export class CDPTargetsProvider implements vscode.TreeDataProvider<CDPTarget> {
     refresh(): void {
         this.telemetryReporter.sendTelemetryEvent('view/refresh');
         this.changeDataEvent.fire(null);
-        void this.clearFaviconResourceDirectory();
         setLaunchConfig();
     }
 
     async clearFaviconResourceDirectory(): Promise<void> {
         const directory = path.join(this.extensionPath, 'resources', 'favicons');
-        let finalFile = false;
-
-        const promise = new Promise<void>(resolve => {
-            fs.readdir(directory, (readdirError: Error | null, files: string[]) => {
-                if (readdirError) {throw readdirError;}
-                for (let i = 0; i < files.length; i++) {
-                    if (i === files.length - 1) {
-                        finalFile = true;
-                    }
-                    const file = files[i];
-                    const fileString = file.toString();
-                    if (fileString !== '.gitkeep') {
-                        fs.unlink(path.join(directory, fileString), unlinkError => {
-                        if (unlinkError) {throw unlinkError;}
-                        if (finalFile) {
-                            resolve();
-                        }
-                        });
-                    } else if (finalFile) {
-                        resolve();
-                    }
-                }
-            });
-        });
-        await promise;
+        const files = await fs.promises.readdir(directory);
+        for (const file of files) {
+            const fileString = file.toString();
+            if (fileString !== '.gitkeep') {
+                await fs.promises.unlink(path.join(directory, fileString));
+            }
+        }
     }
 
-    downloadFaviconFromSitePromise(url: string) : Promise<string | null> | null {
-        if (!url || !url.startsWith('https')) {
-            return null;
+    downloadFaviconFromSitePromise(actualTarget: IRemoteTargetJson) : Promise<IRemoteTargetJson> {
+        if (!actualTarget.url || !actualTarget.url.startsWith('https')) {
+            return Promise.resolve(actualTarget);
         }
         const faviconRegex = /((?:\/\/|\.)([^\.]*)\.[^\.^\/]+\/).*/;
 
@@ -136,44 +116,47 @@ export class CDPTargetsProvider implements vscode.TreeDataProvider<CDPTarget> {
         // urlMatch[0] = .microsoft.com/en-us/microsoft-edge/
         // urlMatch[1] = .microsoft.com/
         // urlMatch[2] = microsoft
-        const urlMatch = faviconRegex.exec(url);
+        const urlMatch = faviconRegex.exec(actualTarget.url);
         let filename;
         if (urlMatch) {
             filename = `${urlMatch[2]}Favicon.ico`;
         } else {
-            return null;
+            return Promise.resolve(actualTarget);
         }
 
         // Replacing ".microsoft.com/en-us/microsoft-edge/" with ".microsoft.com/favicon.ico"
-        const faviconUrl = url.replace(faviconRegex, '$1favicon.ico');
+        const faviconUrl = actualTarget.url.replace(faviconRegex, '$1favicon.ico');
 
         const filePath = path.join(this.extensionPath, 'resources', 'favicons', filename);
 
-        const file = fs.createWriteStream(filePath);
-        const promise = new Promise<string | null>(resolve => {
+        const promise = new Promise<IRemoteTargetJson>((resolve, reject) => {
             https.get(faviconUrl, (response: IncomingMessage) => {
                 if (response.headers['content-type'] && response.headers['content-type'].includes('icon')) {
-                    response.pipe(file);
-                    file.on('error', () => {
-                        resolve(null);
+                    const buffer: Uint8Array[] = [];
+                    response.on('data', data => {
+                        buffer.push(data);
                     });
-                    file.on('finish', () => {
-                        if (file.bytesWritten) {
-                            resolve(filePath);
-                        } else {
-                            resolve(null);
+
+                    // eslint-disable-next-line @typescript-eslint/no-misused-promises
+                    response.on('end', async () => {
+                        try {
+                            await fs.promises.writeFile(filePath, Buffer.concat(buffer));
+                            actualTarget.faviconUrl = filePath;
+                            resolve(actualTarget);
+                        } catch (e) {
+                            reject(actualTarget);
                         }
                     });
                 } else {
-                    resolve(null);
+                    resolve(actualTarget);
                 }
             });
         });
 
-        const timeout = new Promise<null>(resolve => {
+        const timeout = new Promise<IRemoteTargetJson>(resolve => {
             const id = setTimeout(() => {
                 clearTimeout(id);
-                resolve(null);
+                resolve(actualTarget);
             }, 1000);
         });
 
diff --git a/src/utils.ts b/src/utils.ts
index 83edf24..90770e4 100644
--- a/src/utils.ts
+++ b/src/utils.ts
@@ -587,6 +587,24 @@ export function applyPathMapping(
     return sourcePath;
 }
 
+/**
+ * Verifies if a given path points to a local resource.
+ * @param path the path to be tested
+ * @returns True if the path points to a local resource false otherwise.
+ */
+export function isLocalResource(path: string): boolean {
+    try {
+        const pathURL = new URL(path);
+        if (pathURL.protocol && !pathURL.protocol.includes('http')) {
+            return true;
+        }
+    } catch {
+        return false;
+    }
+
+    return false;
+}
+
 /**
  * Verifies if the headless checkbox in extension settings is enabled.
  */


Test patch:
diff --git a/test/cdpTargetsProvider.test.ts b/test/cdpTargetsProvider.test.ts
index d4cc4cb..fa0a3a5 100644
--- a/test/cdpTargetsProvider.test.ts
+++ b/test/cdpTargetsProvider.test.ts
@@ -85,6 +85,7 @@ describe("CDPTargetsProvider", () => {
                 port: "port",
                 useHttps: false,
             }),
+            isLocalResource: jest.fn(),
         };
         jest.doMock("../src/utils", () => mockUtils);
         jest.doMock("../src/cdpTarget", () => ({
@@ -110,10 +111,27 @@ describe("CDPTargetsProvider", () => {
         const dir = './resources/favicons/';
 
         const provider = new ctp.CDPTargetsProvider(mockContext, mockReporter);
-        const microsoftIcon = await provider.downloadFaviconFromSitePromise("https://docs.microsoft.com/en-us/microsoft-edge/");
-        const bingIcon = await provider.downloadFaviconFromSitePromise("https://www.bing.com/");
+        let template: IRemoteTargetJson = {
+            description: '',
+            devtoolsFrontendUrl: '',
+            faviconUrl: '',
+            id: '',
+            title: '',
+            type: '',
+            url: '',
+            webSocketDebuggerUrl: ''
+        }
+        template.url = "https://docs.microsoft.com/en-us/microsoft-edge/";
+        await provider.downloadFaviconFromSitePromise(template);
+        const microsoftIcon = template.faviconUrl;
+        template.url = "https://www.bing.com/";
+        await provider.downloadFaviconFromSitePromise(template);
+        const bingIcon = template.faviconUrl;
         expect(microsoftIcon).not.toEqual(null);
+        expect(microsoftIcon).not.toEqual('');
+        expect(microsoftIcon).not.toEqual(bingIcon);
         expect(bingIcon).not.toEqual(null);
+        expect(bingIcon).not.toEqual('');
         const checkFileLengthPromise = new Promise<number>((resolve) => {
             fs.readdir(dir, (err: Error, files: File[]) => {
                 resolve(files.length);
diff --git a/test/utils.test.ts b/test/utils.test.ts
index c682206..6d1101b 100644
--- a/test/utils.test.ts
+++ b/test/utils.test.ts
@@ -1022,4 +1022,18 @@ describe("utils", () => {
             expect(result.message).toBe('error message');
         });
     });
+
+    describe("isLocalResource", () => {
+        it("tests if a http URL returns false", async () => {
+            let result = utils.isLocalResource('http://bing.com');
+            expect(result).toBe(false);
+            result = utils.isLocalResource('https://www.bing.com');
+            expect(result).toBe(false);
+        });
+
+        it("tests if a local resource path returns true", async () => {
+            let result = utils.isLocalResource('g:/user/test.ico');
+            expect(result).toBe(true);
+        });
+    });
 });