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.

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

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

Open
2 tasks done
mattzeunert opened this issue Mar 26, 2024 · 4 comments
Open
2 tasks done

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

mattzeunert opened this issue Mar 26, 2024 · 4 comments
Assignees
Labels

Comments

@mattzeunert
Copy link
Collaborator

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
Copy link
Collaborator

connorjclark commented Mar 26, 2024

(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
Copy link
Collaborator

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
Copy link
Collaborator

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

@connorjclark connorjclark changed the title Page crashes when running test (TraceEngine related?) Page crashes when running test from large DOM.getDocument response Mar 26, 2024
@connorjclark
Copy link
Collaborator

connorjclark commented Mar 26, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants