[go: nahoru, domu]
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 1 commit
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
Prev Previous commit
Next Next commit
filter based on main frame
  • Loading branch information
@connorjclark
connorjclark committed May 9, 2023
commit 9b4c35d5c6aaa6ac0d18898f40dfbea8e7db28ce
12 changes: 12 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,6 +40,7 @@

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

/**
* A map of target id to target/session information. Used to ensure unique
Expand Down Expand Up @@ -103,13 +104,20 @@
}

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

Check warning on line 108 in core/gather/driver/target-manager.js

View check run for this annotation

Codecov / codecov/patch

core/gather/driver/target-manager.js#L107-L108 

Added lines #L107 - L108 were not covered by tests

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 @@ -124,6 +132,7 @@
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 @@ -166,6 +175,9 @@
* @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;

Expand All @@ -180,12 +192,12 @@
* @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);
}
}

Check warning on line 200 in core/gather/driver/target-manager.js

View check run for this annotation

Codecov / codecov/patch

core/gather/driver/target-manager.js#L195-L200 

Added lines #L195 - L200 were not covered by tests

_onExecutionContextsCleared() {
this._executionContextDescriptions = [];
Expand Down
2 changes: 1 addition & 1 deletion core/gather/gatherers/global-listeners.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
/** @type {Array<LH.Artifacts.GlobalListener>} */
const listeners = [];

for (const executionContext of passContext.driver.targetManager.executionContexts()) {
for (const executionContext of passContext.driver.targetManager.mainFrameExecutionContexts()) {
// Get a RemoteObject handle to `window`.
let objectId;
try {
Expand All @@ -74,14 +74,14 @@
uniqueContextId: executionContext.uniqueId,
});
if (!result.objectId) {
throw new Error('Error fetching information about the global object');
}

Check warning on line 78 in core/gather/gatherers/global-listeners.js

View check run for this annotation

Codecov / codecov/patch

core/gather/gatherers/global-listeners.js#L77-L78 

Added lines #L77 - L78 were not covered by tests
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;
}

Check warning on line 84 in core/gather/gatherers/global-listeners.js

View check run for this annotation

Codecov / codecov/patch

core/gather/gatherers/global-listeners.js#L81-L84 

Added lines #L81 - L84 were not covered by tests

// And get all its listeners of interest.
const response = await session.sendCommand('DOMDebugger.getEventListeners', {objectId});
Expand Down
4 changes: 4 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 @@ -109,8 +109,12 @@ class Driver {
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
1 change: 1 addition & 0 deletions types/gatherer.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ declare module Gatherer {
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
Loading