Skip to content

Commit

Permalink
ref(profiling) Fix electron crash (#14216)
Browse files Browse the repository at this point in the history
This PR fixes the electron crash observed in
#13978

I am not entirely sure as to why this causes a sigabrt, so I am working
around the issue (obtaining a coredump out of electron did not seem
trivial and my knowledge around electron isn't very extensive). The v8
options class and the constants are exposed correctly, I ruled that out,
however the crash still seems to happen when they are used in this
specific signature.

In order to have this running with electron, users will require to use
the electron/rebuild package, which is the recommended approach by
electron that rebuilds native node addons by providing the correct abi
headers for the electron version the user is running. For now, we wont
provide any prebuilt binaries and instead rely on the fallback mechanism
to load the correct module. I will reevaluate this if it causes issues
with bundling and look to add proper runtime electron detection.
  • Loading branch information
JonasBa authored Nov 13, 2024
1 parent a55e2b0 commit b50aefd
Show file tree
Hide file tree
Showing 7 changed files with 114 additions and 8 deletions.
21 changes: 19 additions & 2 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1235,7 +1235,7 @@ jobs:
(needs.job_get_metadata.outputs.is_release == 'true')
)
needs: [job_get_metadata, job_build, job_e2e_prepare]
runs-on: ubuntu-20.04
runs-on: ubuntu-22.04
timeout-minutes: 15
env:
E2E_TEST_AUTH_TOKEN: ${{ secrets.E2E_TEST_AUTH_TOKEN }}
Expand All @@ -1255,19 +1255,24 @@ jobs:
uses: actions/checkout@v4
with:
ref: ${{ env.HEAD_COMMIT }}

- uses: pnpm/action-setup@v4
with:
version: 9.4.0

- name: Set up Node
uses: actions/setup-node@v4
with:
node-version: 22

- name: Restore caches
uses: ./.github/actions/restore-cache
with:
dependency_cache_key: ${{ needs.job_build.outputs.dependency_cache_key }}

- name: Build Profiling Node
run: yarn lerna run build:lib --scope @sentry/profiling-node

- name: Extract Profiling Node Prebuilt Binaries
uses: actions/download-artifact@v4
with:
Expand Down Expand Up @@ -1306,6 +1311,18 @@ jobs:
env:
E2E_TEST_PUBLISH_SCRIPT_NODE_VERSION: ${{ steps.versions.outputs.node }}

- name: Setup xvfb and update ubuntu dependencies
run: |
sudo apt-get install xvfb x11-xkb-utils xfonts-100dpi xfonts-75dpi xfonts-scalable xfonts-cyrillic x11-apps
sudo apt-get install build-essential clang libdbus-1-dev libgtk2.0-dev \
libnotify-dev libgconf2-dev \
libasound2-dev libcap-dev libcups2-dev libxtst-dev \
libxss1 libnss3-dev gcc-multilib g++-multilib
- name: Install dependencies
working-directory: dev-packages/e2e-tests/test-applications/${{ matrix.test-application }}
run: yarn install --ignore-engines --frozen-lockfile

- name: Build E2E app
working-directory: dev-packages/e2e-tests/test-applications/${{ matrix.test-application }}
timeout-minutes: 7
Expand All @@ -1314,7 +1331,7 @@ jobs:
- name: Run E2E test
working-directory: dev-packages/e2e-tests/test-applications/${{ matrix.test-application }}
timeout-minutes: 10
run: yarn test:assert
run: xvfb-run --auto-servernum --server-args="-screen 0 1280x960x24" -- yarn test:assert

job_required_jobs_passed:
name: All required jobs passed or were skipped
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
const process = require('process');
const { test, expect, _electron: electron } = require('@playwright/test');

test('an h1 contains hello world"', async () => {
const electronApp = await electron.launch({
args: ['./index.electron.js'],
process: {
env: {
...process.env,
},
},
});

// Wait for the first BrowserWindow to open
const window = await electronApp.firstWindow();

// Check for the presence of an h1 element with the text "hello"
const headerElement = await window.$('h1');
const headerText = await headerElement.textContent();
expect(headerText).toBe('Hello From Profiled Electron App');

// Close the app
await electronApp.close();
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Modules to control application life and create native browser window
const { app, BrowserWindow } = require('electron');
const Sentry = require('@sentry/electron/main');
const path = require('node:path');

Sentry.init({
dsn: 'https://7fa19397baaf433f919fbe02228d5470@o1137848.ingest.sentry.io/6625302',
debug: true,
tracesSampleRate: 1.0,
});

// Hog the cpu for a second
function block() {
const start = Date.now();
while (start + 1000 > Date.now()) {}
}

const createWindow = () => {
block();
// Create the browser window.
const mainWindow = new BrowserWindow({
width: 800,
height: 600,
webPreferences: {
preload: path.join(__dirname, 'preload.js'),
},
});

// and load the index.html of the app.
mainWindow.loadFile('index.html');
block();
};

// This method will be called when Electron has finished
// initialization and is ready to create browser windows.
// Some APIs can only be used after this event occurs.
app.whenReady().then(() => {
Sentry.profiler.startProfiler();
createWindow();
Sentry.profiler.stopProfiler();

app.on('activate', () => {
// On macOS it's common to re-create a window in the app when the
// dock icon is clicked and there are no other windows open.
if (BrowserWindow.getAllWindows().length === 0) createWindow();
});
});

// Quit when all windows are closed, except on macOS. There, it's common
// for applications and their menu bar to stay active until the user quits
// explicitly with Cmd + Q.
app.on('window-all-closed', () => {
if (process.platform !== 'darwin') app.quit();
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<!DOCTYPE html>
<html>
<body>
<h1>Hello From Profiled Electron App</h1>
</body>
</html>
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,17 @@
"build": "node build.mjs && node build.shimmed.mjs",
"test": "node dist/index.js && node --experimental-require-module dist/index.js && node dist/index.shimmed.mjs",
"clean": "npx rimraf node_modules dist",
"test:build": "npm run typecheck && npm run build",
"test:assert": "npm run test"
"test:electron": "$(pnpm bin)/electron-rebuild && playwright test",
"test:build": "pnpm run typecheck && pnpm run build",
"test:assert": "pnpm run test && pnpm run test:electron"
},
"dependencies": {
"@electron/rebuild": "^3.7.0",
"@playwright/test": "^1.48.2",
"@sentry/electron": "latest || *",
"@sentry/node": "latest || *",
"@sentry/profiling-node": "latest || *"
"@sentry/profiling-node": "latest || *",
"electron": "^33.2.0"
},
"devDependencies": {},
"volta": {
Expand Down
Binary file not shown.
6 changes: 3 additions & 3 deletions packages/profiling-node/bindings/cpu_profiler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -333,9 +333,8 @@ void SentryProfile::Start(Profiler *profiler) {

// Initialize the CPU Profiler
profiler->cpu_profiler->StartProfiling(
profile_title,
{v8::CpuProfilingMode::kCallerLineNumbers,
v8::CpuProfilingOptions::kNoSampleLimit, kSamplingInterval});
profile_title, v8::CpuProfilingMode::kCallerLineNumbers, true,
v8::CpuProfilingOptions::kNoSampleLimit);

// listen for memory sample ticks
profiler->measurements_ticker.add_cpu_listener(id, cpu_sampler_cb);
Expand Down Expand Up @@ -1169,6 +1168,7 @@ napi_value Init(napi_env env, napi_value exports) {
}

Profiler *profiler = new Profiler(env, isolate);
profiler->cpu_profiler->SetSamplingInterval(kSamplingInterval);

if (napi_set_instance_data(env, profiler, FreeAddonData, NULL) != napi_ok) {
napi_throw_error(env, nullptr, "Failed to set instance data for profiler.");
Expand Down

0 comments on commit b50aefd

Please sign in to comment.