Crash in mksnapshot when building with Electron 4

[nathansobo]

When attempting to upgrade to Electron 4 in #19373, we fail the build with the following error when attempting to generate the startup snapshot. (I modified the build script slightly to pass stdio: "inherit" to mksnapshot for better error output)

Minifying startup script
Verifying if snapshot can be executed via `mksnapshot`
Generating startup blob with mksnapshot
Loading script for embedding: /Users/nathan/src/atom/out/startup.js
Error running mksnapshot.
Moving generated startup blob into "/Users/nathan/src/atom/out/Atom Dev.app/Contents/Frameworks/Electron Framework.framework/Resources/v8_context_snapshot.bin"
Error: ENOENT: no such file or directory, rename '/Users/nathan/src/atom/out/v8_context_snapshot.bin' -> '/Users/nathan/src/atom/out/Atom Dev.app/Contents/Frameworks/Electron Framework.framework/Resources/v8_context_snapshot.bin'
  at Object.fs.renameSync (fs.js:766:18)
  at electronLink.then (/Users/nathan/src/atom/script/lib/generate-startup-snapshot.js:137:10)
  at <anonymous>:null:null

After hacking some logging into electron-mksnapshot, it appears that the mksnapshot is crashing with a segmentation fault. In the Node wrapper script, the exit code on the child process is null and the signal is SEGV.

Running mksnapshot directly as follows also produces a segfault, although electron-mksnaphsot seems to do some special magic to move files to a single working directory, so it's unclear whether this is even a good way to run mksnapshot.

Nathans-MacBook-Pro:atom nathan$ script/node_modules/electron-mksnapshot/bin/mksnapshot --startup_blob snapshot_blob.bin --turbo_instruction_scheduling /Users/nathan/src/atom/out/startup.js
Loading script for embedding: /Users/nathan/src/atom/out/startup.js
Segmentation fault: 11
Nathans-MacBook-Pro:atom nathan$ echo $?
139

Trying the latest electron-mksnapshot

I tried to generate the startup snapshot with electron-mksnapshot@6.0.0-beta.1 and got a bit further. I was able to generate snapshot_blob.bin, but the script failed when invoking v8_context_snapshot_generator. The output in this failure mode is pretty bad... the script just prints out the entire child process object, so I'm eliding some of its details here for readability.

Nathans-MacBook-Pro:atom nathan$ cat /Users/nathan/.nvm/versions/node/v8.15.1/lib/node_modules/electron-mksnapshot/package.json | grep "\"version\":"
  "version": "6.0.0-beta.1"
Nathans-MacBook-Pro:atom nathan$ /Users/nathan/.nvm/versions/node/v8.15.1/lib/node_modules/electron-mksnapshot/mksnapshot.js /Users/nathan/src/atom/out/startup.js
Loading script for embedding: /Users/nathan/src/atom/out/startup.js
Error running the v8 context snapshot generator. { status: null,
  signal: 'SIGTRAP',
  output: [ null, null, null ],
  pid: 94008,
  stdout: null,
  stderr: null,
  envPairs: ELIDED
  options:        { cwd: '/Users/nathan/.nvm/versions/node/v8.15.1/lib/node_modules/electron-mksnapshot/bin',
     env: ELIDED
     args: [ '/var/folders/wy/zs1k37fn0d95lsfcv2d3z6m80000gn/T/mksnapshot-workdir119420-94006-vcutmj.h9pos/v8_context_snapshot_generator',
     '--output_file=/Users/nathan/src/atom/v8_context_snapshot.bin' ],
  file: '/var/folders/wy/zs1k37fn0d95lsfcv2d3z6m80000gn/T/mksnapshot-workdir119420-94006-vcutmj.h9pos/v8_context_snapshot_generator' }

I'm not sure where to go from here. To some degree, we're abusing V8 startup snapshots in Atom. They weren't designed for public consumption, and the snapshot that ships with Chrome is much smaller and probably way simpler than what we are attempting in Atom's startup.js script.

cc @MarshallOfSound

[50Wliu]

Yeah, I get exit code 3221225477 reported through Node's child_process, and -1073741819 from cmd when trying to run mksnapshot.

[nathansobo]

I ran mksnapshot under LLDB in a temporary working directory as the electron-mksnapshot script does, but didn't come up with much. We may need a debug build:

Nathans-MacBook-Pro:mksnapshot-workdir119420-2853-qa1u24.krcnm nathan$ lldb ./mksnapshot -- ~/src/atom/out/startup.js --startup_blob snapshot_blob.bin  --turbo_instruction_scheduling
(lldb) target create "./mksnapshot"
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "/usr/local/Cellar/python@2/2.7.16/Frameworks/Python.framework/Versions/2.7/lib/python2.7/copy.py", line 52, in <module>
    import weakref
  File "/usr/local/Cellar/python@2/2.7.16/Frameworks/Python.framework/Versions/2.7/lib/python2.7/weakref.py", line 14, in <module>
    from _weakref import (
ImportError: cannot import name _remove_dead_weakref
Current executable set to './mksnapshot' (x86_64).
(lldb) settings set -- target.run-args  "/Users/nathan/src/atom/out/startup.js" "--startup_blob" "snapshot_blob.bin" "--turbo_instruction_scheduling"
(lldb) run
Process 2897 launched: '/var/folders/wy/zs1k37fn0d95lsfcv2d3z6m80000gn/T/mksnapshot-workdir119420-2853-qa1u24.krcnm/mksnapshot' (x86_64)
Loading script for embedding: /Users/nathan/src/atom/out/startup.js
Process 2897 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x0000000000000000
error: memory read failed for 0x0
Target 0: (mksnapshot) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000000000000
    frame #1: 0x00000001005728f4 mksnapshot`___lldb_unnamed_symbol10134$$mksnapshot + 820
    frame #2: 0x00000001005725a0 mksnapshot`___lldb_unnamed_symbol10133$$mksnapshot + 384
    frame #3: 0x000000010068b0d4 mksnapshot`v8::SnapshotCreator::CreateBlob(v8::SnapshotCreator::FunctionCodeHandling) + 3060
    frame #4: 0x0000000100000e5f mksnapshot`___lldb_unnamed_symbol1$$mksnapshot + 607
    frame #5: 0x00007fff715af3d5 libdyld.dylib`start + 1

[BinaryMuse]

/cc @jkleinsc for info

[nathansobo]

As discussed with @BinaryMuse, as painful as this is, perhaps our best bet is to bisect the files we include in startup.js and try to narrow down the culprit(s).

[nathansobo]

@rafeca found that we're including Atom's package.json in the startup script, which is a very large object. Apparently removing it allows the startup snapshot to build.

[rafeca]

This is the actual object that we're including: https://gist.github.com/rafeca/49379b5a0af04e542504b098b7ec3197

This object has a _atomModuleCache key with a ton of information:

• dependencies: An array with an object for each npm dependency. This object contains 3 keys:
  • name: the npm package name.
  • version: the resolved version for that package.
  • path: the main file for that package.
• folders: An array with an object for each package found in Atom (a package is a folder with a package.json). This object contains 2 keys:
  • paths: an array with all the subfolders of that package.
  • dependencies: an object with all the npm dependencies of that package (key is the npm package name and value is the semver range for that dependency).
• extensions: An object with all the project files indexed by extension (key is the extension and value is an array of files).

Apart from this, in that huge object there's a _atomPackages key which contains every single bundled package metadata (including the _atomModuleCache information for each bundled package).

Why does _atomModuleCache exists?

I still have to understand why this _atomModuleCache object is created and how is it used (haven't been able to find any documentation that explains it, so I'm going to dive into the code).

So far, by grepping I've fond these places where it's used in atom/atom:

1. src/module-cache.js: This is where this object is created and saved into the package.json file (this was originally added in Cache requires across installed packages #3761).
2. src/generate-module-cache.js: Looks just like a hack to modify the folder object of atom/atom (this was added in Replace Grunt-based build system with simple scripts #12410, but that PR has >200 commits with no specific information about the module cache).
3. script/lib/generate-metadata.js: This seems to be similar than the previous usage: it's just mutating the metadata to remove the package.json file from the list of files with json extension (this was added in Replace Grunt-based build system with simple scripts #12410, but that PR has >200 commits with no specific information about the module cache).
4. src/package.js: Here the extensions object is used to speed up finding all the native modules (files with .node extension) and require them to check if they are compatible with the current Atom version (this was added in Read _atomModuleCache to get all the native dependencies while initializing package #8826).

So all of this originally comes from #3761, which aims to speed up the startup time by caching parts of the resolution of JS files (since Node.js resolution logic is so horribly complicated and slow).

I wonder though if this strategy is still impactful (or even if it makes more harm than good) taking into account that we're currently using v8 snapshots.

[as-cii]

I think this may have something to do with the work I recently did on Electron 3 (see electron/electron#18420). mksnapshot may be segfaulting due to creating a Promise during Atom's preload phase. The use of Promise objects should be avoided, as the Node runtime requires promises to contain an internal field (nodejs/node#13242), which makes them unsuitable for serialization. In the electron-3.1 branch, I worked around the problem by avoiding to create a Promise altogether (see 95fe952).

I suspect that modifying package.json to remove _atomPackages is preventing packages from being preloaded and, as such, may be hiding the underlying promise creation issue illustrated above. Perhaps it would be worth cherry-picking 95fe952 into #19373 without further changes and checking whether it resolves the issue?

[as-cii]

For what it's worth, I just tested running Electron 4's and Electron 5's mksnapshot on the file generated in the electron-3.1 branch with the modification suggested above, and it successfully generates a snapshot on my machine. So I think applying 95fe952 to #19373 will fix the issue.

That fix is something that we will need in Electron 3.1 as well, so it may be sufficient to update from master once that branch is merged (we are currently waiting on a hotfix after electron/electron#18426 goes green).

[rafeca]

Awesome @as-cii ! If 95fe952 is independant of the electron 3.1 upgrade, can we just create a PR with this commit and merge it into master?

[lock]

This issue has been automatically locked since there has not been any recent activity after it was closed. If you can still reproduce this issue in Safe Mode then please open a new issue and fill out the entire issue template to ensure that we have enough information to address your issue. Thanks!