diff --git a/src/lib/connectors/jsdom/jsdom.ts b/src/lib/connectors/jsdom/jsdom.ts index 1c0da60ed58..2bc5f6210f7 100644 --- a/src/lib/connectors/jsdom/jsdom.ts +++ b/src/lib/connectors/jsdom/jsdom.ts @@ -226,7 +226,7 @@ class JSDOMConnector implements IConnector { * * uses `favicon.ico` and the final url after redirects. */ private async getFavicon(element?: HTMLElement) { - const href = element ? element.getAttribute('href') : '/favicon.ico'; + const href = (element && element.getAttribute('href')) || '/favicon.ico'; try { await util.promisify(this.resourceLoader).call(this, { element, url: url.parse(href) }); diff --git a/src/lib/connectors/shared/remote-debugging-connector.ts b/src/lib/connectors/shared/remote-debugging-connector.ts index 994c6bcef25..1ae1ca28bcd 100644 --- a/src/lib/connectors/shared/remote-debugging-connector.ts +++ b/src/lib/connectors/shared/remote-debugging-connector.ts @@ -64,6 +64,7 @@ export class Connector implements IConnector { private _tabs = []; /** Tells if the page has specified a manifest or not. */ private _manifestIsSpecified: boolean = false; + /** Tells if a favicon of a page has been downloaded from a link tag */ private _faviconLoaded: boolean = false; private _targetNetworkData: INetworkData; @@ -151,6 +152,21 @@ export class Connector implements IConnector { return null; } + /** Check if a request or response is to or from `/favicon.ico` */ + private rootFaviconRequestOrResponse(params) { + if (!this._finalHref) { + return false; + } + const faviconUrl = url.resolve(this._finalHref, '/favicon.ico'); + const event = params.request || params.response; + + if (!event) { + return false; + } + + return this._finalHref && faviconUrl === event.url; + } + /** Event handler for when the browser is about to make a request. */ private async onRequestWillBeSent(params) { const requestUrl: string = params.request.url; @@ -178,7 +194,14 @@ export class Connector implements IConnector { const eventName: string = this._href === requestUrl ? 'targetfetch::start' : 'fetch::start'; debug(`About to start fetching ${requestUrl}`); - await this._server.emitAsync(eventName, { resource: requestUrl }); + + /* `getFavicon` will make attempts to download favicon later. + * Ignore `cdp` requests to download favicon from the root + * to avoid emitting duplidate events. + */ + if (!this.rootFaviconRequestOrResponse(params)) { + await this._server.emitAsync(eventName, { resource: requestUrl }); + } } /** Event handler fired when HTTP request fails for some reason. */ @@ -227,7 +250,13 @@ export class Connector implements IConnector { this._requests.delete(params.requestId); - await this._server.emitAsync(eventName, event); + /* `getFavicon` will make attempts to download favicon later. + * Ignore `cdp` requests to download favicon from the root + * to avoid emitting duplidate events. + */ + if (!this.rootFaviconRequestOrResponse(params)) { + await this._server.emitAsync(eventName, event); + } } private async getResponseBody(cdpResponse) { @@ -330,7 +359,13 @@ export class Connector implements IConnector { this._faviconLoaded = true; } - await this._server.emitAsync(eventName, data); + /* `getFavicon` will make attempts to download favicon later. + * Ignore `cdp` requests to download favicon from the root + * to avoid emitting duplidate events. + */ + if (!this.rootFaviconRequestOrResponse(params)) { + await this._server.emitAsync(eventName, data); + } /* We don't need to store the request anymore so we can remove it and ignore it * if we receive it in `onLoadingFailed` (used only for "catastrophic" failures). @@ -487,8 +522,8 @@ export class Connector implements IConnector { * * uses the `src` attribute of `` 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 && element.getAttribute('href')) || '/favicon.ico'; try { debug(`resource ${href} to be fetched`); @@ -545,7 +580,9 @@ export class Connector implements IConnector { } if (!this._faviconLoaded) { - await this.getFavicon((await this._dom.querySelectorAll('link[rel~="icon"]'))[0]); + const faviconElement = (await this._dom.querySelectorAll('link[rel~="icon"]'))[0]; + + await this.getFavicon(faviconElement); } await this._server.emitAsync('scan::end', event); diff --git a/tests/lib/connectors/collect.ts b/tests/lib/connectors/collect.ts new file mode 100644 index 00000000000..b49a1b69b82 --- /dev/null +++ b/tests/lib/connectors/collect.ts @@ -0,0 +1,126 @@ +/* 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(``), + '/images/favicon-favicon-32x32.png': fs.readFileSync(pathToFaviconInLinkElement) + }; + + await runTest(t, connectorBuilder, serverConfig); + + 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.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(``), + '/favicon.ico': fs.readFileSync(pathToFaviconInDir), + '/images/favicon-favicon-32x32.png': fs.readFileSync(pathToFaviconInLinkElement) + }; + + await runTest(t, connectorBuilder, serverConfig); + + 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 present in both the root directory and the 'link' element, but the 'link' element has empty 'href'`, async (t) => { + const faviconInRootDir = `http://localhost:${t.context.server.port}/favicon.ico`; + const serverConfig = { + '/': generateHTMLPage(``), + '/favicon.ico': fs.readFileSync(pathToFaviconInDir) + }; + + await runTest(t, connectorBuilder, serverConfig); + + t.is(t.context.sonar.emitAsync.withArgs('fetch::end').callCount, 1); + // Should load favicon from the root even though the link element exists because 'href' is empty. + t.is(t.context.sonar.emitAsync.withArgs('fetch::end').args[0][1].request.url, faviconInRootDir); + }); + + 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); + + // 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); +}); diff --git a/tests/lib/connectors/fixtures/common/favicon-32x32.png b/tests/lib/connectors/fixtures/common/favicon-32x32.png new file mode 100644 index 00000000000..951e6b7eb33 Binary files /dev/null and b/tests/lib/connectors/fixtures/common/favicon-32x32.png differ diff --git a/tests/lib/connectors/fixtures/common/favicon.ico b/tests/lib/connectors/fixtures/common/favicon.ico new file mode 100644 index 00000000000..aa08eac4d8d Binary files /dev/null and b/tests/lib/connectors/fixtures/common/favicon.ico differ