Fix: Avoid analyzing /favicon.ico multiple times

Fix #427

src/lib/connectors/shared/remote-debugging-connector.ts

@@ -326,7 +326,12 @@ export class Connector implements IConnector {
             };
         }
 
-        if (hasAttributeWithValue(data.element, 'link', 'rel', 'icon')) {
+        const hasFaviconLinkTag = hasAttributeWithValue(data.element, 'link', 'rel', 'icon');
+        // If favicon is not present in the html, CDP will send a request to get favicon from the root directory.
+        // We should set the tag to true in this case to avoid fetching favicon twice.
+        const hasFaviconInRootDir = data.request.url === url.resolve(this._finalHref, '/favicon.ico');
+
+        if (hasFaviconLinkTag || hasFaviconInRootDir) {
             this._faviconLoaded = true;
         }
 
@@ -487,8 +492,12 @@ export class Connector implements IConnector {
      * * uses the `src` attribute of `<link rel="icon">` if present.
      * * uses `favicon.ico` and the final url after redirects.
      */
-    private async getFavicon(element?: AsyncHTMLElement) {
-        const href = element ? element.getAttribute('href') : '/favicon.ico';
+    private async getFavicon(element: AsyncHTMLElement) {
+        const href = element.getAttribute('href');
+
+        if (!href) { // `href` is empty.
+            return;
+        }
 
         try {
             debug(`resource ${href} to be fetched`);
@@ -545,7 +554,14 @@ export class Connector implements IConnector {
                     }
 
                     if (!this._faviconLoaded) {
-                        await this.getFavicon((await this._dom.querySelectorAll('link[rel~="icon"]'))[0]);
+                        // We don't need to consider fetching favicon from the root here.
+                        // It should never get to this point if favicon is in the root directory.
+                        const faviconElement = (await this._dom.querySelectorAll('link[rel~="icon"]'))[0];
+
+                        if (faviconElement) {
+                            await this.getFavicon(faviconElement);
+                        }
+
                     }
 
                     await this._server.emitAsync('scan::end', event);

tests/lib/connectors/collect.ts

@@ -0,0 +1,116 @@
+/* eslint-disable no-sync */
+
+import * as fs from 'fs';
+import * as path from 'path';
+import * as url from 'url';
+import * as sinon from 'sinon';
+
+import test from 'ava';
+
+import { builders } from '../../helpers/connectors';
+import { createServer } from '../../helpers/test-server';
+import { IConnector, IConnectorBuilder, INetworkData } from '../../../src/lib/types'; // eslint-disable-line no-unused-vars
+import { generateHTMLPage } from '../../helpers/misc';
+
+test.beforeEach(async (t) => {
+    const sonar = {
+        emit() { },
+        emitAsync() { }
+    };
+
+    const server = createServer();
+
+    await server.start();
+
+    sinon.spy(sonar, 'emit');
+    sinon.spy(sonar, 'emitAsync');
+
+    t.context = {
+        server,
+        sonar
+    };
+});
+
+test.afterEach.always((t) => {
+    t.context.server.stop();
+    t.context.sonar.emit.restore();
+    t.context.sonar.emitAsync.restore();
+});
+
+const pathToFaviconInDir = path.join(__dirname, './fixtures/common/favicon.ico');
+const pathToFaviconInLinkElement = path.join(__dirname, './fixtures/common/favicon-32x32.png');
+
+const runTest = async (t, connectorBuilder, serverConfig?) => {
+    const { sonar } = t.context;
+    const connector: IConnector = await (connectorBuilder)(sonar, {});
+    const server = t.context.server;
+
+    if (serverConfig) {
+        server.configure(serverConfig);
+    }
+
+    await connector.collect(url.parse(`http://localhost:${server.port}/`));
+    await connector.close();
+};
+
+const testConnectorCollect = (connectorInfo) => {
+    const connectorBuilder: IConnectorBuilder = connectorInfo.builder;
+    const name: string = connectorInfo.name;
+
+    test.serial(`[${name}] Favicon is present in a 'link' element with 'rel' attribute set to 'icon' `, async (t) => {
+        const faviconInLinkElementDir = `http://localhost:${t.context.server.port}/images/favicon-32x32.png`;
+        const serverConfig = {
+            '/': generateHTMLPage(`<link rel="icon" type="image/png" href="/images/favicon-32x32.png" sizes="32x32">`),
+            '/images/favicon-favicon-32x32.png': fs.readFileSync(pathToFaviconInLinkElement)
+        };
+
+        await runTest(t, connectorBuilder, serverConfig);
+
+        t.plan(2);
+        t.is(t.context.sonar.emitAsync.withArgs('fetch::end').callCount, 1);
+        t.is(t.context.sonar.emitAsync.withArgs('fetch::end').args[0][1].request.url, faviconInLinkElementDir);
+
+    });
+
+    test.serial(`[${name}] Favicon is present in the root directory`, async (t) => {
+        const faviconInRootDir = `http://localhost:${t.context.server.port}/favicon.ico`;
+        const serverConfig = { '/favicon.ico': fs.readFileSync(pathToFaviconInDir) };
+
+        await runTest(t, connectorBuilder, serverConfig);
+
+        t.plan(2);
+        t.is(t.context.sonar.emitAsync.withArgs('fetch::end').callCount, 1);
+        t.is(t.context.sonar.emitAsync.withArgs('fetch::end').args[0][1].request.url, faviconInRootDir);
+    });
+
+    test.serial(`[${name}] Favicon is present in both the root directory and the 'link' element`, async (t) => {
+        const faviconInLinkElementDir = `http://localhost:${t.context.server.port}/images/favicon-32x32.png`;
+        const serverConfig = {
+            '/': generateHTMLPage(`<link rel="icon" type="image/png" href="/images/favicon-32x32.png" sizes="32x32">`),
+            '/favicon.ico': fs.readFileSync(pathToFaviconInDir),
+            '/images/favicon-favicon-32x32.png': fs.readFileSync(pathToFaviconInLinkElement)
+        };
+
+        await runTest(t, connectorBuilder, serverConfig);
+
+        t.plan(2);
+        t.is(t.context.sonar.emitAsync.withArgs('fetch::end').callCount, 1);
+        // Should load favicon from the link element if it exists
+        t.is(t.context.sonar.emitAsync.withArgs('fetch::end').args[0][1].request.url, faviconInLinkElementDir);
+    });
+
+    test.serial(`[${name}] Favicon is not present in either the root directory or the 'link' element`, async (t) => {
+        const faviconInRootDir = `http://localhost:${t.context.server.port}/favicon.ico`;
+
+        await runTest(t, connectorBuilder);
+
+        t.plan(2);
+        // Requests to `/favicon.ico` are always sent when favicon doesn't exist as a `link` tag in the html.
+        t.is(t.context.sonar.emitAsync.withArgs('fetch::end').callCount, 1);
+        t.is(t.context.sonar.emitAsync.withArgs('fetch::end').args[0][1].request.url, faviconInRootDir);
+    });
+};
+
+builders.forEach((connector) => {
+    testConnectorCollect(connector);
+});