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(oopif): attach to all descendants #7608

Merged
merged 6 commits into from
Mar 22, 2019
Merged

core(oopif): attach to all descendants #7608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
39a03f7
core(oopif): attach to all descendants
patrickhulce Mar 20, 2019
3edfccf
still resume service workers
patrickhulce Mar 21, 2019
9bfe385
feedback
patrickhulce Mar 21, 2019
1e4a35c
feedback
patrickhulce Mar 21, 2019
c8ee8aa
more comments
patrickhulce Mar 21, 2019
9ac2e10
lint
patrickhulce Mar 22, 2019
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
2 changes: 1 addition & 1 deletion lighthouse-cli/test/fixtures/oopif.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@
</head>
<body>
<h1>Hello frames</h1>
<iframe name="oopif" src="https://airhorner.com"></iframe>
<iframe name="oopif" src="https://www.paulirish.com/2012/why-moving-elements-with-translate-is-better-than-posabs-topleft/" style="width: 100vw; height: 100vh;"></iframe>
Copy link
Member

Choose a reason for hiding this comment

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

also, should we just be making our own OOPIF here rather than relying on this page having another nested one? We could do a page on the other port to go cross origin (assuming that triggers OOP) and have that embed the youtube video, for instance

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 wish, I tried like 15 different combinations but localhost never seems to be oopif'd on its own, so we need at least one publicly hosted page that then also iframes a google-y origin that will guarantee 2 separate processes beyond our localhost one

</body>
</html>
9 changes: 5 additions & 4 deletions lighthouse-cli/test/smokehouse/oopif-expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ module.exports = [
'network-requests': {
details: {
items: {
// The page itself only makes a few requests.
// We want to make sure we are finding the iframe's requests (airhorner) while being flexible enough
// to allow changes to the live site.
length: '>10',
// We want to make sure we are finding the iframe's requests (paulirish.com) *AND*
// the iframe's iframe's iframe's requests (youtube.com/doubleclick/etc).
// - paulirish.com ~40-60 requests
// - paulirish.com + all descendant iframes ~80-90 requests
length: '>70',
},
},
},
Expand Down
157 changes: 133 additions & 24 deletions lighthouse-core/gather/driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ class Driver {
*/
this._eventEmitter = /** @type {CrdpEventEmitter} */ (new EventEmitter());
this._connection = connection;
// Used to save network and lifecycle protocol traffic. Just Page, Network, and Target are needed.
this._devtoolsLog = new DevtoolsLog(/^(Page|Network|Target)\./);
// Used to save network and lifecycle protocol traffic. Just Page and Network are needed.
this._devtoolsLog = new DevtoolsLog(/^(Page|Network)\./);
this.online = true;
/** @type {Map<string, number>} */
this._domainEnabledCounts = new Map();
Expand All @@ -69,32 +69,25 @@ class Driver {
*/
this._monitoredUrl = null;

let targetProxyMessageId = 0;
/**
* Used for sending messages to subtargets. Each message needs a unique ID even if we don't bother
* reading back the result of each command.
*
* @type {number}
* @private
*/
this._targetProxyMessageId = 0;

this.on('Target.attachedToTarget', event => {
targetProxyMessageId++;
// We're only interested in network requests from iframes for now as those are "part of the page".
if (event.targetInfo.type !== 'iframe') return;

// We want to receive information about network requests from iframes, so enable the Network domain.
// Network events from subtargets will be stringified and sent back on `Target.receivedMessageFromTarget`.
this.sendCommand('Target.sendMessageToTarget', {
message: JSON.stringify({id: targetProxyMessageId, method: 'Network.enable'}),
sessionId: event.sessionId,
});
this._handleTargetAttached(event, []).catch(this._handleEventError);
});

connection.on('protocolevent', event => {
this._devtoolsLog.record(event);
if (this._networkStatusMonitor) {
this._networkStatusMonitor.dispatch(event);
}

// @ts-ignore TODO(bckenny): tsc can't type event.params correctly yet,
// typing as property of union instead of narrowing from union of
// properties. See https://github.com/Microsoft/TypeScript/pull/22348.
this._eventEmitter.emit(event.method, event.params);
this.on('Target.receivedMessageFromTarget', event => {
this._handleReceivedMessageFromTarget(event, []).catch(this._handleEventError);
});

connection.on('protocolevent', this._handleProtocolEvent.bind(this));

/**
* @type {number}
* @private
Expand Down Expand Up @@ -275,6 +268,121 @@ class Driver {
this._nextProtocolTimeout = timeout;
}

/**
* @param {LH.Protocol.RawEventMessage} event
*/
_handleProtocolEvent(event) {
this._devtoolsLog.record(event);
if (this._networkStatusMonitor) {
this._networkStatusMonitor.dispatch(event);
}

// @ts-ignore TODO(bckenny): tsc can't type event.params correctly yet,
// typing as property of union instead of narrowing from union of
// properties. See https://github.com/Microsoft/TypeScript/pull/22348.
this._eventEmitter.emit(event.method, event.params);
}

/**
* @param {Error} error
*/
_handleEventError(error) {
log.error('Driver', 'Unhandled event error', error.message);
}

/**
* @param {LH.Crdp.Target.ReceivedMessageFromTargetEvent} event
* @param {string[]} parentSessionIds The list of session ids of the parents from youngest to oldest.
*/
async _handleReceivedMessageFromTarget(event, parentSessionIds) {
const {sessionId, message} = event;
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
/** @type {LH.Protocol.RawMessage} */
const protocolMessage = JSON.parse(message);

// Message was a response to some command, not an event, so we'll ignore it.
if ('id' in protocolMessage) return;

// We receive messages from the outermost subtarget which wraps the messages from the inner subtargets.
// We are recursively processing them from outside in, so build the list of parentSessionIds accordingly.
const sessionIdPath = [sessionId, ...parentSessionIds];

if (protocolMessage.method === 'Target.receivedMessageFromTarget') {
// Unravel any messages from subtargets by recursively processing
await this._handleReceivedMessageFromTarget(protocolMessage.params, sessionIdPath);
}

if (protocolMessage.method === 'Target.attachedToTarget') {
// Process any attachedToTarget messages from subtargets
await this._handleTargetAttached(protocolMessage.params, sessionIdPath);
}

if (protocolMessage.method.startsWith('Network')) {
Copy link
Member

Choose a reason for hiding this comment

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

emit this one so it's handled as usual instead of handling it here?

Copy link
Collaborator Author

@patrickhulce patrickhulce Mar 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

well our current handling isn't actually on driver events it's off the connection event, and I thought it would be weird to forcibly emit an event from within driver on the connection. but I guess you disagree?

🤞 hopes the answer isn't let's refactor this whole thing now ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

compromise: moved this bit into a function that gets called by both?

Copy link
Member

Choose a reason for hiding this comment

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

well our current handling isn't actually on driver events it's off the connection event

ah, I had forgotten about this. I also remembered we did it the way it is because you can't listen to Network.*, so we needed to listen to protocolevent, which is too bad.

moved this bit into a function that gets called by both?

sounds perfect

this._handleProtocolEvent(protocolMessage);
}
}

/**
* @param {LH.Crdp.Target.AttachedToTargetEvent} event
* @param {string[]} parentSessionIds The list of session ids of the parents from youngest to oldest.
*/
async _handleTargetAttached(event, parentSessionIds) {
const sessionIdPath = [event.sessionId, ...parentSessionIds];

// We're only interested in network requests from iframes for now as those are "part of the page".
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
// If it's not an iframe, just resume it and move on.
if (event.targetInfo.type !== 'iframe') {
// We suspended the target when we auto-attached, so make sure it goes back to being normal.
await this.sendMessageToTarget(sessionIdPath, 'Runtime.runIfWaitingForDebugger');
return;
}

// Events from subtargets will be stringified and sent back on `Target.receivedMessageFromTarget`.
// We want to receive information about network requests from iframes, so enable the Network domain.
await this.sendMessageToTarget(sessionIdPath, 'Network.enable');
// We also want to receive information about subtargets of subtargets, so make sure we autoattach recursively.
await this.sendMessageToTarget(sessionIdPath, 'Target.setAutoAttach', {
autoAttach: true,
// Pause targets on startup so we don't miss anything
waitForDebuggerOnStart: true,
});

// We suspended the target when we auto-attached, so make sure it goes back to being normal.
await this.sendMessageToTarget(sessionIdPath, 'Runtime.runIfWaitingForDebugger');
}

/**
* Send protocol commands to a subtarget, wraps the message in as many `Target.sendMessageToTarget`
* commands as necessary.
*
* @template {keyof LH.CrdpCommands} C
* @param {string[]} sessionIdPath List of session ids to send to, from youngest-oldest/inside-out.
* @param {C} method
* @param {LH.CrdpCommands[C]['paramsType']} params
* @return {Promise<LH.CrdpCommands[C]['returnType']>}
*/
sendMessageToTarget(sessionIdPath, method, ...params) {
this._targetProxyMessageId++;
/** @type {LH.Crdp.Target.SendMessageToTargetRequest} */
let payload = {
sessionId: sessionIdPath[0],
message: JSON.stringify({id: this._targetProxyMessageId, method, params: params[0]}),
};

for (const sessionId of sessionIdPath.slice(1)) {
this._targetProxyMessageId++;
payload = {
sessionId,
message: JSON.stringify({
id: this._targetProxyMessageId,
method: 'Target.sendMessageToTarget',
params: payload,
}),
};
}

return this.sendCommand('Target.sendMessageToTarget', payload);
}

/**
* Call protocol methods, with a timeout.
* To configure the timeout for the next call, use 'setNextProtocolTimeout'.
Expand Down Expand Up @@ -1044,7 +1152,8 @@ class Driver {
// Enable auto-attaching to subtargets so we receive iframe information
await this.sendCommand('Target.setAutoAttach', {
autoAttach: true,
waitForDebuggerOnStart: false,
// Pause targets on startup so we don't miss anything
waitForDebuggerOnStart: true,
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
});

await this.sendCommand('Page.enable');
Expand Down
31 changes: 1 addition & 30 deletions lighthouse-core/lib/network-recorder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,26 +101,13 @@ class NetworkRecorder extends EventEmitter {
return !!(isQUIC && receivedHeaders && record.endTime);
}

/**
* frame root network requests don't always "finish" even when they're done loading data, use responseReceived instead
* @see https://github.com/GoogleChrome/lighthouse/issues/6067#issuecomment-423211201
* @param {LH.Artifacts.NetworkRequest} record
* @return {boolean}
*/
static _isFrameRootRequestAndFinished(record) {
const isFrameRootRequest = record.url === record.documentURL;
const responseReceived = record.responseReceivedTime > 0;
return !!(isFrameRootRequest && responseReceived && record.endTime);
}

/**
* @param {LH.Artifacts.NetworkRequest} record
* @return {boolean}
*/
static isNetworkRecordFinished(record) {
return record.finished ||
NetworkRecorder._isQUICAndFinished(record) ||
NetworkRecorder._isFrameRootRequestAndFinished(record);
NetworkRecorder._isQUICAndFinished(record);
}

/**
Expand Down Expand Up @@ -303,21 +290,6 @@ class NetworkRecorder extends EventEmitter {
request.onResourceChangedPriority(data);
}

/**
* Events from targets other than the main frame are proxied through `Target.receivedMessageFromTarget`.
* Their payloads are JSON-stringified into the `.message` property
* @param {LH.Crdp.Target.ReceivedMessageFromTargetEvent} data
*/
onReceivedMessageFromTarget(data) {
/** @type {LH.Protocol.RawMessage} */
const protocolMessage = JSON.parse(data.message);

// Message was a response to some command, not an event, so we'll ignore it.
if ('id' in protocolMessage) return;
// Message was an event, replay it through our normal dispatch process.
this.dispatch(protocolMessage);
}

/**
* Routes network events to their handlers, so we can construct networkRecords
* @param {LH.Protocol.RawEventMessage} event
Expand All @@ -331,7 +303,6 @@ class NetworkRecorder extends EventEmitter {
case 'Network.loadingFinished': return this.onLoadingFinished(event.params);
case 'Network.loadingFailed': return this.onLoadingFailed(event.params);
case 'Network.resourceChangedPriority': return this.onResourceChangedPriority(event.params);
case 'Target.receivedMessageFromTarget': return this.onReceivedMessageFromTarget(event.params); // eslint-disable-line max-len
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
default: return;
}
}
Expand Down
67 changes: 63 additions & 4 deletions lighthouse-core/test/gather/driver-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1000,6 +1000,8 @@ describe('.goOnline', () => {
describe('Multi-target management', () => {
it('enables the Network domain for iframes', async () => {
connectionStub.sendCommand = createMockSendCommandFn()
.mockResponse('Target.sendMessageToTarget', {})
.mockResponse('Target.sendMessageToTarget', {})
.mockResponse('Target.sendMessageToTarget', {});

driver._eventEmitter.emit('Target.attachedToTarget', {
Expand All @@ -1016,16 +1018,73 @@ describe('Multi-target management', () => {
});
});

it('ignores other target types', async () => {
it('enables the Network domain for iframes of iframes of iframes', async () => {
connectionStub.sendCommand = createMockSendCommandFn()
.mockResponse('Target.sendMessageToTarget', {});
.mockResponse('Target.sendMessageToTarget', {})
.mockResponse('Target.sendMessageToTarget', {})
.mockResponse('Target.sendMessageToTarget', {});

driver._eventEmitter.emit('Target.receivedMessageFromTarget', {
sessionId: 'Outer',
message: JSON.stringify({
method: 'Target.receivedMessageFromTarget',
params: {
sessionId: 'Middle',
message: JSON.stringify({
method: 'Target.attachedToTarget',
params: {
sessionId: 'Inner',
targetInfo: {type: 'iframe'},
},
}),
},
}),
});

await flushAllTimersAndMicrotasks();

const sendMessageArgs = connectionStub.sendCommand
.findInvocation('Target.sendMessageToTarget');
const stringified = `{
"id": 3,
"method": "Target.sendMessageToTarget",
"params": {
"sessionId": "Middle",
"message": "{
\\"id\\": 2,
\\"method\\": \\"Target.sendMessageToTarget\\",
\\"params\\": {
\\"sessionId\\": \\"Inner\\",
\\"message\\":\\ "{
\\\\\\"id\\\\\\":1,
\\\\\\"method\\\\\\":\\\\\\"Network.enable\\\\\\"
}\\"
}}"
}
}`.replace(/\s+/g, '');

expect(sendMessageArgs).toEqual({
message: stringified,
sessionId: 'Outer',
});
});

it('ignores other target types, but still resumes them', async () => {
connectionStub.sendCommand = createMockSendCommandFn()
.mockResponse('Target.sendMessageToTarget', {});

driver._eventEmitter.emit('Target.attachedToTarget', {
sessionId: 123,
sessionId: 'SW1',
targetInfo: {type: 'service_worker'},
});
await flushAllTimersAndMicrotasks();

expect(connectionStub.sendCommand).not.toHaveBeenCalled();

const sendMessageArgs = connectionStub.sendCommand
.findInvocation('Target.sendMessageToTarget');
expect(sendMessageArgs).toEqual({
message: JSON.stringify({id: 1, method: 'Runtime.runIfWaitingForDebugger'}),
sessionId: 'SW1',
});
});
});
4 changes: 1 addition & 3 deletions lighthouse-core/test/lib/network-recorder-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,7 @@ describe('network recorder', function() {
];

const periods = NetworkRecorder.findNetworkQuietPeriods(records, 0);
assert.deepStrictEqual(periods, [
{start: 1200, end: Infinity},
]);
assert.deepStrictEqual(periods, []);
});

it('should handle QUIC requests', () => {
Expand Down