From f93550fb9576b3c78c41c3b20391b01d67a3f818 Mon Sep 17 00:00:00 2001 From: qingzhou Date: Wed, 16 Aug 2017 15:31:23 -0700 Subject: [PATCH] Fix: Avoid analyzing `/favicon.ico` multiple times Fix #427 --- .../shared/remote-debugging-connector.ts | 24 +++- tests/lib/connectors/collect.ts | 116 ++++++++++++++++++ .../fixtures/common/favicon-32x32.png | Bin 0 -> 719 bytes .../connectors/fixtures/common/favicon.ico | Bin 0 -> 5430 bytes 4 files changed, 136 insertions(+), 4 deletions(-) create mode 100644 tests/lib/connectors/collect.ts create mode 100644 tests/lib/connectors/fixtures/common/favicon-32x32.png create mode 100644 tests/lib/connectors/fixtures/common/favicon.ico diff --git a/src/lib/connectors/shared/remote-debugging-connector.ts b/src/lib/connectors/shared/remote-debugging-connector.ts index 994c6bcef25..be0eb9f28e9 100644 --- a/src/lib/connectors/shared/remote-debugging-connector.ts +++ b/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 `` 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); diff --git a/tests/lib/connectors/collect.ts b/tests/lib/connectors/collect.ts new file mode 100644 index 00000000000..9bb5bb5b513 --- /dev/null +++ b/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(``), + '/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(``), + '/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); +}); 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 0000000000000000000000000000000000000000..951e6b7eb338d6426d947337cdb51456baf69e21 GIT binary patch literal 719 zcmV;=0xQDWab^E&G zpBq=~!Q3vl`I|PT>0g)6*`rj-_qaf@439F|tlu!e)rR_XDI#6?>WMZ>ien=1D%BkfHF7^ zfycdCy~rcIas8#Y`9GukzADB*ry&BU4TfWV-*zv%y?*HmVbWXM*DH~4C?f&#X`e{wba(ayOCnYIX!<#wZZls^7{o%1^~>b{ zOw_X{&cC=HPJ2Ak{vGN+1Sr5DS0M~2!YueO1zC;$;=xLoi`kX)J?=DTq9p^1-*LRsC!aaIh(O z<~WKhnD68R}2{ska6}BuWH}%EJHv002ovPDHLkV1j<0 BVV(d0 literal 0 HcmV?d00001 diff --git a/tests/lib/connectors/fixtures/common/favicon.ico b/tests/lib/connectors/fixtures/common/favicon.ico new file mode 100644 index 0000000000000000000000000000000000000000..aa08eac4d8dc24acff2e260a8b79e43f3c9a391d GIT binary patch literal 5430 zcmeHLe`s4(6u!ogl~K|%LWo1QAW}-%AN@m0(SfDZp`*iEL0VDl?5tx-DP`!y;zC

mCRSX`&^p;pGh-!;F{g8B?1uaxWTlXy3ymc?8r`3wC~UPs zPKqWm7_tC`DJYOd(PR{>6j`wF?vcvy8}Gj7tEfE*OaWdMBho)zIll3qaP1cLht-s{f@rCQ!kF7e+IG&v>_w= z#Y0ER$zHtbv)u4rC>8@jQ}W6z^{v8>KR*oHfPuZh4~=cev=>+p8*I;Ku%8Y*-F#4v z#VwiaW#NWtq35F=u;+Aq^!T4>oJ>srZqH;-a`>=BKRI53wR8dfDq68_Rld#7Oo!U` zcI%x3BcIx{__vex;P&=b?(-9dF=+aFE#_~M_?TJm*`1X!y#GrOiWr+&1KiJ8w+az0 zK?alu{@2xPD9<>7DuD7)rr({N)9TA<_vU~1?1{7xrJP;>SSgnQ4jquZP>S*^@(xQ; z&WxO914^TxenC0C9%u!6fX{Sr0J2@&aZeT509NFaTkf^nojZx+&B*z7@&kW#Z_=F^ z0d(&j9l4;|wDKoTl}ZMAJv|o0YvI2d*#!Ge;IfYIY-7;$Jcyyv=x)(sptVA+K=+(` z1$Z@nYESdSVew%pBU#QJhYvn}J6dOD1 z0Jd@i!@IyUp>oZlJshZgseQiX&0+L|syr8NvY7(EL*u2EYB%2fwH43kjHT~2)m#&y z_RjkF__-4FzXWpSCOZK&T6yXoP)z~(7J8P!cOuyMnuj|&Xb-*K@d9-8YLDf_R7W+e zwg%ug!>22So9eT1;9`nw5;f{3+8@Z$*JwH1q}!mw-vIdl(C@El+(UI@q_g*Vj1Q~w zcfkK3_-UOk0n6n~)xVh^`yV~xUI#dV4R}WiXl+n;nlZMiaz}jP2g?faZ%v)Qwh!5~ z;IF~lr}LQq-_gEfrBvf(!jvi{boA`zXRNKzohQx?fSG{l*hZV zmu30Xw)hsaM7j>u!dIqm2)$U^p-(Z7hIe;$NZ|!*w)54~Lk#O8UA-^E-!Fh3U%HI$ z>z`CzU#QJhjc>fXY6I~qV(9Fpvq*X4G%y2DuGF{%^y%BNm+K!anR~Wn5Pg!-^OHFL zIiCS-O~h+HvoPhg0cF6WKsB&MMGe{tU_ELkOX9-CvbnnE4sg?5wmARlo~Gs00~%*O zZQ{2-Y=Lb}o5BAf@cn%AZik#mo9e zQRL(2p6%W9?XrsZ3Zd!z#5IcIVv5Yz<04}l2|^Vs`mPBmXGJc++s2BN$W7R&2<)gL SvJYUm4_>bWQ62C9iT(xo(IzGU literal 0 HcmV?d00001