Snapshot test failures

[targos]

trafficstars

https://github.com/nodejs/node-v8/runs/7761703491?check_suite_focus=true

/cc @joyeecheung

[targos]

For example, one of the errors:

node:assert:124
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

null !== 0

    at Object.<anonymous> (/home/runner/work/node-v8/node-v8/test/parallel/test-snapshot-warning.js:40:12)
    at Module._compile (node:internal/modules/cjs/loader:1119:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1173:10)
    at Module.load (node:internal/modules/cjs/loader:997:32)
    at Module._load (node:internal/modules/cjs/loader:838:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:86:12)
    at node:internal/main/run_main_module:23:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: null,
  expected: 0,
  operator: 'strictEqual'
}

Node.js v19.0.0-pre
--- stdout ---
# Check snapshot scripts that do not emit warnings.
[stderr]: Unknown external reference 0x55c318be73b0.
<unresolved>

[stdout]: 
SIGTRAP
Command: out/Release/node /home/runner/work/node-v8/node-v8/test/parallel/test-snapshot-warning.js

[joyeecheung]

Looks like it's caused by an unknown external reference somewhere. I'll try to build it locally to see what the reference is. Do I simply need to build the canary branch from this repo?

[targos]

Do I simply need to build the canary branch from this repo?

Yes

[joyeecheung]

Traced this back to https://chromium-review.googlesource.com/c/v8/v8/+/3776678, which added a v8::External to the V8Console, and this band-aid makes the crash go away

diff --git a/lib/internal/main/mksnapshot.js b/lib/internal/main/mksnapshot.js
index c4ea3a06cf..47e0e09495 100644
--- a/lib/internal/main/mksnapshot.js
+++ b/lib/internal/main/mksnapshot.js
@@ -127,6 +127,7 @@ function main() {

   require('internal/v8/startup_snapshot').initializeCallbacks();

+  delete console.createTask;
   if (getOptionValue('--inspect-brk')) {
     internalBinding('inspector').callAndPauseOnStart(
       serializeMainFunction, undefined,

I'll see if we can do something like what's been done for v8::Isolate::async_event_delegate and resolve this within v8

[joyeecheung]

After looking into it a bit I think we should simply remove the inspector console methods during serialization because V8 is always going to re-install them when another inspector client is created during deserialization - with a new reference to the new inspector console. I created a commit that works on both the canary branch here and the main branch in the main repo (these methods are renamed in the new version of V8 so we can't hard-code the methods to remove) at https://github.com/joyeecheung/node/tree/fix-createtask, which depends on https://github.com/nodejs/node/pull/44203 (which is also necessary to fix the two additional failures in snapshot-warning and snapshot-typescript tests in the canary, as the newer version of V8 is also less permissive with re-installing Error.stackTraceLimit in the release builds). I'll open a PR to the upstream after https://github.com/nodejs/node/pull/44203 lands.

[joyeecheung]

Opened https://github.com/nodejs/node/pull/44279