-
Notifications
You must be signed in to change notification settings - Fork 671
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Avoid analyzing /favicon.ico
multiple times
#430
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 `<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 && element.getAttribute('href')) || '/favicon.ico'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as before |
||
|
||
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); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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(`<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.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(`<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.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(`<link rel="icon" type="image/png" href="" sizes="32x32">`), | ||
'/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); | ||
}); |
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file not shown.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to do the assignment in this way instead of the old way?
In terms of readability I prefer the old way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sarvaje This is due to the scenario when
element
exists buthref
is an empty string. We still want to send out a request to/favicon.ico
in this case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh, ok!