Debug error in node_mksnapshot #175
Comments
|
/cc @joyeecheung ? Do we have a team to ping for these kind of issues? |
|
No, we don't have a team for this, from a glance I am guessing this is a V8 issue, I will try to reproduce locally and see what object this is. Also cc @nodejs/v8 |
|
It looks like one of the typed arrays ( See reference stack |
|
I dug around a bit and it looks like it's not related to the global uncaught exception handler after all. As long as the serializer reaches a Uint32Array in the closure of |
|
This is affecting V8 8.7: nodejs/node#35700 (comment) |
|
Maybe it would be easier to fix if we find the V8 commit where this starts to fail. Is there a way to automate bisect between two V8 versions with Node? Or do you have closer hashes other than v8.6 and v8.7? @targos |
|
I do not have a way to bisect V8 versions with Node. The only additional info I can give you is that the oldest CI run with canary, which was on Sep 15, 2020, already showed that failure. It gives us this commit range, if I compare from the point of 8.6 branch to Sep 16: v8/v8@a626bc0...c73ca67 |
|
From the CI this also at least showed up on Sep 11, do you remember the last good commit of this? |
|
Oh, wait, the last good commit should be a626bc036236c9bf92ac7b87dc40c9e538b087e3. This gives us v8/v8@a626bc0...86e5f1a |
|
@joyeecheung where did you find this? I can only go back to Sep 15 in the history |
|
Still a lot of candidates :( |
|
How can I pull the source code of node canary (if the build file works) for a given V8 hash or a build on a particular day to build and test it? If we are lucky we just need to test 5 builds. |
|
If GitHub doesn't garbage collect the commits (we do not tag them, maybe we should?), you can find the commit hashes here: https://nodejs.org/download/v8-canary/ For example, with the build from Sep 01: |
|
That doesn't give you the V8 hash though, only the canary hash. At least this way you can reduce the list based on the day canary was updated |
|
This might be related to https://chromium-review.googlesource.com/c/v8/v8/+/2418101 - there are a bunch of relands and reverts of these CLs, so simply bisecting might not be useful |
|
The source can't be checked out from the command line with the hash. It's available on the website as tarball though |
|
Maybe worthing add debug build to V8 canary workflow like nodejs/node#35721. |
|
I get When I run When I |
|
Oops... You still are running Python 2. https://docs.python.org/2.7/library/urllib2.html should be builtin. Please do
For the second one |
|
@cclauss Thanks, I wonder if we could add our python 3 locating code in |
The gyp is in |
|
@gengjiawen I redownloaded the tarball and it worked, maybe that wasn't a proper tarball in the first place, sorry |
|
I bisected the range to v8/v8@8.7.75...8.7.80 which includes the patches mentioned in #175 (comment) I wonder if we can just relax the DCHECK because the release build seems to work just fine...still verifying the fix |
|
Update: looks like we'd need to make objects with embedder fields that have pending forward references work in the context serializer instead of just ignoring the back reference check. Before v8/v8@76d684c they worked as deferred objects with valid back references, and after they don't. Not sure why it wasn't caught in the V8 test suite (but again, similar to the filler map processing in https://chromium-review.googlesource.com/c/v8/v8/+/2212085 it's difficult to reproduce an object graph that would precisely trigger this bug, so we probably just have to watch out for the integration on our side) |
|
By the way before the fix can be backported, you can still use the debug build if you just disable our own snapshot with the configure option |
Which fix? |
|
@targos I should've typed "a fix" (trying to work it out..or maybe more because serializer patches tend to take multiple relands) |
|
I think I was overthinking and simply avoiding deferring embedder objects instead of trying to add support for it should do the trick diff --git a/deps/v8/src/snapshot/serializer-deserializer.cc b/deps/v8/src/snapshot/serializer-deserializer.cc
index afa41e7d03..b05c2f574b 100644
--- a/deps/v8/src/snapshot/serializer-deserializer.cc
+++ b/deps/v8/src/snapshot/serializer-deserializer.cc
@@ -36,7 +36,8 @@ bool SerializerDeserializer::CanBeDeferred(HeapObject o) {
// references to the now-thin string will already have been written.
// TODO(leszeks): Could we defer string serialization if forward references
// were resolved after object post processing?
- return !o.IsMap() && !o.IsInternalizedString();
+ return !o.IsMap() && !o.IsInternalizedString() &&
+ !(o.IsJSObject() && JSObject::cast(o).GetEmbedderFieldCount() > 0);
}
void SerializerDeserializer::RestoreExternalReferenceRedirector(CI: |
|
Funny that I just find this now :) |
|
https://chromium-review.googlesource.com/c/v8/v8/+/2531195 has been merged. We can wait for a day or two so that more bots in the upstream verifies it before we can backport it (I think this fix should be simple enough to not regress though) |
pull bot
pushed a commit
to ashu8912/v8
that referenced
this issue
Nov 11, 2020
JS objects with embedder fields cannot be deferred because the serialize/deserialize callbacks need the back reference immediately to identify the object. Refs: nodejs/node-v8#175 Bug: v8:11146 Change-Id: I4292f2ab0041f7b0779620437ed26905c194cd9b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2531195 Reviewed-by: Jakob Gruber <jgruber@chromium.org> Commit-Queue: Joyee Cheung <joyee@igalia.com> Cr-Commit-Position: refs/heads/master@{#71134}
|
I pushed the fix to nodejs/node#35700. Let's see how it goes. |
targos
added a commit
to targos/node
that referenced
this issue
Nov 15, 2020
Original commit message:
[serializer] avoid deferring objects with embedder fields
JS objects with embedder fields cannot be deferred because the
serialize/deserialize callbacks need the back reference immediately
to identify the object.
Refs: nodejs/node-v8#175
Bug: v8:11146
Change-Id: I4292f2ab0041f7b0779620437ed26905c194cd9b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2531195
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/master@{#71134}
Refs: v8/v8@821fb38
PR-URL: nodejs#35700
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
targos
added a commit
to targos/node
that referenced
this issue
Nov 19, 2020
Original commit message:
[serializer] avoid deferring objects with embedder fields
JS objects with embedder fields cannot be deferred because the
serialize/deserialize callbacks need the back reference immediately
to identify the object.
Refs: nodejs/node-v8#175
Bug: v8:11146
Change-Id: I4292f2ab0041f7b0779620437ed26905c194cd9b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2531195
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/master@{#71134}
Refs: v8/v8@821fb38
PR-URL: nodejs#35700
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
targos
added a commit
to targos/node
that referenced
this issue
Nov 29, 2020
Original commit message:
[serializer] avoid deferring objects with embedder fields
JS objects with embedder fields cannot be deferred because the
serialize/deserialize callbacks need the back reference immediately
to identify the object.
Refs: nodejs/node-v8#175
Bug: v8:11146
Change-Id: I4292f2ab0041f7b0779620437ed26905c194cd9b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2531195
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/master@{#71134}
Refs: v8/v8@821fb38
PR-URL: nodejs#35700
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
cjihrig
pushed a commit
to cjihrig/node
that referenced
this issue
Dec 6, 2020
Original commit message:
[serializer] avoid deferring objects with embedder fields
JS objects with embedder fields cannot be deferred because the
serialize/deserialize callbacks need the back reference immediately
to identify the object.
Refs: nodejs/node-v8#175
Bug: v8:11146
Change-Id: I4292f2ab0041f7b0779620437ed26905c194cd9b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2531195
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/master@{#71134}
Refs: v8/v8@821fb38
PR-URL: nodejs#35700
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
targos
added a commit
to targos/node
that referenced
this issue
Jan 8, 2021
Original commit message:
[serializer] avoid deferring objects with embedder fields
JS objects with embedder fields cannot be deferred because the
serialize/deserialize callbacks need the back reference immediately
to identify the object.
Refs: nodejs/node-v8#175
Bug: v8:11146
Change-Id: I4292f2ab0041f7b0779620437ed26905c194cd9b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2531195
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/master@{#71134}
Refs: v8/v8@821fb38
PR-URL: nodejs#35700
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
targos
added a commit
to targos/node
that referenced
this issue
Jan 25, 2021
Original commit message:
[serializer] avoid deferring objects with embedder fields
JS objects with embedder fields cannot be deferred because the
serialize/deserialize callbacks need the back reference immediately
to identify the object.
Refs: nodejs/node-v8#175
Bug: v8:11146
Change-Id: I4292f2ab0041f7b0779620437ed26905c194cd9b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2531195
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/master@{#71134}
Refs: v8/v8@821fb38
PR-URL: nodejs#35700
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
targos
added a commit
to targos/node
that referenced
this issue
Feb 7, 2021
Original commit message:
[serializer] avoid deferring objects with embedder fields
JS objects with embedder fields cannot be deferred because the
serialize/deserialize callbacks need the back reference immediately
to identify the object.
Refs: nodejs/node-v8#175
Bug: v8:11146
Change-Id: I4292f2ab0041f7b0779620437ed26905c194cd9b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2531195
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/master@{#71134}
Refs: v8/v8@821fb38
PR-URL: nodejs#35700
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
targos
added a commit
to targos/node
that referenced
this issue
Feb 11, 2021
Original commit message:
[serializer] avoid deferring objects with embedder fields
JS objects with embedder fields cannot be deferred because the
serialize/deserialize callbacks need the back reference immediately
to identify the object.
Refs: nodejs/node-v8#175
Bug: v8:11146
Change-Id: I4292f2ab0041f7b0779620437ed26905c194cd9b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2531195
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/master@{#71134}
Refs: v8/v8@821fb38
PR-URL: nodejs#35700
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
targos
added a commit
to targos/node
that referenced
this issue
Feb 11, 2021
Original commit message:
[serializer] avoid deferring objects with embedder fields
JS objects with embedder fields cannot be deferred because the
serialize/deserialize callbacks need the back reference immediately
to identify the object.
Refs: nodejs/node-v8#175
Bug: v8:11146
Change-Id: I4292f2ab0041f7b0779620437ed26905c194cd9b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2531195
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/master@{#71134}
Refs: v8/v8@821fb38
PR-URL: nodejs#35700
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
https://ci.nodejs.org/job/node-test-commit-linux-containered/22283/nodes=ubuntu1804_sharedlibs_debug_x64/console
The text was updated successfully, but these errors were encountered: