GoogleChrome / lighthouse

Automated auditing, performance metrics, and best practices for the web.
https://developer.chrome.com/docs/lighthouse/overview/
Apache License 2.0
28.26k stars 9.35k forks source link

Page crashes when running test from large DOM.getDocument response #15892

Open mattzeunert opened 6 months ago

mattzeunert commented 6 months ago

FAQ

URL

https://uk.elemis.com/

What happened?

Run lighthouse https://uk.elemis.com/ --view --preset=desktop.

The page crashes:

Screenshot 2024-03-26 at 12 02 27

What did you expect?

The test should finish successfully.

What have you tried?

No response

How were you running Lighthouse?

CLI

Lighthouse Version

11.7.0

Chrome Version

125.0.6379.3

Node Version

No response

OS

No response

Relevant log output

LH:status:verbose Computing artifact: TraceEngineResult +0ms
  LH:status:verbose Computing artifact: ProcessedTrace +0ms
  LH:statusEnd:verbose Computing artifact: ProcessedTrace +57ms
  LH:statusEnd:verbose Computing artifact: TraceEngineResult +110ms

(Also separately when running with Pupppeteer + dumpio:)
[0326/120724.422471:WARNING:in_range_cast.h(38)] value -634136515 out of range
connorjclark commented 6 months ago

(internal) crash/c962bbbdb7165d4f https://g-issues.chromium.org/issues/40059370

It crashes when converting the binary protocol response to JSON. The message is apparently too large.

connorjclark commented 6 months ago

This is from response of DOM.getDocument having too many nested children. I get 488. The max allowed for any response JSON depth is 300.

Debugging this required some setup so I'll record it here.


I remove the depth limit from Chrome and print out all the responses being sent out

diff --git a/third_party/blink/renderer/core/inspector/devtools_session.cc b/third_party/blink/renderer/core/inspector/devtools_session.cc
index 912eb6434368a..0c9785fe74b14 100644
--- a/third_party/blink/renderer/core/inspector/devtools_session.cc
+++ b/third_party/blink/renderer/core/inspector/devtools_session.cc
@@ -409,6 +409,12 @@ blink::mojom::blink::DevToolsMessagePtr DevToolsSession::FinalizeMessage(
         crdtp::json::ConvertCBORToJSON(crdtp::SpanFrom(message_to_send), &json);
     CHECK(status.ok()) << status.ToASCIIString();
     message_to_send = std::move(json);
+
+    if (message_to_send.size() > 300) {
+      std::string str(message_to_send.begin(), message_to_send.end());
+      LOG(ERROR) << "!!! msg";
+      LOG(ERROR) << str;
+    }
   }
   auto mojo_msg = mojom::blink::DevToolsMessage::New();
   mojo_msg->data = std::move(message_to_send);
diff --git a/third_party/inspector_protocol/crdtp/cbor.cc b/third_party/inspector_protocol/crdtp/cbor.cc
index 801c170802276..050bb079ecb7f 100644
--- a/third_party/inspector_protocol/crdtp/cbor.cc
+++ b/third_party/inspector_protocol/crdtp/cbor.cc
@@ -885,11 +885,11 @@ bool ParseEnvelope(int32_t stack_depth,
 bool ParseValue(int32_t stack_depth,
                 CBORTokenizer* tokenizer,
                 ParserHandler* out) {
-  if (stack_depth > kStackLimit) {
-    out->HandleError(
-        Status{Error::CBOR_STACK_LIMIT_EXCEEDED, tokenizer->Status().pos});
-    return false;
-  }
+  // if (stack_depth > kStackLimit) {
+  //   out->HandleError(
+  //       Status{Error::CBOR_STACK_LIMIT_EXCEEDED, tokenizer->Status().pos});
+  //   return false;
+  // }
   switch (tokenizer->TokenTag()) {
     case CBORTokenTag::ERROR_VALUE:
       out->HandleError(tokenizer->Status());

We need to get at the chrome logs

diff --git a/cli/run.js b/cli/run.js
index 6366ce6c7..f4c207751 100644
--- a/cli/run.js
+++ b/cli/run.js
@@ -88,6 +88,7 @@ function getDebuggableChrome(flags) {
     ignoreDefaultFlags: flags.chromeIgnoreDefaultFlags,
     chromeFlags: parseChromeFlags(flags.chromeFlags),
     logLevel: flags.logLevel,
+    userDataDir: '.tmp/userDataDir',
   });
 }

This script finds what the deepest CDP response message was. chrome-err.log requires some manual editing first.

import fs from 'fs';

function search(node) {
    if (Array.isArray(node)) {
        if (!node.length) return 1;
        return 1 + Math.max(...node.map(search));
    }
    if (typeof node == 'object' && node !== null) {
        let list = Array.isArray(node) ? node : Object.values(node);
        return list.length ? 1 + Math.max(...list.map(search)) : 1;
    }
    return 0;
}

const lines = fs.readFileSync('/Users/cjamcl/src/lighthouse/.tmp/userDataDir/chrome-err.log', {'encoding': 'utf-8'}).split('\n').filter(Boolean);
const objects = [];
for (const line of lines) {
    try {
        objects.push(JSON.parse(line));
    } catch {
        console.log({line});
        process.exit(1);
    }
}
const max = search(objects);

for (const object of objects) {
    console.log({max});
    if (search(object) == max - 1) {
        console.log(object);
        process.exit(1);
    }
}
connorjclark commented 6 months ago

btw, not specific to root causes/trace engine. it's any usage of DOM.getDocument. still repros with --only-audits crawlable-anchors

connorjclark commented 6 months ago

possible fix is to bump this: https://source.chromium.org/chromium/chromium/src/+/main:third_party/inspector_protocol/crdtp/cbor.cc;l=817?q=cbor.cc&ss=chromium

last done here: https://chromium.googlesource.com/deps/inspector_protocol/+/454b6bcf7d08535681bbd967030f5248bcbc8e6a%5E%21

From looking at chrome crash graphs, this seems very rare, so I'm marking as P3. However, this seems like a straightforward change for any interested chromium contributor to make - except for the fact I couldn't find any history of discussion about why this limit is (imo) so low. Ramifications of increasing are not known to me. Perhaps just increasing the outgoing messages encoder stackLimit would be safer than also increasing the incoming (that's in a different file).