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

patrickhulce · 2019-01-03T18:10:50Z

Summary
Enables us to collect network domain information from iframes. This is likely going to have a significant impact on performance numbers. I'll want to run DZL on it to get a sense of the impact, but we should consider it a "fix" since you could basically stick your whole site into a cross-origin iframe before and get a 100 performance score before.

I think this is all we really want to tackle for now? Future work could enable some cool stuff like the long awaited "fetch inside service worker" and whatnot, but we'd want some fancier target handling helpers first and this seems like a good, high impact start.

Related Issues/PRs
fixes #6337

DZL, do a barrel roll!

brendankenny

I haven't thought through the oopif case very much. What are the contention concerns here

CPU/process/thread contention is difficult to even formulate what questions we should be asking, but is the argument that iframe network requests will be competing for the same available network resources so should be factored into our byte-efficiency, etc audits?

brendankenny · 2019-01-03T19:12:08Z

Also, this might just be your point about DZL, but how does this affect the metrics? Has anyone done any investigation of TTI in a world of iframes? Seems like network quiet could be dramatically different even if the iframe isn't noticeably affecting when the page is "interactive"?

patrickhulce · 2019-01-03T19:19:10Z

but is the argument that iframe network requests will be competing for the same available network resources so should be factored into our byte-efficiency, etc audits?

This is the only real contention problem I see iframes posing in an OOPIF world. The story is definitely more complicated than that, but at its simplest I think network requests slowing down main content is the biggest problem we can capture.

I think there are a few other concerns we might want to think through. Like

Do we want to recommend opportunities on things that are inside iframes? IMO we do, and just filter like 3p
(You already mentioned this but) Do network requests inside iframes count towards network quiet? IMO they should since that was what was used when the original TTI investigation was done
Do we go any steps further with other audits? i.e. run all our gatherers in all subtargets too

patrickhulce · 2019-01-03T19:30:59Z

lighthouse-core/lib/network-recorder.js

  /**
   * Routes network events to their handlers, so we can construct networkRecords
   * @param {LH.Protocol.RawEventMessage} event
   */
  dispatch(event) {
-    if (!event.method.startsWith('Network.')) {


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

devtools-bot · 2019-01-03T20:29:10Z

DZL is done! Go check it out http://lh-dzl-6922.surge.sh

patrickhulce · 2019-01-16T17:27:10Z

this one too, I wonder if this should've been considered a breaking change 🤔

patrickhulce · 2019-02-27T19:04:40Z

@paulirish said he wanted to take a look, thought this was already merged 😆

paulirish

How should oopifs affect our view of the page?

Network: yes any requests should be considered so we can evaluate contention, etc.
CPU: nope these are a separate thread and we assume a simple model where we dont need to consider cross-process CPU contention.

Right now this is tackling the network side of things so IMO we are good. :)

paulirish · 2019-02-27T23:29:00Z

lighthouse-core/gather/driver.js

+
+      // 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', {


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.

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 :)

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

paulirish · 2019-02-28T00:43:09Z

This also flips Network.enable on service workers. I am not sure what affect this has.. In devtools service workers get their own network record, so I'm not sure if we'd be double-counting these requests now?

We can look into this or only flip on Network.enable if the new target is an iframe.

paulirish

just setting status: We need to resolve the network-requests-in-service-workers concern.

Aside from that, this PR lgtm.

devtools-bot · 2019-03-01T16:21:33Z

DZL is done! Go check it out http://lh-dzl-6922.surge.sh

devtools-bot · 2019-03-01T16:36:21Z

DZL is done! Go check it out http://lh-dzl-6922.surge.sh

brendankenny

LGTM

I still think this is going to do some weird things to our audits with network records from all frames but all the other artifacts from just the main frame, but I guess this is one way to find out :)

brendankenny · 2019-03-02T00:00:27Z

lighthouse-core/lib/network-recorder.js

+   */
+  onReceivedMessageFromTarget(data) {
+    /** @type {LH.Protocol.RawMessage} */
+    const protocolMessage = JSON.parse(data.message);


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)

brendankenny · 2019-03-02T00:01:42Z

lighthouse-core/lib/network-recorder.js

+  onReceivedMessageFromTarget(data) {
+    /** @type {LH.Protocol.RawMessage} */
+    const protocolMessage = JSON.parse(data.message);
+    if ('id' in protocolMessage) return;


and a comment here that (I'm assuming like in connection.js) if it has an id it's a command response, and we only care about events

brendankenny · 2019-03-02T00:09:47Z

lighthouse-cli/test/smokehouse/oopif-expectations.js

+      'network-requests': {
+        details: {
+          items: {
+            length: '>10',


maybe add a comment why this is a good bounds

brendankenny · 2019-03-02T00:25:24Z

lighthouse-core/gather/driver.js

@@ -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)\./);
+    // currently used by network-recorder just Page, Network, and Target are needed


do we need this comment anymore? It's devtoolsLog :)

(at most we could say it's used to provide the artifact of the same name)

heh, fair, I made a note that its purpose is to save network traffic. Seems like we need something to denote why we are filtering this way.

patrickhulce · 2019-03-02T17:18:39Z

We need to resolve the network-requests-in-service-workers concern.

@paulirish I limited it to just iframes for now and added tests 👍

devtools-bot · 2019-03-02T17:31:14Z

DZL is done! Go check it out http://lh-dzl-6922.surge.sh

paulirish

lg!

patrickhulce added 2 commits January 3, 2019 11:56

core(oopif): collect network requests from iframes

35de9e9

comment tweaks

d6e8aea

patrickhulce requested review from brendankenny and paulirish as code owners January 3, 2019 18:10

brendankenny reviewed Jan 3, 2019

View reviewed changes

patrickhulce commented Jan 3, 2019

View reviewed changes

patrickhulce added the waiting4reviewer label Jan 8, 2019

paulirish reviewed Feb 28, 2019

View reviewed changes

paulirish requested changes Feb 28, 2019

View reviewed changes

patrickhulce added 3 commits February 28, 2019 22:59

Merge branch 'master' into oopif

52e1640

only listen for iframes

1734ee0

driver tests

f546378

brendankenny approved these changes Mar 2, 2019

View reviewed changes

feedback

3a10248

paulirish approved these changes Mar 2, 2019

View reviewed changes

paulirish merged commit 390343a into master Mar 2, 2019

paulirish deleted the oopif branch March 2, 2019 17:47

paulirish mentioned this pull request Mar 14, 2019

core(network-recorder): consider iframe responses finished #6078

Merged

This was referenced Mar 20, 2019

core(oopif): attach to all descendants #7608

Merged

core(oopifs): skip OOPIF network records in some gatherers #7640

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

patrickhulce commented Jan 3, 2019 •

edited

Loading

brendankenny left a comment

brendankenny commented Jan 3, 2019

patrickhulce commented Jan 3, 2019

patrickhulce Jan 3, 2019

devtools-bot commented Jan 3, 2019

patrickhulce commented Jan 16, 2019

patrickhulce commented Feb 27, 2019

paulirish left a comment

paulirish Feb 27, 2019

patrickhulce Mar 1, 2019 •

edited

Loading

brendankenny Mar 1, 2019

paulirish commented Feb 28, 2019

paulirish left a comment

devtools-bot commented Mar 1, 2019

devtools-bot commented Mar 1, 2019

brendankenny left a comment

brendankenny Mar 2, 2019

patrickhulce Mar 2, 2019

brendankenny Mar 2, 2019

patrickhulce Mar 2, 2019

brendankenny Mar 2, 2019

patrickhulce Mar 2, 2019

brendankenny Mar 2, 2019

patrickhulce Mar 2, 2019

patrickhulce commented Mar 2, 2019

devtools-bot commented Mar 2, 2019

paulirish left a comment

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

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

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

Conversation

patrickhulce commented Jan 3, 2019 • edited Loading

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny commented Jan 3, 2019

patrickhulce commented Jan 3, 2019

Choose a reason for hiding this comment

devtools-bot commented Jan 3, 2019

patrickhulce commented Jan 16, 2019

patrickhulce commented Feb 27, 2019

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Feb 28, 2019

paulirish left a comment

Choose a reason for hiding this comment

devtools-bot commented Mar 1, 2019

devtools-bot commented Mar 1, 2019

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Mar 2, 2019

devtools-bot commented Mar 2, 2019

paulirish left a comment

Choose a reason for hiding this comment

patrickhulce commented Jan 3, 2019 •

edited

Loading

patrickhulce Mar 1, 2019 •

edited

Loading