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

ref(profiling) Fix electron crash #14216

Merged
merged 14 commits into from
Nov 13, 2024
21 changes: 19 additions & 2 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1234,7 +1234,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 @@ -1254,19 +1254,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 @@ -1305,6 +1310,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 @@ -1313,7 +1330,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,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear why using the options signature causes this to sigabrt, but this will do too.

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
Loading