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.

Sign up for GitHub

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

Jump to bottom

electron - limit crash reporter to few insiders users #132961

Closed
wants to merge 2 commits into from
Closed

electron - limit crash reporter to few insiders users #132961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b5c4ba
electron - limit crash reporter to few insiders users
bpasero Sep 13, 2021
12799b9
Merge branch 'main' into ben/120902
bpasero Sep 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 51 additions & 33 deletions src/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,20 +47,19 @@ const codeCachePath = getCodeCachePath();
const argvConfig = configureCommandlineSwitchesSync(args);

// Configure crash reporter
perf.mark('code/willStartCrashReporter');
// If a crash-reporter-directory is specified we store the crash reports
// in the specified directory and don't upload them to the crash server.
//
// Appcenter crash reporting is enabled if
// * enable-crash-reporter runtime argument is set to 'true'
// * --disable-crash-reporter command line parameter is not set
// * insiders quality is used
// * the crash reporter ID ends with a 0
//
// Disable crash reporting in all other cases.
if (args['crash-reporter-directory'] ||
(argvConfig['enable-crash-reporter'] && !args['disable-crash-reporter'])) {
if (args['crash-reporter-directory'] || (argvConfig['enable-crash-reporter'] && !args['disable-crash-reporter'])) {
configureCrashReporter();
}
perf.mark('code/didStartCrashReporter');

// Set logs path before app 'ready' event if running portable
// to ensure that no 'logs' folder is created on disk at a
Expand Down Expand Up @@ -312,9 +311,11 @@ function getArgvConfigPath() {
}

function configureCrashReporter() {
let enableCrashReporter = false;
let submitURL = '';

// Always enable crash reporter for a dedicated directory if specified
let crashReporterDirectory = args['crash-reporter-directory'];
let submitURL = '';
if (crashReporterDirectory) {
crashReporterDirectory = path.normalize(crashReporterDirectory);

Expand All @@ -336,19 +337,24 @@ function configureCrashReporter() {
// need to change that directory to the provided one
console.log(`Found --crash-reporter-directory argument. Setting crashDumps directory to be '${crashReporterDirectory}'`);
app.setPath('crashDumps', crashReporterDirectory);

enableCrashReporter = true;
}

// Otherwise we configure the crash reporter from product.json
// Otherwise we configure the crash reporter from product.json if enabled
else {
const appCenter = product.appCenter;
if (appCenter) {
const isWindows = (process.platform === 'win32');
const isLinux = (process.platform === 'linux');
const isDarwin = (process.platform === 'darwin');
const crashReporterId = argvConfig['crash-reporter-id'];
const uuidPattern = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
if (uuidPattern.test(crashReporterId)) {
if (isWindows) {
const crashReporterId = argvConfig['crash-reporter-id'];
const uuidPattern = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
if (
appCenter && // only when appCenter is configured
product.quality === 'insider' && // only for insiders
typeof crashReporterId === 'string' && // only if we have a crash reporter ID
uuidPattern.test(crashReporterId) && // only if the crash reporter ID is valid
crashReporterId.endsWith('0') // only if the crash reporter ID ends with `0`
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a heuristic I picked, but maybe you have a better idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

@deepak1556 I ran some experiments generating 30000 UUIDs to roughly simulate our number of present insider users and typically end up with a number around ~1900 that match. Would you think that number if sufficient to find crashes?

) {
switch (process.platform) {
case 'win32':
switch (process.arch) {
case 'ia32':
submitURL = appCenter['win32-ia32'];
Expand All @@ -360,7 +366,8 @@ function configureCrashReporter() {
submitURL = appCenter['win32-arm64'];
break;
}
} else if (isDarwin) {
break;
case 'darwin':
if (product.darwinUniversalAssetId) {
submitURL = appCenter['darwin-universal'];
} else {
Expand All @@ -373,37 +380,48 @@ function configureCrashReporter() {
break;
}
}
} else if (isLinux) {
break;
case 'linux':
submitURL = appCenter['linux-x64'];
}
submitURL = submitURL.concat('&uid=', crashReporterId, '&iid=', crashReporterId, '&sid=', crashReporterId);
// Send the id for child node process that are explicitly starting crash reporter.
// For vscode this is ExtensionHost process currently.
const argv = process.argv;
const endOfArgsMarkerIndex = argv.indexOf('--');
if (endOfArgsMarkerIndex === -1) {
argv.push('--crash-reporter-id', crashReporterId);
} else {
// if the we have an argument "--" (end of argument marker)
// we cannot add arguments at the end. rather, we add
// arguments before the "--" marker.
argv.splice(endOfArgsMarkerIndex, 0, '--crash-reporter-id', crashReporterId);
}
break;
}

submitURL = `${submitURL}&uid=${crashReporterId}&iid=${crashReporterId}&sid=${crashReporterId}`;

// Send the id for child node process that are explicitly starting crash reporter.
// For vscode this is ExtensionHost process currently.
const argv = process.argv;
const endOfArgsMarkerIndex = argv.indexOf('--');
if (endOfArgsMarkerIndex === -1) {
argv.push('--crash-reporter-id', crashReporterId);
} else {
// if the we have an argument "--" (end of argument marker)
// we cannot add arguments at the end. rather, we add
// arguments before the "--" marker.
argv.splice(endOfArgsMarkerIndex, 0, '--crash-reporter-id', crashReporterId);
}

enableCrashReporter = true;
}
}

// Start crash reporter for all processes
const productName = (product.crashReporter ? product.crashReporter.productName : undefined) || product.nameShort;
const companyName = (product.crashReporter ? product.crashReporter.companyName : undefined) || 'Microsoft';
if (!enableCrashReporter) {
return; // crash reporter is not enabled
}

const productName = product.crashReporter?.productName || product.nameShort;
const companyName = product.crashReporter?.companyName || 'Microsoft';
const uploadToServer = !process.env['VSCODE_DEV'] && submitURL && !crashReporterDirectory;

perf.mark('code/willStartCrashReporter');
crashReporter.start({
companyName,
productName: process.env['VSCODE_DEV'] ? `${productName} Dev` : productName,
submitURL,
uploadToServer,
compress: true
});
perf.mark('code/didStartCrashReporter');
}

/**
Expand Down
3 changes: 0 additions & 3 deletions src/vs/platform/environment/common/environmentService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,9 +222,6 @@ export abstract class AbstractNativeEnvironmentService implements INativeEnviron
@memoize
get serviceMachineIdResource(): URI { return joinPath(URI.file(this.userDataPath), 'machineid'); }

get crashReporterId(): string | undefined { return this.args['crash-reporter-id']; }
get crashReporterDirectory(): string | undefined { return this.args['crash-reporter-directory']; }

get driverHandle(): string | undefined { return this.args['driver']; }

@memoize
Expand Down
3 changes: 3 additions & 0 deletions src/vs/workbench/services/environment/electron-sandbox/environmentService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,9 @@ export class NativeWorkbenchEnvironmentService extends AbstractNativeEnvironment
return this.configuration.os;
}

get crashReporterId(): string | undefined { return this.args['crash-reporter-id']; }
get crashReporterDirectory(): string | undefined { return this.args['crash-reporter-directory']; }

constructor(
readonly configuration: INativeWorkbenchConfiguration,
productService: IProductService
Expand Down
46 changes: 26 additions & 20 deletions src/vs/workbench/services/extensions/electron-browser/localProcessExtensionHost.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,28 +214,34 @@ export class LocalProcessExtensionHost implements IExtensionHost {
opts.execArgv.unshift(`--max-old-space-size=${this._environmentService.args['max-memory']}`);
}

// On linux crash reporter needs to be started on child node processes explicitly
// On linux crash reporter needs to be started on
// child node processes explicitly if enabled either
// because a crash directory is specified or a crashId.
if (platform.isLinux) {
const crashReporterStartOptions: CrashReporterStartOptions = {
companyName: this._productService.crashReporter?.companyName || 'Microsoft',
productName: this._productService.crashReporter?.productName || this._productService.nameShort,
submitURL: '',
uploadToServer: false
};
const crashReporterId = this._environmentService.crashReporterId; // crashReporterId is set by the main process only when crash reporting is enabled by the user.
const appcenter = this._productService.appCenter;
const uploadCrashesToServer = !this._environmentService.crashReporterDirectory; // only upload unless --crash-reporter-directory is provided
if (uploadCrashesToServer && appcenter && crashReporterId && isUUID(crashReporterId)) {
const submitURL = appcenter[`linux-x64`];
crashReporterStartOptions.submitURL = submitURL.concat('&uid=', crashReporterId, '&iid=', crashReporterId, '&sid=', crashReporterId);
crashReporterStartOptions.uploadToServer = true;
const crashReporterDirectory = this._environmentService.crashReporterDirectory;
const crashReporterId = this._environmentService.crashReporterId;
if (crashReporterDirectory || crashReporterId) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Now using these properties as a hint whether crash reporting is enabled at all.

const crashReporterStartOptions: CrashReporterStartOptions = {
companyName: this._productService.crashReporter?.companyName || 'Microsoft',
productName: this._productService.crashReporter?.productName || this._productService.nameShort,
submitURL: '',
uploadToServer: false
};

const appcenter = this._productService.appCenter;
const uploadCrashesToServer = !crashReporterDirectory; // only upload unless --crash-reporter-directory is provided
if (uploadCrashesToServer && appcenter && crashReporterId && isUUID(crashReporterId)) {
crashReporterStartOptions.submitURL = `${appcenter['linux-x64']}&uid=${crashReporterId}&iid=${crashReporterId}&sid=${crashReporterId}`;
crashReporterStartOptions.uploadToServer = true;
}

// In the upload to server case, there is a bug in electron that creates client_id file in the current
// working directory. Setting the env BREAKPAD_DUMP_LOCATION will force electron to create the file in that location,
// For https://github.com/microsoft/vscode/issues/105743
const extHostCrashDirectory = crashReporterDirectory || this._environmentService.userDataPath;
opts.env.BREAKPAD_DUMP_LOCATION = join(extHostCrashDirectory, `${ExtensionHostLogFileName} Crash Reports`);
opts.env.VSCODE_CRASH_REPORTER_START_OPTIONS = JSON.stringify(crashReporterStartOptions);
}
// In the upload to server case, there is a bug in electron that creates client_id file in the current
// working directory. Setting the env BREAKPAD_DUMP_LOCATION will force electron to create the file in that location,
// For https://github.com/microsoft/vscode/issues/105743
const extHostCrashDirectory = this._environmentService.crashReporterDirectory || this._environmentService.userDataPath;
opts.env.BREAKPAD_DUMP_LOCATION = join(extHostCrashDirectory, `${ExtensionHostLogFileName} Crash Reports`);
opts.env.VSCODE_CRASH_REPORTER_START_OPTIONS = JSON.stringify(crashReporterStartOptions);
}

// Run Extension Host as fork of current process
Expand Down