Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { map, truncate } from 'lodash';
import open from 'opn';
import { ElementHandle, EvaluateFn, Page, Response, SerializableOrJSHandle } from 'puppeteer';
import { parse as parseUrl } from 'url';
import { getDisallowedOutgoingUrlError } from '../';
import { LevelLogger } from '../../../lib';
import { ViewZoomWidthHeight } from '../../../lib/layouts/layout';
import { ConditionalHeaders, ElementPosition } from '../../../types';
Expand Down Expand Up @@ -76,6 +77,9 @@ export class HeadlessChromiumDriver {
});
}

/*
* Call Page.goto and wait to see the Kibana DOM content
*/
public async open(
url: string,
{
Expand Down Expand Up @@ -113,6 +117,16 @@ export class HeadlessChromiumDriver {
logger.info(`handled ${this.interceptedCount} page requests`);
}

/*
* Let modules poll if Chrome is still running so they can short circuit if needed
*/
public isPageOpen() {
return !this.page.isClosed();
}

/*
* Call Page.screenshot and return a base64-encoded string of the image
*/
public async screenshot(elementPosition: ElementPosition): Promise<string> {
const { boundingClientRect, scroll } = elementPosition;
const screenshot = await this.page.screenshot({
Expand Down Expand Up @@ -220,18 +234,13 @@ export class HeadlessChromiumDriver {

// We should never ever let file protocol requests go through
if (!allowed || !this.allowRequest(interceptedUrl)) {
logger.error(`Got bad URL: "${interceptedUrl}", closing browser.`);
await client.send('Fetch.failRequest', {
errorReason: 'Aborted',
requestId,
});
this.page.browser().close();
throw new Error(
i18n.translate('xpack.reporting.chromiumDriver.disallowedOutgoingUrl', {
defaultMessage: `Received disallowed outgoing URL: "{interceptedUrl}", exiting`,
values: { interceptedUrl },
})
);
logger.error(getDisallowedOutgoingUrlError(interceptedUrl));
return;
}

if (this._shouldUseCustomHeaders(conditionalHeaders.conditions, interceptedUrl)) {
Expand Down Expand Up @@ -292,9 +301,9 @@ export class HeadlessChromiumDriver {
}

if (!allowed || !this.allowRequest(interceptedUrl)) {
logger.error(`Got disallowed URL "${interceptedUrl}", closing browser.`);
this.page.browser().close();
throw new Error(`Received disallowed URL in response: ${interceptedUrl}`);
logger.error(getDisallowedOutgoingUrlError(interceptedUrl));
throw getDisallowedOutgoingUrlError(interceptedUrl);
}
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
import * as Rx from 'rxjs';
import { InnerSubscriber } from 'rxjs/internal/InnerSubscriber';
import { ignoreElements, map, mergeMap, tap } from 'rxjs/operators';
import { getChromiumDisconnectedError } from '../';
import { BROWSER_TYPE } from '../../../../common/constants';
import { CaptureConfig } from '../../../../server/types';
import { LevelLogger } from '../../../lib';
Expand Down Expand Up @@ -115,13 +116,19 @@ export class HeadlessChromiumDriverFactory {

logger.debug(`Browser page driver created`);
} catch (err) {
observer.error(new Error(`Error spawning Chromium browser: [${err}]`));
observer.error(new Error(`Error spawning Chromium browser!`));
observer.error(err);
Copy link
Member Author

Choose a reason for hiding this comment

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

Stringifying error objects in a message has shown to cause problems where the error context or error data gets lost. I searched the code to see if this is happening in other places and fixed them in this PR.

throw err;
}

const childProcess = {
async kill() {
await browser.close();
try {
await browser.close();
} catch (err) {
// do not throw
logger.error(err);
}
},
};
const { terminate$ } = safeChildProcess(logger, childProcess);
Expand Down Expand Up @@ -167,7 +174,8 @@ export class HeadlessChromiumDriverFactory {
// the unsubscribe function isn't `async` so we're going to make our best effort at
// deleting the userDataDir and if it fails log an error.
del(userDataDir, { force: true }).catch((error) => {
logger.error(`error deleting user data directory at [${userDataDir}]: [${error}]`);
logger.error(`error deleting user data directory at [${userDataDir}]!`);
logger.error(error);
});
});
});
Expand Down Expand Up @@ -219,7 +227,7 @@ export class HeadlessChromiumDriverFactory {
mergeMap((err) => {
return Rx.throwError(
i18n.translate('xpack.reporting.browsers.chromium.errorDetected', {
defaultMessage: 'Reporting detected an error: {err}',
defaultMessage: 'Reporting encountered an error: {err}',
values: { err: err.toString() },
})
);
Expand All @@ -230,23 +238,15 @@ export class HeadlessChromiumDriverFactory {
mergeMap((err) => {
return Rx.throwError(
i18n.translate('xpack.reporting.browsers.chromium.pageErrorDetected', {
defaultMessage: `Reporting detected an error on the page: {err}`,
defaultMessage: `Reporting encountered an error on the page: {err}`,
values: { err: err.toString() },
})
);
})
);

const browserDisconnect$ = Rx.fromEvent(browser, 'disconnected').pipe(
mergeMap(() =>
Rx.throwError(
new Error(
i18n.translate('xpack.reporting.browsers.chromium.chromiumClosed', {
defaultMessage: `Reporting detected that Chromium has closed.`,
})
)
)
)
mergeMap(() => Rx.throwError(getChromiumDisconnectedError()))
);

return Rx.merge(pageError$, uncaughtExceptionPageError$, browserDisconnect$);
Expand Down
16 changes: 16 additions & 0 deletions x-pack/plugins/reporting/server/browsers/chromium/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { i18n } from '@kbn/i18n';
import { BrowserDownload } from '../';
import { CaptureConfig } from '../../../server/types';
import { LevelLogger } from '../../lib';
Expand All @@ -15,3 +16,18 @@ export const chromium: BrowserDownload = {
createDriverFactory: (binaryPath: string, captureConfig: CaptureConfig, logger: LevelLogger) =>
new HeadlessChromiumDriverFactory(binaryPath, captureConfig, logger),
};

export const getChromiumDisconnectedError = () =>
Copy link
Member Author

Choose a reason for hiding this comment

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

Exporting this as a function that returns an Error, so that when we need to throw the error, the stack trace makes sense.

new Error(
i18n.translate('xpack.reporting.screencapture.browserWasClosed', {
defaultMessage: 'Browser was closed unexpectedly! Check the server logs for more info.',
})
);

export const getDisallowedOutgoingUrlError = (interceptedUrl: string) =>
new Error(
i18n.translate('xpack.reporting.chromiumDriver.disallowedOutgoingUrl', {
defaultMessage: `Received disallowed outgoing URL: "{interceptedUrl}". Failing the request and closing the browser.`,
values: { interceptedUrl },
})
);
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,7 @@ export const scheduleTaskFnFactory: ScheduleTaskFnFactory<ImmediateCreateJobFn>
if (errPayload.statusCode === 404) {
throw notFound(errPayload.message);
}
if (err.stack) {
logger.error(err.stack);
}
logger.error(err);
throw new Error(`Unable to create a job from saved object data! Error: ${err}`);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ export async function generatePdfObservableFactory(reporting: ReportingCore) {
logger.debug(`PDF buffer byte length: ${buffer?.byteLength || 0}`);
tracker.endGetBuffer();
} catch (err) {
logger.error(`Could not generate the PDF buffer! ${err}`);
logger.error(`Could not generate the PDF buffer!`);
logger.error(err);
}

tracker.end();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { HeadlessChromiumDriver } from '../../browsers';
import { getChromiumDisconnectedError } from '../../browsers/chromium';

/*
* Call this function within error-handling `catch` blocks while setup and wait
* for the Kibana URL to be ready for screenshot. This detects if a block of
* code threw an exception because the page is closed or crashed.
*
* Also call once after `setup$` fires in the screenshot pipeline
*/
export const checkPageIsOpen = (browser: HeadlessChromiumDriver) => {
if (!browser.isPageOpen()) {
throw getChromiumDisconnectedError();
}
};
35 changes: 2 additions & 33 deletions x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ describe('Screenshot Observable Pipeline', () => {
`);
});

it('recovers if exit$ fires a timeout signal', async () => {
it('observes page exit', async () => {
// mocks
const mockGetCreatePage = (driver: HeadlessChromiumDriver) =>
jest
Expand Down Expand Up @@ -311,38 +311,7 @@ describe('Screenshot Observable Pipeline', () => {
}).toPromise();
};

await expect(getScreenshot()).resolves.toMatchInlineSnapshot(`
Array [
Object {
"elementsPositionAndAttributes": Array [
Object {
"attributes": Object {},
"position": Object {
"boundingClientRect": Object {
"height": 200,
"left": 0,
"top": 0,
"width": 200,
},
"scroll": Object {
"x": 0,
"y": 0,
},
},
},
],
"error": "Instant timeout has fired!",
"screenshots": Array [
Object {
"base64EncodedData": "allyourBase64",
"description": undefined,
"title": undefined,
},
],
"timeRange": null,
},
]
`);
await expect(getScreenshot()).rejects.toMatchInlineSnapshot(`"Instant timeout has fired!"`);
});

it(`uses defaults for element positions and size when Kibana page is not ready`, async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
ScreenshotResults,
ScreenshotsObservableFn,
} from '../../types';
import { checkPageIsOpen } from './check_browser_open';
import { DEFAULT_PAGELOAD_SELECTOR } from './constants';
import { getElementPositionAndAttributes } from './get_element_position_data';
import { getNumberOfItems } from './get_number_of_items';
Expand Down Expand Up @@ -68,7 +69,6 @@ export function screenshotsObservableFactory(
return Rx.from(urls).pipe(
concatMap((url, index) => {
const setup$: Rx.Observable<ScreenSetupData> = Rx.of(1).pipe(
takeUntil(exit$),
mergeMap(() => {
// If we're moving to another page in the app, we'll want to wait for the app to tell us
// it's loaded the next page.
Expand Down Expand Up @@ -117,14 +117,19 @@ export function screenshotsObservableFactory(
}));
}),
catchError((err) => {
checkPageIsOpen(driver); // if browser has closed, throw a relevant error about it

logger.error(err);
return Rx.of({ elementsPositionAndAttributes: null, timeRange: null, error: err });
})
);

return setup$.pipe(
takeUntil(exit$),
mergeMap(
async (data: ScreenSetupData): Promise<ScreenshotResults> => {
checkPageIsOpen(driver); // re-check that the browser has not closed

const elements = data.elementsPositionAndAttributes
? data.elementsPositionAndAttributes
: getDefaultElementPosition(layout.getViewport(1));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ export const createMockBrowserDriverFactory = async (
mockBrowserDriver.evaluate = opts.evaluate ? opts.evaluate : defaultOpts.evaluate;
mockBrowserDriver.screenshot = opts.screenshot ? opts.screenshot : defaultOpts.screenshot;
mockBrowserDriver.open = opts.open ? opts.open : defaultOpts.open;
mockBrowserDriver.isPageOpen = () => true;

mockBrowserDriverFactory.createPage = opts.getCreatePage
? opts.getCreatePage(mockBrowserDriver)
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/translations/translations/ja-JP.json
Original file line number Diff line number Diff line change
Expand Up @@ -14321,7 +14321,6 @@
"xpack.remoteClusters.updateRemoteCluster.noRemoteClusterErrorMessage": "その名前のリモートクラスターはありません。",
"xpack.remoteClusters.updateRemoteCluster.unknownRemoteClusterErrorMessage": "ES からレスポンスが返らず、クラスターを編集できません。",
"xpack.reporting.breadcrumb": "レポート",
"xpack.reporting.browsers.chromium.chromiumClosed": "レポート生成時に Chromium の終了を検出しました。",
"xpack.reporting.browsers.chromium.errorDetected": "レポート生成時にエラーを検出しました: {err}",
"xpack.reporting.browsers.chromium.pageErrorDetected": "レポート生成時にページでエラーを検出しました: {err}",
"xpack.reporting.chromiumDriver.disallowedOutgoingUrl": "無許可の着信 URL を受信しました: 「{interceptedUrl}」、終了します",
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/translations/translations/zh-CN.json
Original file line number Diff line number Diff line change
Expand Up @@ -14326,7 +14326,6 @@
"xpack.remoteClusters.updateRemoteCluster.noRemoteClusterErrorMessage": "没有该名称的远程集群。",
"xpack.remoteClusters.updateRemoteCluster.unknownRemoteClusterErrorMessage": "无法编辑集群,ES 未返回任何响应。",
"xpack.reporting.breadcrumb": "报告",
"xpack.reporting.browsers.chromium.chromiumClosed": "Reporting 检测到 Chromium 已关闭。",
"xpack.reporting.browsers.chromium.errorDetected": "Reporting 检测到错误:{err}",
"xpack.reporting.browsers.chromium.pageErrorDetected": "Reporting 在页面上检测到错误:{err}",
"xpack.reporting.chromiumDriver.disallowedOutgoingUrl": "接收到禁止的传出 URL:“{interceptedUrl}”,正在退出",
Expand Down
Binary file not shown.
Loading