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

core: throw fatally on page hang #6497

Merged
merged 5 commits into from
Nov 16, 2018
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
9 changes: 9 additions & 0 deletions lighthouse-cli/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const opn = require('opn');

const _RUNTIME_ERROR_CODE = 1;
const _PROTOCOL_TIMEOUT_EXIT_CODE = 67;
const _PAGE_HUNG_EXIT_CODE = 68;

/**
* exported for testing
Expand Down Expand Up @@ -70,6 +71,12 @@ function showProtocolTimeoutError() {
process.exit(_PROTOCOL_TIMEOUT_EXIT_CODE);
}

/** @param {LH.LighthouseError} err */
function showPageHungError(err) {
console.error('Page hung:', err.friendlyMessage);
process.exit(_PAGE_HUNG_EXIT_CODE);
}

/**
* @param {LH.LighthouseError} err
*/
Expand All @@ -89,6 +96,8 @@ function handleError(err) {
showConnectionError();
} else if (err.code === 'CRI_TIMEOUT') {
showProtocolTimeoutError();
} else if (err.code === 'PAGE_HUNG') {
showPageHungError(err);
} else {
showRuntimeError(err);
}
Expand Down
22 changes: 22 additions & 0 deletions lighthouse-cli/test/fixtures/infinite-loop.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<!doctype html>
<!--
* Copyright 2018 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
-->
<html>
Copy link
Member

Choose a reason for hiding this comment

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

license

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

<meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1">
<body>
This is the function that never ends
Yes, it just goes on and on my friends.
Some people started computing it, not knowing what it was.
And they'll continue computing it forever just because,
Copy link
Member

Choose a reason for hiding this comment

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

🤣

This is the function that never ends.

<script>
while (true) {
for (let i = 0; i < 1000000; i++) ;
}
</script>
</body>
</html>
19 changes: 19 additions & 0 deletions lighthouse-cli/test/smokehouse/error-config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* @license Copyright 2018 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

/**
* Config file for sites with various errors, just fail out quickly.
*/
module.exports = {
extends: 'lighthouse:default',
settings: {
maxWaitForLoad: 5000,
onlyAudits: [
'first-contentful-paint',
],
},
};
18 changes: 18 additions & 0 deletions lighthouse-cli/test/smokehouse/error-expectations.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* @license Copyright 2018 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

/**
* Expected Lighthouse audit values for sites with various errors.
Copy link
Member

Choose a reason for hiding this comment

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

🎉

*/
module.exports = [
{
requestedUrl: 'http://localhost:10200/infinite-loop.html',
finalUrl: 'http://localhost:10200/infinite-loop.html',
errorCode: 'PAGE_HUNG',
audits: {},
},
];
5 changes: 5 additions & 0 deletions lighthouse-cli/test/smokehouse/run-smoke.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ const SMOKETESTS = [{
config: smokehouseDir + 'a11y/a11y-config.js',
expectations: 'a11y/expectations.js',
batch: 'parallel-first',
}, {
id: 'errors',
expectations: smokehouseDir + 'error-expectations.js',
config: smokehouseDir + 'error-config.js',
batch: 'errors',
}, {
id: 'pwa',
expectations: smokehouseDir + 'pwa-expectations.js',
Expand Down
9 changes: 7 additions & 2 deletions lighthouse-cli/test/smokehouse/smokehouse.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const yargs = require('yargs');
const log = require('lighthouse-logger');

const PROTOCOL_TIMEOUT_EXIT_CODE = 67;
const PAGE_HUNG_EXIT_CODE = 68;
const RETRIES = 3;
const NUMERICAL_EXPECTATION_REGEXP = /^(<=?|>=?)((\d|\.)+)$/;

Expand Down Expand Up @@ -87,7 +88,7 @@ function runLighthouse(url, configPath, isDebug) {
if (runResults.status === PROTOCOL_TIMEOUT_EXIT_CODE) {
console.error(`Lighthouse debugger connection timed out ${RETRIES} times. Giving up.`);
process.exit(1);
} else if (runResults.status !== 0) {
} else if (runResults.status !== 0 && runResults.status !== PAGE_HUNG_EXIT_CODE) {
console.error(`Lighthouse run failed with exit code ${runResults.status}. stderr to follow:`);
console.error(runResults.stderr);
process.exit(runResults.status);
Expand All @@ -98,10 +99,14 @@ function runLighthouse(url, configPath, isDebug) {
console.error(`STDERR: ${runResults.stderr}`);
}

if (runResults.status === PAGE_HUNG_EXIT_CODE) {
return {requestedUrl: url, finalUrl: url, errorCode: 'PAGE_HUNG', audits: {}};
}

const lhr = fs.readFileSync(outputPath, 'utf8');
if (isDebug) {
console.log('LHR output available at: ', outputPath);
} else {
} else if (fs.existsSync(outputPath)) {
fs.unlinkSync(outputPath);
}

Expand Down
33 changes: 30 additions & 3 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,7 @@ class Driver {
includeCommandLineAPI: true,
awaitPromise: true,
returnByValue: true,
timeout: 60000,
contextId,
};

Expand Down Expand Up @@ -687,6 +688,25 @@ class Driver {
};
}

/**
* Returns whether the page appears to be hung.
* @return {Promise<boolean>}
*/
async isPageHung() {
try {
this.setNextProtocolTimeout(1000);
await this.sendCommand('Runtime.evaluate', {
expression: '"ping"',
returnByValue: true,
timeout: 1000,
});

return false;
} catch (err) {
return true;
}
}

/**
* Returns a promise that resolves when:
* - All of the following conditions have been met:
Expand Down Expand Up @@ -736,11 +756,18 @@ class Driver {
const maxTimeoutPromise = new Promise((resolve, reject) => {
maxTimeoutHandle = setTimeout(resolve, maxWaitForLoadedMs);
}).then(_ => {
return function() {
log.warn('Driver', 'Timed out waiting for page load. Moving on...');
return async () => {
log.warn('Driver', 'Timed out waiting for page load. Checking if page is hung...');
waitForLoadEvent.cancel();
waitForNetworkIdle.cancel();
waitForCPUIdle && waitForCPUIdle.cancel();

if (await this.isPageHung()) {
log.warn('Driver', 'Page appears to be hung, killing JavaScript...');
await this.sendCommand('Emulation.setScriptExecutionDisabled', {value: true});
await this.sendCommand('Runtime.terminateExecution');
Copy link
Member

Choose a reason for hiding this comment

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

i think we should also run setScriptDisabled({disabled: true}) at this point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

throw new LHError(LHError.errors.PAGE_HUNG);
}
};
});

Expand All @@ -749,7 +776,7 @@ class Driver {
loadPromise,
maxTimeoutPromise,
]);
cleanupFn();
await cleanupFn();
Copy link
Member

Choose a reason for hiding this comment

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

now necessary since we have the awaits inside of maxTimeoutPromise, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well the awaits are inside the function that it returns too, so we want to wait to find out if the page is hung before moving on

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

order of execution is

  1. wait for earlier of load + quiet and maxWaitForLoad
  2. If we reached maxWaitForLoad then check if we're hung

we don't want to be doing our killing and hang check while we're trying to see if the page is quiet.

}

/**
Expand Down
6 changes: 6 additions & 0 deletions lighthouse-core/lib/lh-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,12 @@ const ERRORS = {
message: strings.pageLoadFailedInsecure,
lhrRuntimeError: true,
},
/* Used when the page stopped responding and did not finish loading. */
PAGE_HUNG: {
Copy link
Member

Choose a reason for hiding this comment

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

this'll need to go into the proto. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

code: 'PAGE_HUNG',
message: strings.pageLoadFailedHung,
lhrRuntimeError: true,
},

// Protocol internal failures
TRACING_ALREADY_STARTED: {
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/lib/strings.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ module.exports = {
badTraceRecording: `Something went wrong with recording the trace over your page load. Please run Lighthouse again.`,
pageLoadTookTooLong: `Your page took too long to load. Please follow the opportunities in the report to reduce your page load time, and then try re-running Lighthouse.`,
pageLoadFailed: `Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests.`,
pageLoadFailedHung: `Lighthouse was unable to reliably load the URL you requested because the page stopped responding.`,
pageLoadFailedInsecure: `The URL you have provided does not have valid security credentials.`,
internalChromeError: `An internal Chrome error occurred. Please restart Chrome and try re-running Lighthouse.`,
requestContentTimeout: 'Fetching resource content has exceeded the allotted time',
Expand Down
2 changes: 2 additions & 0 deletions proto/lighthouse-result.proto
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ enum LighthouseError {
INSECURE_DOCUMENT_REQUEST = 16;
// Used when protocol command times out.
PROTOCOL_TIMEOUT = 17;
// Used when the page is not responding after maxWaitForLoad.
PAGE_HUNG = 18;
}

// The overarching Lighthouse Response object (LHR)
Expand Down