core: use flatten protocol #9783

connorjclark · 2019-10-04T03:03:09Z

We need to migrate to the flatten protocol for a few reasons:

Non-flatten is officially deprecated.
It's required to fix a protocol bug in DevTools.
Wrapping the protocol via Target.sendMessageToTarget / Target.receivedMessageFromTarget is a waste of bandwidth and processing time.

For this initial pass, I created a new sendCommandToSession method so the ~200 usages of sendCommand don't need to change. Could split out the very common code between sendCommand / sendCommandToSession, but I don't think we combine it into one interface (the rest params make an optional parameter impossible).

Open problems:

For CLI + Chrome Stable, the oopif smoke fixture never seems to render the iframe. The iframe remains white the entire run and just hangs. No issue for Canary or Beta.

The oopif smoke fixture seems to take a few seconds longer. Why?
DevTools crashes the renderer process when the mobile emulation Lighthouse applies is reverted. This happens even with just the LH bundle changes (no other devtools frontend changes).

tracing_service_impl.cc: Tracing session 1 ended, total sessions:0
Received signal 11 SEGV_MAPERR 000000000000
#0 0x7f0837a456e9 base::debug::CollectStackTrace()
#1 0x7f083797ef63 base::debug::StackTrace::StackTrace()
#2 0x7f0837a45271 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#3 0x7f082c0b03a0 <unknown>
#4 0x7f083590b279 content::RenderWidgetScreenMetricsEmulator::DisableAndApply()
#5 0x7f08358fb700 content::RenderWidget::OnDisableDeviceEmulation()
#6 0x7f08358fb647 IPC::MessageT<>::Dispatch<>()
#7 0x7f08358faa4c content::RenderWidget::OnMessageReceived()
#8 0x7f0836170511 IPC::ChannelProxy::Context::OnDispatchMessage()
#9 0x7f08379e406f base::TaskAnnotator::RunTask()
#10 0x7f08379f86ca base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl()
#11 0x7f08379f84a8 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoSomeWork()
#12 0x7f083799f15a base::MessagePumpDefault::Run()
#13 0x7f08379f8ee9 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run()
#14 0x7f08379c79c7 base::RunLoop::Run()
#15 0x7f083590ec61 content::RendererMain()
#16 0x7f0835a25dfe content::RunZygote()
#17 0x7f0835a27064 content::ContentMainRunnerImpl::Run()
#18 0x7f08294428ad service_manager::Main()
#19 0x7f0835a25381 content::ContentMain()
#20 0x55b6ba09146f ChromeMain
#21 0x7f082ae6e52b __libc_start_main
#22 0x55b6ba0912da _start
  r8: 00007f08358fb6e0  r9: 0000000000000000 r10: 0000000000000002 r11: 0000000000000003
 r12: 0000000000000000 r13: 0000069d5a620738 r14: 00007f08358fb6e0 r15: 0000069d59f98800
  di: 0000000000000000  si: 0000069d5a620738  bp: 00007fff05c25860  bx: 0000000000000000
  dx: 0000069d59f98800  ax: 0000069d5a245a58  cx: 0000000000000000  sp: 00007fff05c25850
  ip: 00007f083590b279 efl: 0000000000010202 cgf: 002b000000000033 erf: 0000000000000004
 trp: 000000000000000e msk: 0000000000000000 cr2: 0000000000000000
[end of stack trace]
Calling _exit(1). Core file will not be generated.

I left some useful debugging code commented out. Will remove after these open problems are resolved.

Timings:

CHROME_PATH=~/Desktop/opt/google/chrome-beta/google-chrome-beta node lighthouse-core/scripts/compare-timings.js --name base --collect -n 10 --urls http://localhost:10200/oopif.html https://www.nyt.com https://www.sfgate.com/

https://gist.githubusercontent.com/connorjclark/b6ed524cfd9cbead8186e219a0f74f59/raw/a089d238eb166ff3954ba7918f61736effcbb664/gistfile1.txt

lighthouse-core/gather/driver.js

patrickhulce

Very fast @connorjclark nice work! 💨

This should not merge until the Chromium changes are ready.

The Chromium changes in that CL look like they include an LH roll, is that a circularish dependency? They also seem frontend only, does that mean that the bug...

DevTools crashes the renderer process when the mobile emulation Lighthouse applies is reverted. This happens even with just the LH bundle changes (no other devtools frontend changes).

will need to be fixed by a separate CL?

Basically, what should we reviewers do for you now? :)

lighthouse-core/gather/connections/connection.js

lighthouse-core/gather/driver.js

patrickhulce · 2019-10-04T15:13:22Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

@@ -40,7 +40,7 @@ class ResponseCompression extends Gatherer {
    const unoptimizedResponses = [];

    networkRecords.forEach(record => {
-      // Ignore records from OOPIFs
+      // Ignore records from child targets (OOPIFS).
      if (record.sessionId) return;


requests from the main target still won't have a sessionId will they?

connorjclark · 2019-10-04T17:32:37Z

Very fast @connorjclark nice work!

Thanks :)

The Chromium changes in that CL look like they include an LH roll, is that a circularish dependency?

I mean we shouldn't land this in LH until we fix the crashing bug in DevTools. Otherwise, we won't be able to release to DevTools. The CL doesn't have to land first.

They also seem frontend only, does that mean that the ...
will need to be fixed by a separate CL?

We should have a fix ready to go before landing any of this.

Basically, what should we reviewers do for you now? :)

Mostly, we should start bike-shedding the necessary interface changes to sendCommand.

Any idea why the crash is happening? That's what I'm up to today.

connorjclark · 2019-10-08T00:57:18Z

DevTools crashes the renderer process when the mobile emulation Lighthouse applies is reverted. This happens even with just the LH bundle changes (no other devtools frontend changes).

Update on this. This is unrelated to these changes. Can be repro'd by merging master LH into ToT Chromium. AFAICT, an additional Session/EmulationHandler.cc is sending an extra DisableDeviceEmulation IPC. The second "disable" crashes, I guess because the delegate has already been destroyed. Without rolling LH master, I only see a single EmulationHandler.

some logging I found helpful, for later.

./out/Linux/chrome --enable-logging=stderr --v=1 2>&1 | grep  connor

# plz someone teach me better chromium debugging

diff --git a/content/browser/devtools/protocol/emulation_handler.cc b/content/browser/devtools/protocol/emulation_handler.cc
index b2ffbf81c4c0..60f3c3be8fb1 100644
--- a/content/browser/devtools/protocol/emulation_handler.cc
+++ b/content/browser/devtools/protocol/emulation_handler.cc
@@ -170,6 +170,8 @@ Response EmulationHandler::SetDeviceMetricsOverride(
     Maybe<bool> dont_set_visible_size,
     Maybe<Emulation::ScreenOrientation> screen_orientation,
     Maybe<protocol::Page::Viewport> viewport) {
+  LOG(WARNING) << "connor EmulationHandler::SetDeviceMetricsOverride "<<device_emulation_enabled_;
+
   const static int max_size = 10000000;
   const static double max_scale = 10;
   const static int max_orientation_angle = 360;
@@ -290,6 +292,7 @@ Response EmulationHandler::ClearDeviceMetricsOverride() {
     return Response::OK();
   if (!host_)
     return Response::Error("Can't find the associated web contents");
+  LOG(WARNING) << "connor EmulationHandler::ClearDeviceMetricsOverride "<<device_emulation_enabled_;
   GetWebContents()->ClearDeviceEmulationSize();
   device_emulation_enabled_ = false;
   device_emulation_params_ = blink::WebDeviceEmulationParams();
@@ -337,6 +340,7 @@ void EmulationHandler::SetDeviceEmulationParams(
     const blink::WebDeviceEmulationParams& params) {
   bool enabled = params != blink::WebDeviceEmulationParams();
   if (params != device_emulation_params_) {
+    LOG(WARNING) << "connor EmulationHandler::SetDeviceEmulationParams " << device_emulation_enabled_;
     device_emulation_enabled_ = enabled;
     device_emulation_params_ = params;
     UpdateDeviceEmulationState();
@@ -376,6 +380,9 @@ void EmulationHandler::UpdateDeviceEmulationState() {
   // Device emulation only happens on the main frame.
   if (!host_->frame_tree_node()->IsMainFrame())
     return;
+  
+  LOG(WARNING) << "connor EmulationHandler::UpdateDeviceEmulationState " << " " << this << " --- " << device_emulation_enabled_;
+  base::debug::StackTrace().PrintWithPrefix("connor ");

connorjclark · 2019-10-08T01:01:46Z

It's #9377. yay...I solved it? :/ Let's merge this plz.

connorjclark · 2019-10-08T19:58:30Z

Must merge #9193 first. Flatten won't work with the extension.

connorjclark · 2019-10-08T20:31:41Z

These changes won't work with M77 (see failing oopif smoke test). I don't know why. We don't really need it to (78 comes
out Oct 15, and that works), except that CI uses M77.

connorjclark · 2019-10-08T23:19:25Z

Bisected to https://chromium.googlesource.com/chromium/src/+log/87e8027a5738d0daf1dbb7e350693eff745b5961..abc13cb07e3b62fa21003cfc38fcb5c2fab06aab

Only DevTools change: https://chromium.googlesource.com/chromium/src/+/001ceaaaa92cf3dbfa573fb40c3a682e0dc72fd7

This is what fixed the issue.

Snippet from CL:

When devtools auto-attaches to OOPIF, it will trigger several calls to AttachToAgent. The session in that case is not suspended, and all waiting messages are forwarded to the new render frame host (when it is ready to commit navigation). After the navigation, ResumeSendingMessagesToAgent will be called and should be ignored as the session is not suspended and all messages have been forwarded to the RFH.

Bisect command:

python tools/bisect-builds.py -b 681094 -g 693954 -a mac64 -c \
 'bash -c "CHROME_PATH=%p node /Users/cjamcl/src/lighthouse/lighthouse-cli http://localhost:10200/oopif.html"'

You are probably looking for a change made after 691305 (known bad), but no later than 691327 (first known good).

connorjclark · 2019-10-08T23:30:33Z

So here are the pertinent dates and constraints:

Oct 15: M78 stable release. To avoid breaking master LH on Stable Chrome, must land this PR after this date.

Also Oct 15: scheduled LH release.

Oct 17: M79 branch. To fix auditing OOPIFs in the Audits panel, must land this PR and the associated Chromium CL by this date (plus, roll LH). This is a soft deadline - we can probably get a merge OK'd a bit after easily enough.

Removing Draft status, since the big two issues have been addressed. PTAL

connorjclark · 2019-10-14T23:35:10Z

@connorjclark this doesn't pass our OOPIF smoketest in travis, seems like there's something to be fixed and/or something local you need to push?

See #9783 (comment) and the two comments following. These changes won't work in CI until the new stable release tomorrow.

EDIT: are we still waiting on the Chromium change to be merged? I'm still not 100% clear on the sequence needed here

The Chromium change has been merged already.

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

patrickhulce · 2019-10-14T23:41:09Z

See #9783 (comment) and the two comments following. These changes won't work in CI until the new stable release tomorrow.

Oh right, sorry! I guess we won't get green builds until #9193 is merged anyway?

This approach LGTM though 👍

paulirish · 2019-10-15T00:06:01Z

LGTM from me.
The chromium breakage is certainly making this annoying.

patrickhulce

alright LGTM assuming we can get a green build out of the new stable :)

patrickhulce · 2019-10-15T20:12:55Z

lighthouse-core/gather/driver.js

   * @param {boolean} enable
   * @return {boolean}
   * @private
   */
-  _shouldToggleDomain(domain, enable) {
-    const enabledCount = this._domainEnabledCounts.get(domain) || 0;
+  _shouldToggleDomain(domain, sessionId, enable) {


time to add tests for this functionality? :D

should hopefully be pretty easy with the new mock capabilities and can just sendCommand('Network.enable') or whatever to make sure it doesn't turn off to early :)

time to add tests for this functionality? :D

should hopefully be pretty easy with the new mock capabilities and can just sendCommand('Network.enable') or whatever to make sure it doesn't turn off to early :)

+1 to this. After a certain number of enables and disables, watching when the actual command is sent shouldn't end up too bad, I hope.

brendankenny

LGTM3 with an extra test or two

Nice and neat and great to be able to delete a good chunk of code

lighthouse-core/gather/gatherers/dobetterweb/optimized-images.js

brendankenny · 2019-10-15T20:52:41Z

lighthouse-core/test/gather/driver-test.js

-    });
-  });
-
-  it('enables the Network domain for iframes of iframes of iframes', async () => {


was the earlier discussions about tests about this? It seems a shame to lose this assertion (and we still have to make sure to recursively enable attaching, so could still conceivably be broken at some point)

The path for attaching to an iframe target is now the exact same as attaching to an iframe's iframe target, so I don't see functionality in having a test for this. I removed the test because it needed to be rewritten to continue working, and how to do so was a head-scratcher.

The path for attaching to an iframe target is now the exact same as attaching to an iframe's iframe target, so I don't see functionality in having a test for this.

one level deep appears to be in gotoURL while 2+ levels deep appears to happen in _handleTargetAttached, I believe?

@patrickhulce could maybe help make the call/give an idea for testing

goToURL calls Target.setAutoAttach on the main target, but the first and subsequent targets will be via _handleTargetAttached. I suppose it was this way before too, but what's different now is that we don't have to worry about the complicated message routing - I believe this test existed just to verify that mechanism.

yeah, it's having a test of the inductive step that would feel good, even if it's a trivial difference from the initial attachment

though I guess what you may be saying is that since we don't keep any hierarchy of the child targets (e.g. looking at openerId), having a child target of a child target is indistinguishable from having two sibling child targets in terms of protocol traffic we'd care about.

That's a good point and that means the second one is a really trivial difference from the first, but it seems ok to include a two-child-target case, which would also provide a nice tripwire/starting point if and when we ever start treating fourth-party, etc content differently and need to track the target hierarchy

I mostly lean on @connorjclark's side of this that there is no difference to driver in this case now and we're basically relying on chrome to do the right thing and send us nested targets, mocking Chrome doing the right thing doesn't feel super valuable since it ends up being indistinguishable to us

I feel comfortable enough with the grandchild OOPIF smoke test we have that exercises this, but a two-child-target test case seems reasonable as the test for this anyhow, so might as well :)

brendankenny · 2019-10-15T20:55:10Z

lighthouse-core/gather/driver.js

   * @param {boolean} enable
   * @return {boolean}
   * @private
   */
-  _shouldToggleDomain(domain, enable) {
-    const enabledCount = this._domainEnabledCounts.get(domain) || 0;
+  _shouldToggleDomain(domain, sessionId, enable) {


time to add tests for this functionality? :D

should hopefully be pretty easy with the new mock capabilities and can just sendCommand('Network.enable') or whatever to make sure it doesn't turn off to early :)

+1 to this. After a certain number of enables and disables, watching when the actual command is sent shouldn't end up too bad, I hope.

Also cherry-picked: GoogleChrome/lighthouse#9377 GoogleChrome/lighthouse#9783 Bug: 772558 Bug: 1011228 Change-Id: Ibe9e4b3849940bf86c666bf55698a9ced4253fd3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1867249 Reviewed-by: Erik Luo <luoe@chromium.org> Commit-Queue: Connor Clark <cjamcl@google.com> Cr-Original-Commit-Position: refs/heads/master@{#707110} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 5f16f693662c78467b8ae531a3f03e37ccf94c69

Also cherry-picked: GoogleChrome/lighthouse#9377 GoogleChrome/lighthouse#9783 Bug: 772558 Bug: 1011228 Change-Id: Ibe9e4b3849940bf86c666bf55698a9ced4253fd3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1867249 Reviewed-by: Erik Luo <luoe@chromium.org> Commit-Queue: Connor Clark <cjamcl@google.com> Cr-Commit-Position: refs/heads/master@{#707110}

connorjclark · 2019-10-21T23:50:20Z

Should start passing tomorrow :)

connorjclark · 2019-11-04T21:25:14Z

This needs to land, or else it becomes the second PR that needs to be merged to releases for DevTools to work (the first being #9377)

brendankenny · 2019-11-04T21:29:47Z

This needs to land, or else it becomes the second PR that needs to be merged to releases for DevTools to work

I see two official LGTMs and a third unofficial one, so... :P

flatten

c67c7c9

googlebot added the cla: yes label Oct 4, 2019

connorjclark commented Oct 4, 2019

View reviewed changes

lighthouse-core/gather/driver.js Outdated Show resolved Hide resolved

patrickhulce reviewed Oct 4, 2019

View reviewed changes

undo ws

67b9ace

vercel bot deployed to staging October 4, 2019 17:17 View deployment

sendRawMessage

9229478

vercel bot deployed to staging October 4, 2019 17:22 View deployment

use sendCommandToSession

f6a2831

vercel bot deployed to staging October 4, 2019 17:25 View deployment

connorjclark added 3 commits October 8, 2019 10:28

Merge remote-tracking branch 'origin/master' into flatten

ab6582f

rm test code

de0c3c6

fix driver tests

06c2fe8

vercel bot deployed to staging October 8, 2019 18:51 View deployment

connorjclark added 2 commits October 8, 2019 11:54

fix gather runner

a040b1a

fix gather runner test. skip extension test

f503fdb

vercel bot deployed to staging October 8, 2019 19:57 View deployment

connorjclark mentioned this pull request Oct 8, 2019

clients: retire extension; replace with PSI launcher #9193

Merged

reuse impl

0175d0a

vercel bot deployed to staging October 8, 2019 20:14 View deployment

connorjclark marked this pull request as ready for review October 8, 2019 23:31

connorjclark requested review from brendankenny and paulirish as code owners October 8, 2019 23:31

Apply suggestions from code review

e506a63

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

vercel bot deployed to staging October 14, 2019 23:36 View deployment

supress extension errors

9274b8a

vercel bot deployed to staging October 15, 2019 18:42 View deployment

patrickhulce approved these changes Oct 15, 2019

View reviewed changes

brendankenny approved these changes Oct 15, 2019

View reviewed changes

connorjclark added 2 commits October 21, 2019 15:44

Merge remote-tracking branch 'origin/master' into flatten

7854c89

domain test

5e3c161

vercel bot deployed to staging October 21, 2019 23:07 View deployment

Merge remote-tracking branch 'origin/master' into flatten

0040b2b

vercel bot deployed to staging October 24, 2019 21:52 View deployment

Merge remote-tracking branch 'origin/master' into flatten

3fb5d12

vercel bot deployed to staging October 26, 2019 04:12 View deployment

connorjclark added the 6.0 label Oct 30, 2019

Merge remote-tracking branch 'origin/master' into flatten

64adfcd

vercel bot deployed to staging November 4, 2019 21:25 View deployment

comment

65795cc

vercel bot deployed to staging November 4, 2019 21:26 View deployment

connorjclark merged commit 7c16d34 into master Nov 4, 2019

connorjclark deleted the flatten branch November 4, 2019 21:49

connorjclark added a commit that referenced this pull request Nov 7, 2019

core: use flatten protocol (#9783)

4ba38ff

connorjclark added a commit that referenced this pull request Nov 7, 2019

core: use flatten protocol (#9783)

5dd4e59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: use flatten protocol #9783

core: use flatten protocol #9783

connorjclark commented Oct 4, 2019 •

edited

patrickhulce left a comment

patrickhulce Oct 4, 2019

connorjclark Oct 4, 2019

connorjclark commented Oct 4, 2019 •

edited

connorjclark commented Oct 8, 2019

connorjclark commented Oct 8, 2019 •

edited

connorjclark commented Oct 8, 2019

connorjclark commented Oct 8, 2019

connorjclark commented Oct 8, 2019 •

edited

connorjclark commented Oct 8, 2019 •

edited

connorjclark commented Oct 14, 2019

patrickhulce commented Oct 14, 2019

paulirish commented Oct 15, 2019

patrickhulce left a comment

patrickhulce Oct 15, 2019

brendankenny Oct 15, 2019

connorjclark Oct 21, 2019

brendankenny left a comment

brendankenny Oct 15, 2019

connorjclark Oct 15, 2019

brendankenny Oct 15, 2019

connorjclark Oct 15, 2019

brendankenny Oct 15, 2019

brendankenny Oct 15, 2019

patrickhulce Oct 15, 2019

brendankenny Oct 15, 2019

connorjclark commented Oct 21, 2019

connorjclark commented Nov 4, 2019 •

edited

brendankenny commented Nov 4, 2019

core: use flatten protocol #9783

core: use flatten protocol #9783

Conversation

connorjclark commented Oct 4, 2019 • edited

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Oct 4, 2019 • edited

connorjclark commented Oct 8, 2019

connorjclark commented Oct 8, 2019 • edited

connorjclark commented Oct 8, 2019

connorjclark commented Oct 8, 2019

connorjclark commented Oct 8, 2019 • edited

connorjclark commented Oct 8, 2019 • edited

connorjclark commented Oct 14, 2019

patrickhulce commented Oct 14, 2019

paulirish commented Oct 15, 2019

patrickhulce 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

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

connorjclark commented Oct 21, 2019

connorjclark commented Nov 4, 2019 • edited

brendankenny commented Nov 4, 2019

connorjclark commented Oct 4, 2019 •

edited

connorjclark commented Oct 4, 2019 •

edited

connorjclark commented Oct 8, 2019 •

edited

connorjclark commented Oct 8, 2019 •

edited

connorjclark commented Oct 8, 2019 •

edited

connorjclark commented Nov 4, 2019 •

edited