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

core(global-listeners): iterate all execution contexts #15054

Merged
merged 6 commits into from
May 19, 2023
Merged

core(global-listeners): iterate all execution contexts #15054

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d4fe038
core(global-listeners): iterate all execution contexts
connorjclark May 8, 2023
84afe30
types
connorjclark May 8, 2023
9b4c35d
filter based on main frame
connorjclark May 9, 2023
3a9be89
add test for timespan
connorjclark May 9, 2023
078123a
map time map time
connorjclark May 9, 2023
ed67e34
pr
connorjclark May 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions core/gather/driver/target-manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,16 +40,22 @@ class TargetManager extends ProtocolEventEmitter {

this._enabled = false;
this._rootCdpSession = cdpSession;
this._mainFrameId = '';

/**
* A map of target id to target/session information. Used to ensure unique
* attached targets.
* @type {Map<string, TargetWithSession>}
*/
this._targetIdToTargets = new Map();
/** @type {Array<LH.Crdp.Runtime.ExecutionContextDescription>} */
this._executionContextDescriptions = [];

this._onSessionAttached = this._onSessionAttached.bind(this);
this._onFrameNavigated = this._onFrameNavigated.bind(this);
this._onExecutionContextCreated = this._onExecutionContextCreated.bind(this);
this._onExecutionContextDestroyed = this._onExecutionContextDestroyed.bind(this);
this._onExecutionContextsCleared = this._onExecutionContextsCleared.bind(this);
}

/**
Expand Down Expand Up @@ -97,10 +103,21 @@ class TargetManager extends ProtocolEventEmitter {
return this._findSession(rootSessionId);
}

executionContexts() {
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
return [...this._executionContextDescriptions];
}

mainFrameExecutionContexts() {
return this._executionContextDescriptions.filter(executionContext => {
return executionContext.auxData.frameId === this._mainFrameId;
Copy link
Member

Choose a reason for hiding this comment

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

do we need this since we only listen on the root session?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, just adding for later in case that ever changes. Could be easy to overlook.

Copy link
Collaborator Author

@connorjclark connorjclark May 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

er, wait no this is important now. It filters out a number of execution contexts for this simple page:

Who Framed A11y Tester?!
<iframe src="a11y_tester.html" width="100%" height="100%"></iframe>

coming from the iframe. comment out the iframe, and only the first execution context is present

{
  id: 7,
  origin: 'http://localhost:10503',
  name: '',
  uniqueId: '5889311435627745206.-699647868331920856',
  auxData: {
    isDefault: true,
    type: 'default',
    frameId: 'E3DEC3BA2977996FAA61E3E4418D4122'
  }
}
{
  id: 9,
  origin: 'http://localhost:10503',
  name: '',
  uniqueId: '-9077244664686306541.-7829375488897728831',
  auxData: {
    isDefault: true,
    type: 'default',
    frameId: '48AA5C7F280A6810D012BAD09CB99732'
  }
}
{
  id: 11,
  origin: 'http://localhost:10503',
  name: '',
  uniqueId: '2049821010959430554.-6542202802565848863',
  auxData: {
    isDefault: true,
    type: 'default',
    frameId: '9548FD706B2909A5E58864CB898DDF7D'
  }
}
{
  id: 13,
  origin: '://',
  name: '',
  uniqueId: '7110504061362463461.-3201941903195113152',
  auxData: {
    isDefault: true,
    type: 'default',
    frameId: 'E1DD688E090D4775B24C805636F001C2'
  }
}

9 is the iframe in the main document, 11 is an inframe inside a11y_tester.html. not sure what 13 could be, perhaps an extenstion.

});
}

/**
* @param {LH.Puppeteer.CDPSession} cdpSession
*/
async _onSessionAttached(cdpSession) {
const isRootSession = this._rootCdpSession === cdpSession;
const newSession = new ProtocolSession(cdpSession);

try {
Expand All @@ -115,6 +132,7 @@ class TargetManager extends ProtocolEventEmitter {
const targetId = target.targetInfo.targetId;
if (this._targetIdToTargets.has(targetId)) return;

if (isRootSession) this._mainFrameId = targetId;
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
newSession.setTargetInfo(target.targetInfo);
const targetName = target.targetInfo.url || target.targetInfo.targetId;
log.verbose('target-manager', `target ${targetName} attached`);
Expand Down Expand Up @@ -153,6 +171,38 @@ class TargetManager extends ProtocolEventEmitter {
}
}

/**
* @param {LH.Crdp.Runtime.ExecutionContextCreatedEvent} event
*/
_onExecutionContextCreated(event) {
// This execution context was made via the protocol (e.g. by us or Puppeteer).
if (event.context.origin === '://' || event.context.origin === '') return;
// Just in case the above changes somehow.
if (event.context.name === '__puppeteer_utility_world__') return;
if (event.context.name === 'lighthouse_isolated_context') return;
connorjclark marked this conversation as resolved.
Show resolved Hide resolved

const index = this._executionContextDescriptions.findIndex(d =>
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
d.uniqueId === event.context.uniqueId);
if (index === -1) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure why, but puppeteer is sending a context created event twice for every single one. So I had to do a check on uniqueId here.

this._executionContextDescriptions.push(event.context);
}
}

/**
* @param {LH.Crdp.Runtime.ExecutionContextDestroyedEvent} event
*/
_onExecutionContextDestroyed(event) {
const index = this._executionContextDescriptions.findIndex(d =>
d.uniqueId === event.executionContextUniqueId);
if (index !== -1) {
this._executionContextDescriptions.splice(index, 1);
}
}

_onExecutionContextsCleared() {
this._executionContextDescriptions = [];
}

/**
* Returns a listener for all protocol events from session, and augments the
* event with the sessionId.
Expand Down Expand Up @@ -185,8 +235,12 @@ class TargetManager extends ProtocolEventEmitter {
this._targetIdToTargets = new Map();

this._rootCdpSession.on('Page.frameNavigated', this._onFrameNavigated);
this._rootCdpSession.on('Runtime.executionContextCreated', this._onExecutionContextCreated);
this._rootCdpSession.on('Runtime.executionContextDestroyed', this._onExecutionContextDestroyed);
this._rootCdpSession.on('Runtime.executionContextsCleared', this._onExecutionContextsCleared);

await this._rootCdpSession.send('Page.enable');
await this._rootCdpSession.send('Runtime.enable');
connorjclark marked this conversation as resolved.
Show resolved Hide resolved

// Start with the already attached root session.
await this._onSessionAttached(this._rootCdpSession);
Expand All @@ -197,14 +251,22 @@ class TargetManager extends ProtocolEventEmitter {
*/
async disable() {
this._rootCdpSession.off('Page.frameNavigated', this._onFrameNavigated);
this._rootCdpSession.off('Runtime.executionContextCreated', this._onExecutionContextCreated);
this._rootCdpSession.off('Runtime.executionContextDestroyed',
this._onExecutionContextDestroyed);
this._rootCdpSession.off('Runtime.executionContextsCleared', this._onExecutionContextsCleared);

for (const {cdpSession, protocolListener} of this._targetIdToTargets.values()) {
cdpSession.off('*', protocolListener);
cdpSession.off('sessionattached', this._onSessionAttached);
}

await this._rootCdpSession.send('Page.disable');
await this._rootCdpSession.send('Runtime.disable');

this._enabled = false;
this._targetIdToTargets = new Map();
this._executionContextDescriptions = [];
}
}

Expand Down
57 changes: 36 additions & 21 deletions core/gather/gatherers/global-listeners.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,30 +61,45 @@ class GlobalListeners extends FRGatherer {
async getArtifact(passContext) {
const session = passContext.driver.defaultSession;

// Get a RemoteObject handle to `window`.
const {result: {objectId}} = await session.sendCommand('Runtime.evaluate', {
expression: 'window',
returnByValue: false,
});
if (!objectId) {
throw new Error('Error fetching information about the global object');
}
/** @type {Array<LH.Artifacts.GlobalListener>} */
const listeners = [];

// And get all its listeners of interest.
const {listeners} = await session.sendCommand('DOMDebugger.getEventListeners', {objectId});
const filteredListeners = listeners.filter(GlobalListeners._filterForAllowlistedTypes)
.map(listener => {
const {type, scriptId, lineNumber, columnNumber} = listener;
return {
type,
scriptId,
lineNumber,
columnNumber,
};
});
for (const executionContext of passContext.driver.targetManager.mainFrameExecutionContexts()) {
// Get a RemoteObject handle to `window`.
let objectId;
try {
const {result} = await session.sendCommand('Runtime.evaluate', {
expression: 'window',
returnByValue: false,
uniqueContextId: executionContext.uniqueId,
});
if (!result.objectId) {
throw new Error('Error fetching information about the global object');
}
objectId = result.objectId;
} catch (err) {
// Execution context is no longer valid, but don't let that fail the gatherer.
console.error(err);
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
continue;
}

// And get all its listeners of interest.
const response = await session.sendCommand('DOMDebugger.getEventListeners', {objectId});
for (const listener of response.listeners) {
if (GlobalListeners._filterForAllowlistedTypes(listener)) {
const {type, scriptId, lineNumber, columnNumber} = listener;
listeners.push({
type,
scriptId,
lineNumber,
columnNumber,
});
}
}
}

// Dedupe listeners with same underlying data.
return this.dedupeListeners(filteredListeners);
return this.dedupeListeners(listeners);
}
}

Expand Down
14 changes: 14 additions & 0 deletions core/legacy/gather/driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,20 @@ class Driver {
rootSession: () => {
return this.defaultSession;
},
// For legacy driver, only bother supporting access to the default execution context.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seems like a reasonable time to stop trying for parity wrt legacy driver. So...just giving the default EC here.

executionContexts: () => {
// @ts-expect-error - undefined ids are OK for purposes of calling protocol commands like Runtime.evaluate.
return [/** @type {LH.Crdp.Runtime.ExecutionContextDescription} */({
id: undefined,
uniqueId: undefined,
origin: '',
name: '',
auxData: {isDefault: true, type: 'default', frameId: ''},
})];
},
mainFrameExecutionContexts: () => {
return this.targetManager.executionContexts();
},
/**
* Bind to *any* protocol event.
* @param {'protocolevent'} event
Expand Down
3 changes: 3 additions & 0 deletions core/test/gather/driver-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ beforeEach(() => {
const puppeteerSession = createMockCdpSession();
puppeteerSession.send
.mockResponse('Page.enable')
.mockResponse('Runtime.enable')
.mockResponse('Page.disable')
.mockResponse('Runtime.disable')
.mockResponse('Target.getTargetInfo', {targetInfo: {type: 'page', targetId: 'page'}})
.mockResponse('Network.enable')
.mockResponse('Target.setAutoAttach')
Expand Down
1 change: 1 addition & 0 deletions core/test/gather/driver/network-monitor-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ describe('NetworkMonitor', () => {
const cdpSessionMock = createMockCdpSession(id);
cdpSessionMock.send
.mockResponse('Page.enable')
.mockResponse('Runtime.enable')
.mockResponse('Target.getTargetInfo', {targetInfo: {type: targetType, targetId: id}})
.mockResponse('Network.enable')
.mockResponse('Target.setAutoAttach')
Expand Down
7 changes: 6 additions & 1 deletion core/test/gather/driver/target-manager-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ describe('TargetManager', () => {
sendMock = sessionMock.send;
sendMock
.mockResponse('Page.enable')
.mockResponse('Runtime.enable')
.mockResponse('Page.disable')
.mockResponse('Runtime.disable')
.mockResponse('Runtime.runIfWaitingForDebugger');
targetManager = new TargetManager(sessionMock.asCdpSession());
targetInfo = createTargetInfo();
Expand Down Expand Up @@ -78,7 +81,7 @@ describe('TargetManager', () => {
await targetManager.enable();

expect(sessionMock.on).toHaveBeenCalled();
const sessionListener = sessionMock.on.mock.calls[3][1];
const sessionListener = sessionMock.on.mock.calls.find(c => c[0] === 'sessionattached')[1];

// Original, attach.
expect(sendMock.findAllInvocations('Target.getTargetInfo')).toHaveLength(1);
Expand Down Expand Up @@ -258,6 +261,7 @@ describe('TargetManager', () => {
// Still mock command responses at session level.
rootSession.send = createMockSendCommandFn({useSessionId: false})
.mockResponse('Page.enable')
.mockResponse('Runtime.enable')
.mockResponse('Target.getTargetInfo', {targetInfo: rootTargetInfo})
.mockResponse('Network.enable')
.mockResponse('Target.setAutoAttach')
Expand Down Expand Up @@ -327,6 +331,7 @@ describe('TargetManager', () => {
// Still mock command responses at session level.
rootSession.send = createMockSendCommandFn({useSessionId: false})
.mockResponse('Page.enable')
.mockResponse('Runtime.enable')
.mockResponse('Target.getTargetInfo', {targetInfo})
.mockResponse('Network.enable')
.mockResponse('Target.setAutoAttach')
Expand Down
14 changes: 14 additions & 0 deletions core/test/scenarios/api-test-pptr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import jestMock from 'jest-mock';
import * as api from '../../index.js';
import {createTestState, getAuditsBreakdown} from './pptr-test-utils.js';
import {LH_ROOT} from '../../../root.js';
import {TargetManager} from '../../gather/driver/target-manager.js';

describe('Fraggle Rock API', function() {
// eslint-disable-next-line no-invalid-this
Expand Down Expand Up @@ -147,6 +148,7 @@ describe('Fraggle Rock API', function() {

// eslint-disable-next-line max-len
it('should know target type of network requests from frames created before timespan', async () => {
const spy = jestMock.spyOn(TargetManager.prototype, '_onExecutionContextCreated');
state.server.baseDir = `${LH_ROOT}/cli/test/fixtures`;
const {page, serverBaseUrl} = state;

Expand Down Expand Up @@ -192,6 +194,18 @@ Array [
},
]
`);

// Check that TargetManager is getting execution context created events even if connecting
// to the page after they already exist.
// There are two execution contexts, one for the main frame and one for the iframe of
// the same origin.
const contextCreatedMainFrameCalls =
spy.mock.calls.filter(call => call[0].context.origin === 'http://localhost:10200');
// For some reason, puppeteer gives us two created events for every uniqueId,
// so using Set here to ignore that detail.
Copy link
Member

Choose a reason for hiding this comment

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

I did some poking around. I think it's because we Runtime.disable in the stopInstrumentation phase console messages gatherer

lighthouse/core/gather/gatherers/console-messages.js

Line 163 in 3ba11a8

await driver.defaultSession.sendCommand('Runtime.disable');

Presumably Runtime.enable is called sometime in the getArtifact phase which re-emits the events.

Regardless, de-duping on the uniqueId is probably the safest way to handle this.

expect(new Set(contextCreatedMainFrameCalls.map(call => call[0].context.uniqueId)).size)
.toEqual(2);
spy.mockRestore();
});
});

Expand Down
2 changes: 2 additions & 0 deletions types/gatherer.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ declare module Gatherer {
url: () => Promise<string>;
targetManager: {
rootSession(): FRProtocolSession;
executionContexts(): Array<Crdp.Runtime.ExecutionContextDescription>;
mainFrameExecutionContexts(): Array<Crdp.Runtime.ExecutionContextDescription>;
Copy link
Collaborator Author

@connorjclark connorjclark May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Although we only want to process main frames in global listeners, I don't want that to happen via a method called just executionContexts because that locks in the contract that this is only main frames (which could mess with potential plugins if we ever need to expand that). Having the gatherer provide the main frame id is not possible for snapshots, so this logic is kept in target manager. Hence, an explicit mainFrameExecutionContexts... and adding executionContexts just-cuz.

on(event: 'protocolevent', callback: (payload: Protocol.RawEventMessage) => void): void
off(event: 'protocolevent', callback: (payload: Protocol.RawEventMessage) => void): void
};
Expand Down