core(global-listeners): iterate all execution contexts #15054
Conversation
cd56f41
to
d4fe038
Compare
| @@ -101,6 +101,20 @@ class Driver { | |||
| rootSession: () => { | |||
| return this.defaultSession; | |||
| }, | |||
| // For legacy driver, only bother supporting access to the default execution context. | |||
There was a problem hiding this comment.
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>; | |||
There was a problem hiding this comment.
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.
58cb298
to
9b4c35d
Compare
core/gather/driver/target-manager.js
Outdated
|
|
||
| const index = this._executionContextDescriptions.findIndex(d => | ||
| d.uniqueId === event.context.uniqueId); | ||
| if (index === -1) { |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
I did some poking around. I think it's because we Runtime.disable in the stopInstrumentation phase console messages gatherer
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.
| @@ -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; | |||
There was a problem hiding this comment.
do we need this since we only listen on the root session?
There was a problem hiding this comment.
Nope, just adding for later in case that ever changes. Could be easy to overlook.
There was a problem hiding this comment.
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.
Fixes #14980