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.

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

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

Merged
merged 6 commits into from
May 19, 2023

Conversation

connorjclark
Copy link
Collaborator

Fixes #14980

image

@connorjclark connorjclark requested a review from a team as a code owner May 8, 2023 23:25
@connorjclark connorjclark requested review from adamraine and removed request for a team May 8, 2023 23:25
@@ -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.

@@ -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

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.


const index = this._executionContextDescriptions.findIndex(d =>
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.

core/gather/driver/target-manager.js Outdated Show resolved Hide resolved
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

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.

core/gather/driver/target-manager.js Outdated Show resolved Hide resolved
core/gather/driver/target-manager.js Outdated Show resolved Hide resolved
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

exciting!

core/gather/driver/target-manager.js Show resolved Hide resolved
core/gather/gatherers/global-listeners.js Outdated Show resolved Hide resolved
core/gather/driver/target-manager.js Outdated Show resolved Hide resolved
@@ -97,6 +103,12 @@ class TargetManager extends ProtocolEventEmitter {
return this._findSession(rootSessionId);
}

mainFrameExecutionContexts() {
return [...this._executionContextIdToDescriptions.values()].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

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.

@connorjclark connorjclark merged commit 8c053ed into main May 19, 2023
@connorjclark connorjclark deleted the target-manager-execs branch May 19, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extension unload listeners can fail the bfcache audit but be otherwise undetectable
4 participants