Skip to content

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

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.

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
Contributor

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.

Copy link
Contributor

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


mainFrameExecutionContexts() {
return [...this._executionContextIdToDescriptions.values()].filter(executionContext => {
return executionContext.auxData.frameId === this._mainFrameId;
Copy link
Contributor

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

5 participants