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): collect network requests from iframes #6922

Merged
merged 6 commits into from
Mar 2, 2019
Merged

core(oopif): collect network requests from iframes #6922

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
35de9e9
core(oopif): collect network requests from iframes
patrickhulce Jan 3, 2019
d6e8aea
comment tweaks
patrickhulce Jan 3, 2019
52e1640
Merge branch 'master' into oopif
patrickhulce Mar 1, 2019
1734ee0
only listen for iframes
patrickhulce Mar 1, 2019
f546378
driver tests
patrickhulce Mar 1, 2019
3a10248
feedback
patrickhulce Mar 2, 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
10 changes: 10 additions & 0 deletions lighthouse-cli/test/fixtures/oopif.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<!DOCTYPE html>
<html>
<head>
<title>Where is my iframe?</title>
</head>
<body>
<h1>Hello frames</h1>
<iframe name="oopif" src="https://airhorner.com"></iframe>
</body>
</html>
18 changes: 18 additions & 0 deletions lighthouse-cli/test/smokehouse/oopif-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* @license Copyright 2019 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

/**
* Config file for running the OOPIF tests
*/
module.exports = {
extends: 'lighthouse:default',
settings: {
onlyAudits: [
'network-requests',
],
},
};
28 changes: 28 additions & 0 deletions lighthouse-cli/test/smokehouse/oopif-expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* @license Copyright 2019 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

/**
* Expected Lighthouse audit values for sites with OOPIFS.
*/
module.exports = [
{
requestedUrl: 'http://localhost:10200/oopif.html',
finalUrl: 'http://localhost:10200/oopif.html',
audits: {
'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',
Copy link
Member

Choose a reason for hiding this comment

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

maybe add a comment why this is a good bounds

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

},
},
},
},
},
];
5 changes: 5 additions & 0 deletions lighthouse-cli/test/smokehouse/run-smoke.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ const SMOKETESTS = [{
expectations: smokehouseDir + 'error-expectations.js',
config: smokehouseDir + 'error-config.js',
batch: 'errors',
}, {
id: 'oopif',
expectations: smokehouseDir + 'oopif-expectations.js',
config: smokehouseDir + 'oopif-config.js',
batch: 'parallel-first',
}, {
id: 'pwa',
expectations: smokehouseDir + 'pwa-expectations.js',
Expand Down
24 changes: 22 additions & 2 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;
// currently only used by WPT where just Page and Network are needed
this._devtoolsLog = new DevtoolsLog(/^(Page|Network)\./);
// Used to save network and lifecycle protocol traffic. Just Page, Network, and Target are needed.
this._devtoolsLog = new DevtoolsLog(/^(Page|Network|Target)\./);
this.online = true;
/** @type {Map<string, number>} */
this._domainEnabledCounts = new Map();
Expand All @@ -69,6 +69,20 @@ class Driver {
*/
this._monitoredUrl = null;

let 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', {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.sendCommand('Target.sendMessageToTarget', {
this.sendCommand('Network.enable', undefined, event.sessionId);

This kind of thing could be possible with the flattened protocol... See https://github.com/GoogleChrome/puppeteer/pull/3827/files We'd also add flatten: true on setAutoAttach.

The sessionId is sent not as a param but as a sibling to params. And then it's received as a sibling to object.id when events come back.

I tried this out, but it adds a bunch of complication around our typing for sendCommand. So IMO it's probably not worth dealing with that right now.

Copy link
Collaborator Author

@patrickhulce patrickhulce Mar 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I tried this out, but it adds a bunch of complication around our typing for sendCommand. So IMO it's probably not worth dealing with that right now.

+1, the direction that method started taking me seemed far more complicated given our existing setup than this approach. As long as it's not disappearing from protocol tomorrow, I think this is better for us :)

Copy link
Member

Choose a reason for hiding this comment

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

I tried this out, but it adds a bunch of complication around our typing for sendCommand

maybe we can work on this so we aren't straightjacketed by our type checker

message: JSON.stringify({id: targetProxyMessageId, method: 'Network.enable'}),
sessionId: event.sessionId,
});
});

connection.on('protocolevent', event => {
this._devtoolsLog.record(event);
if (this._networkStatusMonitor) {
Expand Down Expand Up @@ -1015,6 +1029,12 @@ class Driver {
await this._beginNetworkStatusMonitoring(url);
await this._clearIsolatedContextId();

// Enable auto-attaching to subtargets so we receive iframe information
await this.sendCommand('Target.setAutoAttach', {
autoAttach: true,
waitForDebuggerOnStart: false,
});

await this.sendCommand('Page.enable');
await this.sendCommand('Page.setLifecycleEventsEnabled', {enabled: true});
await this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS});
Expand Down
20 changes: 16 additions & 4 deletions lighthouse-core/lib/network-recorder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,15 +303,26 @@ 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);
Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment (jsdoc or inline) about where these messages come from and the expected form? (basically, I assume, stringified protocol responses in the message property)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done


// 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
*/
dispatch(event) {
if (!event.method.startsWith('Network.')) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in case you're curious like me if this was saving us anytime, it looks like it wasn't

just a switch is ~10x faster

https://jsperf.com/switch-vs-startswith

return;
}

switch (event.method) {
case 'Network.requestWillBeSent': return this.onRequestWillBeSent(event.params);
case 'Network.requestServedFromCache': return this.onRequestServedFromCache(event.params);
Expand All @@ -320,6 +331,7 @@ 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
default: return;
}
}
Expand Down
34 changes: 34 additions & 0 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 @@ -487,6 +487,7 @@ describe('.gotoURL', () => {
.mockResponse('Page.setLifecycleEventsEnabled', {})
.mockResponse('Emulation.setScriptExecutionDisabled', {})
.mockResponse('Page.navigate', {})
.mockResponse('Target.setAutoAttach', {})
.mockResponse('Runtime.evaluate', {});
});

Expand Down Expand Up @@ -930,3 +931,36 @@ describe('.goOnline', () => {
});
});
});

describe('Multi-target management', () => {
it('enables the Network domain for iframes', async () => {
connectionStub.sendCommand = createMockSendCommandFn()
.mockResponse('Target.sendMessageToTarget', {});

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

const sendMessageArgs = connectionStub.sendCommand
.findInvocation('Target.sendMessageToTarget');
expect(sendMessageArgs).toEqual({
message: '{"id":1,"method":"Network.enable"}',
sessionId: 123,
});
});

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

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

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