Skip to content
Permalink
Browse files

Fix: Error handling improvements

* Better report unhandled errors in promises
* Ignore errors originating from the target page
* Remove usage of HAR API (fixed for FF 67+)
* Exclude error stacks from extension telemetry
* Limit extension max scan time to three minutes

- - - - - - - - - - - - - -

Fix #2431 
Fix #2434
Fix #2442 
Close #2458
  • Loading branch information...
molant authored and antross committed May 21, 2019
1 parent 8d37a3d commit 7db6ffa11bb6e7c776b3fa7f06cddae62fd4a3be
@@ -21,7 +21,7 @@ export default class WebExtensionConnector implements IConnector {
private _originalDocument: HTMLDocument | undefined;
private _engine: Engine;
private _fetchEndQueue: FetchEnd[] = [];
private _onComplete: (resource: string) => void = () => { };
private _onComplete: (err: Error | null, resource?: string) => void = () => { };
private _options: ConnectorOptionsConfig;

public static schema = {
@@ -49,21 +49,24 @@ export default class WebExtensionConnector implements IConnector {
});

browser.runtime.onMessage.addListener(async (events: Events) => {
if (events.fetchEnd) {
await this.notifyFetch(events.fetchEnd);
}
if (events.fetchStart) {
await this._engine.emitAsync('fetch::start', events.fetchStart);
try {
if (events.fetchEnd) {
await this.notifyFetch(events.fetchEnd);
}
if (events.fetchStart) {
await this._engine.emitAsync('fetch::start', events.fetchStart);
}
// TODO: Trigger 'fetch::start::target'.
} catch (err) {
this._onComplete(err);
}
// TODO: Trigger 'fetch::start::target'.
});

const onLoad = () => {
const resource = location.href;

setTimeout(async () => {

if (document.documentElement) {
try {
await this.evaluateInPage(`(${createHelpers})()`);

const snapshot: DocumentData = await this.evaluateInPage('__webhint.snapshotDocument(document)');
@@ -72,18 +75,21 @@ export default class WebExtensionConnector implements IConnector {

this._document = new HTMLDocument(snapshot, this._originalDocument);

await this.sendFetchEndEvents();

await traverse(this._document, this._engine, resource);
}

await this.sendFetchEndEvents();
/*
* Evaluate after the traversing, just in case something goes wrong
* in any of the evaluation and some scripts are left in the DOM.
*/
await this._engine.emitAsync('can-evaluate::script', { resource });

/*
* Evaluate after the traversing, just in case something goes wrong
* in any of the evaluation and some scripts are left in the DOM.
*/
await this._engine.emitAsync('can-evaluate::script', { resource });
this._onComplete(null, resource);

this._onComplete(resource);
} catch (err) {
this._onComplete(err);
}
}, this._options.waitFor);
};

@@ -180,11 +186,21 @@ export default class WebExtensionConnector implements IConnector {

this.sendMessage({ ready: true });

return new Promise((resolve) => {
this._onComplete = async (resource: string) => {
await this._engine.emitAsync('scan::end', { resource });
resolve();
this.sendMessage({ done: true });
return new Promise((resolve, reject) => {
this._onComplete = async (err: Error | null, resource = '') => {
if (err) {
reject(err);

return;
}

try {
await this._engine.emitAsync('scan::end', { resource });
resolve();
this.sendMessage({ done: true });
} catch (e) {
reject(e);
}
};
});
}
@@ -201,7 +217,7 @@ export default class WebExtensionConnector implements IConnector {
* of the website.
*/
private evaluateInPage(source: string): Promise<any> {
return new Promise((resolve) => {
return new Promise((resolve, reject) => {
const script = document.createElement('script');
const config = {
attributes: true,
@@ -210,29 +226,44 @@ export default class WebExtensionConnector implements IConnector {
};

const callback = (mutationsList: MutationRecord[], observer: MutationObserver) => {
mutationsList.forEach((mutation: MutationRecord) => {
if (mutation.type !== 'attributes' && mutation.attributeName !== 'data-result') {
return;
}

const result = JSON.parse(script.getAttribute('data-result')!);

document.body.removeChild(script);
observer.disconnect();

resolve(result);
});
try {
mutationsList.forEach((mutation: MutationRecord) => {
if (mutation.type !== 'attributes' || !(/^data-(error|result)$/).test(mutation.attributeName || '')) {
return;
}

const error = script.getAttribute('data-error');
const result = script.getAttribute('data-result');

document.body.removeChild(script);
observer.disconnect();

if (error) {
reject(JSON.parse(error));
} else if (result) {
resolve(JSON.parse(result));
} else {
reject(new Error('No error or result returned from evaluate.'));
}
});
} catch (err) {
reject(err);
}
};

const observer = new MutationObserver(callback);

observer.observe(script, config);

script.textContent = `(async () => {
const scriptElement = document.currentScript;
const result = await ${source}
const scriptElement = document.currentScript;
try {
const result = await ${source}
scriptElement.setAttribute('data-result', JSON.stringify(result) || null);
scriptElement.setAttribute('data-result', JSON.stringify(result) || null);
} catch (err) {
scriptElement.setAttribute('data-error', JSON.stringify({ message: err.message, stack: err.stack }));
}
})();`;

document.body.appendChild(script);
@@ -12,7 +12,7 @@ import HTMLParser from '@hint/parser-html';
import JavaScriptParser from '@hint/parser-javascript';
import ManifestParser from '@hint/parser-manifest';

import { browser, location, window } from '../shared/globals';
import { browser, location } from '../shared/globals';
import { Config, Events } from '../shared/types';

import WebExtensionConnector from './connector';
@@ -29,14 +29,6 @@ const reportError = (message: string, stack: string) => {
});
};

window.addEventListener('error', (evt) => {
reportError(evt.error.message, evt.error.stack);
});

window.addEventListener('unhandledrejection', (evt) => {
reportError(evt.reason.message, evt.reason.stack);
});

/** Use the provided `browserslist` query if valid; `defaults` otherwise. */
const determineBrowserslist = (list?: string) => {
if (list) {
@@ -119,7 +111,9 @@ const main = async (userConfig: Config) => {

const onMessage = (events: Events) => {
if (events.config) {
main(events.config);
main(events.config).catch((err) => {
reportError(err.message, err.stack);
});
browser.runtime.onMessage.removeListener(onMessage);
}
};
@@ -34,7 +34,8 @@ export const trackCancel = (duration: number) => {

/** Called when analysis fails due to an error. */
export const trackError = (error: ErrorData) => {
trackEvent('f12-error', error);
// Drop the error stack as it can contain filesystem paths.
trackEvent('f12-error', { message: error.message });
};

/** Called when analysis finished. */
@@ -51,3 +52,8 @@ export const trackShow = () => {
export const trackStart = () => {
trackEvent('f12-start');
};

/** Called when analysis fails to complete in the allotted time. */
export const trackTimeout = (duration: number) => {
trackEvent('f12-timeout', undefined, { 'f12-timeout-duration': duration });
};
@@ -1,9 +1,6 @@
import { Entry, Log } from 'har-format';

import { browser } from '../../shared/globals';
import { mapHeaders } from '../../shared/headers';

import { evaluate } from './inject';
import { sendMessage } from './messaging';

/** Track the number of redirects by request. */
@@ -19,59 +16,14 @@ const generateFetchStart = (request: chrome.devtools.network.Request) => {
sendMessage({ fetchStart: { resource: request.request.url } });
};

/**
* Wait for the provided entry to include all response data.
* This works around a bug in Firefox which sometimes triggers
* onRequestFinished before HAR entries have been populated.
* https://bugzilla.mozilla.org/show_bug.cgi?id=1472653
*/
const waitForFullEntry = (entry: Entry, retries = 5): Promise<Entry> => {
return new Promise((resolve) => {

// Don't wait if the entry has already been populated.
if (typeof entry.response.content.size === 'number' && typeof entry.response.headersSize === 'number' && entry.response.status) {
resolve(entry);

return;
}

// Give up after a few attempts to avoid retrying forever.
if (!retries) {
evaluate(`console.warn("[webhint] HAR missing data after max retries for ${entry.request.url}")`);
resolve(entry);

return;
}

// Wait just a bit to give the HAR time to be populated.
setTimeout(() => {
// Then check if the HAR has a matching entry with more data.
browser.devtools.network.getHAR((async (harLog: Log) => {
const match = harLog.entries.filter((ent) => {
return ent.request.url === entry.request.url;
})[0];

// Retry with the matching entry if found, or the old one otherwise (as a match may not exist yet).
resolve(await waitForFullEntry(match || entry, retries - 1));
}) as any);
}, 500);
});
};

/**
* Get the entry response content directly from a `Request` or `Entry`.
*/
const getContent = (entry: chrome.devtools.network.Request | Entry): Promise<string> => {
const getContent = (entry: chrome.devtools.network.Request): Promise<string> => {
return new Promise((resolve) => {
if ('getContent' in entry) {
// If the first attempt, we can get the content directly (Chrome).
entry.getContent((content) => {
resolve(content);
});
} else {
// If a retry, we need to look on the response (Firefox; not populated by Chrome).
resolve(entry.response.content.text);
}
entry.getContent((content) => {
resolve(content || '');
});
});
};

@@ -80,19 +32,18 @@ const getContent = (entry: chrome.devtools.network.Request | Entry): Promise<str
* These are forwarded to the content-script via the background-script.
*/
const generateFetchEnd = async (entry: chrome.devtools.network.Request) => {
const fullEntry = await waitForFullEntry(entry);
const content = await getContent(entry) || '';
const url = fullEntry.request.url;
const content = await getContent(entry);
const url = entry.request.url;

if (fullEntry.response.redirectURL) {
if (entry.response.redirectURL) {

// Track hops on a redirect, using an existing list of hops if one exists.
const urls = hops.has(url) ? hops.get(url)! : [];

// Add the previous URL to the list and stash under the current requested URL.
urls.push(url);
hops.delete(url);
hops.set(fullEntry.response.redirectURL, urls);
hops.set(entry.response.redirectURL, urls);

} else {

@@ -104,7 +55,7 @@ const generateFetchEnd = async (entry: chrome.devtools.network.Request) => {
fetchEnd: {
element: null, // Set by `content-script/connector`.
request: {
headers: mapHeaders(fullEntry.request.headers),
headers: mapHeaders(entry.request.headers),
url: requestURL
},
resource: url,
@@ -115,10 +66,10 @@ const generateFetchEnd = async (entry: chrome.devtools.network.Request) => {
rawResponse: null as any
},
charset: '', // Set by `content-script/connector`.
headers: mapHeaders(fullEntry.response.headers),
headers: mapHeaders(entry.response.headers),
hops: requestHops,
mediaType: '', // Set by `content-script/connector`.
statusCode: fullEntry.response.status,
statusCode: entry.response.status,
url
}
}
@@ -8,7 +8,7 @@ import ConfigPage from './pages/config';
import ErrorPage from './pages/error';
import ResultsPage from './pages/results';

import { trackCancel, trackError, trackFinish, trackStart } from '../utils/analytics';
import { trackCancel, trackError, trackFinish, trackStart, trackTimeout } from '../utils/analytics';
import { useCurrentDesignStyles, useCurrentTheme, withCurrentDesign } from '../utils/themes';

import * as fluent from './app.fluent.css';
@@ -74,6 +74,13 @@ const App = () => {
onRestart();
}, [onRestart]);

const onTimeout = useCallback((duration: number) => {
setIsAnalyzing(false);
setPage(Page.Error);
setError({ message: 'Scan timed out.', stack: '' });
trackTimeout(duration);
}, []);

const getCurrentPage = () => {
switch (page) {
case Page.Config:
@@ -90,7 +97,7 @@ const App = () => {
return (
<div className={styles.root} data-theme={theme}>
{getCurrentPage()}
{isAnalyzing && <Analyze config={config} onCancel={onCancel} onError={onError} onResults={onResults}/>}
{isAnalyzing && <Analyze config={config} onCancel={onCancel} onError={onError} onResults={onResults} onTimeout={onTimeout}/>}
</div>
);
};

0 comments on commit 7db6ffa

Please sign in to comment.
You can’t perform that action at this time.