Skip to content
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.

Sign up for GitHub

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

Jump to bottom

new_audit: add no-unload-listeners audit #11085

Merged
merged 7 commits into from
Jul 21, 2020
Merged

new_audit: add no-unload-listeners audit #11085

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c29c7d3
new_audit: add no-unload-listeners audit
brendankenny Jul 10, 2020
73738de
strings and smoke test
brendankenny Jul 10, 2020
784c6bd
switch to source-location details
brendankenny Jul 10, 2020
1fcaf1d
GlobalListeners
brendankenny Jul 10, 2020
ff2efc3
fixes
brendankenny Jul 10, 2020
4e71cb3
add no-unload unit tests
brendankenny Jul 10, 2020
e5685e1
feedback
brendankenny Jul 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Prev Previous commit
Next Next commit
GlobalListeners
  • Loading branch information
@brendankenny
brendankenny committed Jul 21, 2020
commit 1fcaf1dd987b6545aa478a538e7263683d385e14
6 changes: 3 additions & 3 deletions lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1331,6 +1331,9 @@ Object {
Object {
"path": "main-document-content",
},
Object {
"path": "global-listeners",
},
Object {
"path": "dobetterweb/appcache",
},
Expand Down Expand Up @@ -1376,9 +1379,6 @@ Object {
Object {
"path": "source-maps",
},
Object {
"path": "unload-listeners",
},
],
"loadFailureMode": "fatal",
"networkQuietThresholdMs": 1000,
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-cli/test/smokehouse/test-definitions/dobetterweb/dbw-expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ const expectations = [
},
},
],
UnloadListeners: [{
GlobalListeners: [{
type: 'unload',
scriptId: /^\d+$/,
lineNumber: '>300',
Expand Down
16 changes: 9 additions & 7 deletions lighthouse-core/audits/no-unload-listeners.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class NoUnloadListeners extends Audit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['UnloadListeners', 'JsUsage'],
requiredArtifacts: ['GlobalListeners', 'JsUsage'],
};
}

Expand All @@ -38,7 +38,7 @@ class NoUnloadListeners extends Audit {
* @return {LH.Audit.Product}
*/
static audit(artifacts) {
const unloadListeners = artifacts.UnloadListeners.filter(l => l.type === 'unload');
const unloadListeners = artifacts.GlobalListeners.filter(l => l.type === 'unload');
if (!unloadListeners.length) {
return {
score: 1,
Expand All @@ -50,14 +50,16 @@ class NoUnloadListeners extends Audit {
{key: 'source', itemType: 'source-location', text: str_(i18n.UIStrings.columnURL)},
];

const jsUsageValues = Object.values(artifacts.JsUsage)
.reduce((acc, usage) => acc.concat(usage), []); // single-level arr.flat().
// Look up scriptId to script URL via the JsUsage artifact.
/** @type {Array<[string, string]>} */
const scriptIdToUrlEntries = Object.values(artifacts.JsUsage)
.reduce((acc, usage) => acc.concat(usage), []) // single-level arr.flat().
.map(usage => [usage.scriptId, usage.url]);
const scriptIdToUrl = new Map(scriptIdToUrlEntries);

/** @type {Array<{source: LH.Audit.Details.SourceLocationValue}>} */
const tableItems = unloadListeners.map(listener => {
// Look up scriptId to script URL via the JsUsage artifact.
const usageEntry = jsUsageValues.find(usage => usage.scriptId === listener.scriptId);
const url = usageEntry && usageEntry.url || '(unknown)';
const url = scriptIdToUrl.get(listener.scriptId) || '(unknown)';

// line: `line: ${listener.lineNumber}`,
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
return {
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/config/default-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ const defaultConfig = {
'script-elements',
'iframe-elements',
'main-document-content',
'global-listeners',
'dobetterweb/appcache',
'dobetterweb/doctype',
'dobetterweb/domstats',
Expand All @@ -162,7 +163,6 @@ const defaultConfig = {
'trace-elements',
'inspector-issues',
'source-maps',
'unload-listeners',
],
},
{
Expand Down
17 changes: 12 additions & 5 deletions ...core/gather/gatherers/unload-listeners.js → ...core/gather/gatherers/global-listeners.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,16 @@
*/
'use strict';

/**
* @fileoverview
* A gatherer to collect information about the event listeners registered on the
* global object. For now, the scope is narrowed to events that occur on and
* around page unload, but this can be expanded in the future.
*/

const Gatherer = require('./gatherer.js');

class UnloadListeners extends Gatherer {
class GlobalListeners extends Gatherer {
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
/**
* @param {LH.Crdp.DOMDebugger.EventListener} listener
* @return {listener is {type: 'pagehide'|'unload'|'visibilitychange'} & LH.Crdp.DOMDebugger.EventListener}
Expand All @@ -20,7 +27,7 @@ class UnloadListeners extends Gatherer {

/**
* @param {LH.Gatherer.PassContext} passContext
* @return {Promise<LH.Artifacts['UnloadListeners']>}
* @return {Promise<LH.Artifacts['GlobalListeners']>}
*/
async afterPass(passContext) {
const driver = passContext.driver;
Expand All @@ -34,10 +41,10 @@ class UnloadListeners extends Gatherer {
throw new Error('Error fetching information about the global object');
}

// And get all its unload-ish listeners.
// And get all its listeners of interest.
const {listeners} = await driver.sendCommand('DOMDebugger.getEventListeners', {objectId});
return listeners
.filter(UnloadListeners._filterForUnloadTypes)
.filter(GlobalListeners._filterForUnloadTypes)
.map(listener => {
const {type, scriptId, lineNumber, columnNumber} = listener;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just noticing this is already back in node-land so serialization isn't the reason for reexposing, just trying to limit what we return?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just trying to limit what we return?

yeah

return {
Expand All @@ -50,4 +57,4 @@ class UnloadListeners extends Gatherer {
}
}

module.exports = UnloadListeners;
module.exports = GlobalListeners;
2 changes: 1 addition & 1 deletion lighthouse-core/test/results/artifacts/artifacts.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2410,7 +2410,7 @@
"blockedByResponse": [],
"mixedContent": []
},
"UnloadListeners": [
"GlobalListeners": [
{
"type": "unload",
"scriptId": "23",
Expand Down
14 changes: 7 additions & 7 deletions types/artifacts.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,14 @@ declare global {
FontSize: Artifacts.FontSize;
/** Screenshot of the entire page (rather than just the above the fold content). */
FullPageScreenshot: Artifacts.FullPageScreenshot | null;
/** The issues surfaced in the devtools Issues panel */
InspectorIssues: Artifacts.InspectorIssues;
/** Information about event listeners registered on the global object. */
GlobalListeners: Array<Artifacts.GlobalListener>;
/** The page's document body innerText if loaded with JavaScript disabled. */
HTMLWithoutJavaScript: {bodyText: string, hasNoScript: boolean};
/** Whether the page ended up on an HTTPS page after attempting to load the HTTP version. */
HTTPRedirect: {value: boolean};
/** The issues surfaced in the devtools Issues panel */
InspectorIssues: Artifacts.InspectorIssues;
/** JS coverage information for code used during page load. Keyed by URL. */
JsUsage: Record<string, Crdp.Profiler.ScriptCoverage[]>;
/** Parsed version of the page's Web App Manifest, or null if none found. */
Expand Down Expand Up @@ -146,8 +148,6 @@ declare global {
TapTargets: Artifacts.TapTarget[];
/** Elements associated with metrics (ie: Largest Contentful Paint element). */
TraceElements: Artifacts.TraceElement[];
/** Information about the event handlers that were registered to listen for when the page is unloaded. */
UnloadListeners: Array<Artifacts.UnloadListener>;
}

module Artifacts {
Expand Down Expand Up @@ -698,9 +698,9 @@ declare global {
observedSpeedIndexTs: number;
}

/** Information on a listener for an event that happens on or around page unload. */
export interface UnloadListener {
/** Event listener type. */
/** Information about an event listener registered on the global object. */
export interface GlobalListener {
/** Event listener type, limited to those events currently of interest. */
type: 'pagehide'|'unload'|'visibilitychange';
/** The DevTools protocol script identifier. */
scriptId: string;
Expand Down