core(oopif): autoattach to all descendants #7517

patrickhulce · 2019-03-14T21:47:18Z

Summary
While trying to revert #6078 discovered a few iframes that don't get autoattached for some reason. It's unclear to me why setAutoAttach doesn't find them. At first I thought it was a grandchild iframe problem, but that didn't really explain the pattern and recursively sending setAutoAttach didn't solve it either. It's cross-origin grandchild frames that get put into new processes distinct from all other previous frames. AFAICT, in many cases the grandchild issue wasn't a problem before because the site also had a direct child that got grouped into that same process (i.e. the verge has a doubleclick iframe so when a different ad iframe adds a doubleclick grandchild frame we were still good, or at least that's why I believe my test cases were all my passing). The reproducible case I've got now is our localhost iframing paulirish.com which iframes youtube, 3 distinct origins that always get separated.

Switching over to autodiscover solved it though, so I've switched to that here. I feel a tad out of my depth here maybe someone from devtools would want to comment on what my issue is. Perhaps flatten: true solves this and isn't just about the sessionId bookkeeping change? Per Pavel we should avoid autodiscover and go with autoattach. This means we must start keeping track of session ids on our connection, so here comes the plumbing! 😭

~~I'm also not 100% how to add a test for this since I don't fully understand why devtools doesn't attach to all targets and I have trouble reproducing it anyhow, but suggestions welcome :)~~

Related Issues/PRs
#6337 #6078

paulirish · 2019-03-14T22:20:59Z

@pavelfeldman @aslushnikov can you take a look and lend some insight?

paulirish · 2019-03-14T22:34:44Z

Pavel says setDiscoverTargets will report all targets in the browser.. (so.. iframes in other tabs would make it through). the targetInfo does report a parent.. so it's possible to use this and filter to just your tree, but funky.

autoAttach is preferred and should work for us. And Pavel's interested in a test case if recursively calling setAutoattach isn't reliably finding iframes.

patrickhulce · 2019-03-15T16:17:11Z

And Pavel's interested in a test case if recursively calling setAutoattach isn't reliably finding iframes.

Finally found the narrow case where it's happening and I see where I went wrong 👍

Take 3 is up, if we're going with setAutoAttach there's not really a way to avoid threading sessionId down to the connection level which is what I was hoping to avoid. If we wanted to start reporting opportunities on iframe resources though this was somewhat inevitable.

I did it in the least impactful way I could think of right now. WDYT?

paulirish · 2019-03-15T16:26:04Z

DZL, do a barrel roll!

paulirish · 2019-03-15T16:26:46Z

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

+            // the iframe's iframe's iframe's requests (youtube.com/doubleclick/etc).
+            // - paulirish.com ~50-60 requests
+            // - paulirish.com + all descendant iframes ~80-90 requests
+            length: '>70',


I have a feeling this is going to have a big effect on TTI as network idle is likely going to be pushed out in a bunch of cases.

paulirish · 2019-03-15T16:29:34Z

lighthouse-core/gather/driver.js

@@ -279,11 +279,15 @@ class Driver {
   * Call protocol methods, with a timeout.
   * To configure the timeout for the next call, use 'setNextProtocolTimeout'.
   * @template {keyof LH.CrdpCommands} C
-   * @param {C} method
+   * @param {C|{method: C, sessionId: string}} methodObj


Yah I dig this solution.

A followup PR can move this._nextProtocolTimeout into this methodObj. cc @hoten

I feel the same way about this as I did for #6347 :) We're trying to generalize a simple thing by making it more complicated, just for the two calls that need it.

Since they're both inside driver, would it be possible for now to make those two calls be calls of _innerSendCommand instead, leave sendCommand the same, and we set up a time to figure out what driver should look like in a multi target world (which is also going to help us do better SW work)? I think long term that can also help with complexity when we start doing more opportunities with iframe resources, as ideally we can minimize the manual sessionId management that has to be done in gatherers.

Since they're both inside driver, would it be possible for now to make those two calls be calls of _innerSendCommand instead, leave sendCommand the same, and we set up a time to figure out what driver should look like in a multi target world (which is also going to help us do better SW work)?

sure

paulirish · 2019-03-15T16:37:13Z

lighthouse-core/gather/driver.js

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


We still get back these messages over the target domain?

In https://github.com/GoogleChrome/puppeteer/pull/3827/files#diff-0c1fe9dd78cc7f29823c09b251c8aae0R109 I saw the sessionId being received and handled in the handleRawMessage method (basically).

whoops no, I need to update the comment 👍

brendankenny · 2019-03-15T17:22:57Z

lighthouse-core/gather/driver.js

@@ -279,11 +279,15 @@ class Driver {
   * Call protocol methods, with a timeout.
   * To configure the timeout for the next call, use 'setNextProtocolTimeout'.
   * @template {keyof LH.CrdpCommands} C
-   * @param {C} method
+   * @param {C|{method: C, sessionId: string}} methodObj


I feel the same way about this as I did for #6347 :) We're trying to generalize a simple thing by making it more complicated, just for the two calls that need it.

Since they're both inside driver, would it be possible for now to make those two calls be calls of _innerSendCommand instead, leave sendCommand the same, and we set up a time to figure out what driver should look like in a multi target world (which is also going to help us do better SW work)? I think long term that can also help with complexity when we start doing more opportunities with iframe resources, as ideally we can minimize the manual sessionId management that has to be done in gatherers.

brendankenny · 2019-03-15T17:26:25Z

lighthouse-core/gather/driver.js

    const domainCommand = /^(\w+)\.(enable|disable)$/.exec(method);
-    if (domainCommand) {
+    if (domainCommand && !sessionId) {


if we're going to expand to other targets, we should probably track enabled domains there as well...

I'll use your point back atcha for now ;)

It's only the two messages that are never disabled, so we can cross that bridge when we get to it IMO

It's only the two messages that are never disabled, so we can cross that bridge when we get to it IMO

ha, I am 👌👌 with crossing that bridge later, but maybe leave a comment about not bothering to check other targets?

brendankenny · 2019-03-15T17:26:41Z

lighthouse-core/test/lib/network-recorder-test.js

-      assert.deepStrictEqual(periods, [
-        {start: 1200, end: Infinity},
-      ]);
+      assert.deepStrictEqual(periods, []);


what changed with the quiet period?

this was the intentional removal of our hack for frame roots, now if an iframe never fires loadingFinished it shouldn't be seen as finished

ah, got it, I wasn't thinking about the network-recorder.js changes

patrickhulce · 2019-03-18T21:27:26Z

Soooooooo, I totally forgot the extension existed again (as I tend to do) and the story is a lot more complicated there. I'll put up what I have in a separate PR ( #7566 ) so it's easier to see, but basically my conclusion is that we cannot use setAutoAttach there.

patrickhulce · 2019-03-20T20:27:57Z

This has changed enough from the beginning that I'm just opening a new PR: #7608

core(oopif): prefer autodiscover over autoattach

7f2f90c

patrickhulce requested review from brendankenny and paulirish as code owners March 14, 2019 21:47

patrickhulce mentioned this pull request Mar 14, 2019

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

Merged

patrickhulce added 3 commits March 15, 2019 10:49

revert old attempt

ddd8ee5

thread session ID all the way down

552f042

update smokehouse tests

b33d132

paulirish reviewed Mar 15, 2019

View reviewed changes

brendankenny reviewed Mar 15, 2019

View reviewed changes

patrickhulce changed the title ~~core(oopif): prefer autodiscover over autoattach~~ core(oopif): autoattach to all descendants Mar 15, 2019

patrickhulce added 3 commits March 15, 2019 14:07

feedback

e6f2d43

fix driver tests

5545c08

fix gather runner test

79932bd

patrickhulce mentioned this pull request Mar 18, 2019

OOPIF full support in extension #7566

Closed

patrickhulce closed this Mar 20, 2019

patrickhulce mentioned this pull request Mar 20, 2019

core(oopif): attach to all descendants #7608

Merged

brendankenny mentioned this pull request Sep 20, 2019

Embedding in another extension #7350

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(oopif): autoattach to all descendants #7517

core(oopif): autoattach to all descendants #7517

patrickhulce commented Mar 14, 2019 •

edited

Loading

paulirish commented Mar 14, 2019

paulirish commented Mar 14, 2019

patrickhulce commented Mar 15, 2019

paulirish commented Mar 15, 2019

paulirish Mar 15, 2019

paulirish Mar 15, 2019

brendankenny Mar 15, 2019 •

edited

Loading

patrickhulce Mar 15, 2019

paulirish Mar 15, 2019

patrickhulce Mar 15, 2019

brendankenny Mar 15, 2019 •

edited

Loading

brendankenny Mar 15, 2019

patrickhulce Mar 15, 2019

brendankenny Mar 15, 2019

brendankenny Mar 15, 2019

patrickhulce Mar 15, 2019

brendankenny Mar 15, 2019

patrickhulce commented Mar 18, 2019 •

edited

Loading

patrickhulce commented Mar 20, 2019 •

edited by paulirish

Loading

core(oopif): autoattach to all descendants #7517

core(oopif): autoattach to all descendants #7517

Conversation

patrickhulce commented Mar 14, 2019 • edited Loading

paulirish commented Mar 14, 2019

paulirish commented Mar 14, 2019

patrickhulce commented Mar 15, 2019

paulirish commented Mar 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Mar 15, 2019 • edited Loading

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

brendankenny Mar 15, 2019 • edited Loading

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 18, 2019 • edited Loading

patrickhulce commented Mar 20, 2019 • edited by paulirish Loading

patrickhulce commented Mar 14, 2019 •

edited

Loading

brendankenny Mar 15, 2019 •

edited

Loading

brendankenny Mar 15, 2019 •

edited

Loading

patrickhulce commented Mar 18, 2019 •

edited

Loading

patrickhulce commented Mar 20, 2019 •

edited by paulirish

Loading