-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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: INSECURE_DOCUMENT_REQUEST errors still return lhr #6608
Changes from 25 commits
4c658fb
eb7f5a3
62787ee
3577e0c
2c0b251
e22d9ca
9e54f1b
059d95e
51e4f05
1ce9144
4bd59e4
53e00a7
1b6e678
dde5b95
4807cf0
2d505e8
1e1fc8a
0a0e8e0
b863fbd
da6fd9c
3c54326
ab0582b
f272978
e6a659a
271a80a
8bbcbc4
04b1f64
1da3b01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,13 +81,6 @@ class Driver { | |
this._eventEmitter.emit(event.method, event.params); | ||
}); | ||
|
||
/** | ||
* Used for monitoring network status events during gotoURL. | ||
* @type {?LH.Crdp.Security.SecurityStateChangedEvent} | ||
* @private | ||
*/ | ||
this._lastSecurityState = null; | ||
|
||
/** | ||
* @type {number} | ||
* @private | ||
|
@@ -546,14 +539,17 @@ class Driver { | |
/** @param {LH.Crdp.Page.LifecycleEventEvent} e */ | ||
const lifecycleListener = e => { | ||
if (e.name === 'firstContentfulPaint') { | ||
cancel(); | ||
resolve(); | ||
this.off('Page.lifecycleEvent', lifecycleListener); | ||
} | ||
}; | ||
|
||
this.on('Page.lifecycleEvent', lifecycleListener); | ||
|
||
let canceled = false; | ||
cancel = () => { | ||
if (canceled) return; | ||
canceled = true; | ||
this.off('Page.lifecycleEvent', lifecycleListener); | ||
}; | ||
}); | ||
|
@@ -634,7 +630,10 @@ class Driver { | |
networkStatusMonitor.on('network-2-busy', logStatus); | ||
|
||
this.once('Page.domContentEventFired', domContentLoadedListener); | ||
let canceled = false; | ||
cancel = () => { | ||
if (canceled) return; | ||
canceled = true; | ||
idleTimeout && clearTimeout(idleTimeout); | ||
this.off('Page.domContentEventFired', domContentLoadedListener); | ||
networkStatusMonitor.removeListener('network-2-busy', onBusy); | ||
|
@@ -666,7 +665,7 @@ class Driver { | |
|
||
/** @type {NodeJS.Timer|undefined} */ | ||
let lastTimeout; | ||
let cancelled = false; | ||
let canceled = false; | ||
|
||
const checkForQuietExpression = `(${pageFunctions.checkTimeSinceLastLongTaskString})()`; | ||
/** | ||
|
@@ -675,9 +674,9 @@ class Driver { | |
* @return {Promise<void>} | ||
*/ | ||
async function checkForQuiet(driver, resolve) { | ||
if (cancelled) return; | ||
if (canceled) return; | ||
const timeSinceLongTask = await driver.evaluateAsync(checkForQuietExpression); | ||
if (cancelled) return; | ||
if (canceled) return; | ||
|
||
if (typeof timeSinceLongTask === 'number') { | ||
if (timeSinceLongTask >= waitForCPUQuiet) { | ||
|
@@ -698,9 +697,9 @@ class Driver { | |
const promise = new Promise((resolve, reject) => { | ||
checkForQuiet(this, resolve).catch(reject); | ||
cancel = () => { | ||
cancelled = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this feels like a bug, we want to just add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops, didn't meant to delete that. can skip the guard here since reject/clearTimeout should have no effect after the first call. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. of course, leaving out the guard looks like a mistake, so perhaps I should just add it anyways? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I mean, it's also unnecessary everywhere else it's be added, isn't it? All removing listeners and clearing timeouts, which have no problem being run multiple times. I'm OK with having them since that's how we would write a cancellable promise utility instead of doing each of these separately (in which case all of the methods should have it), but I'm also OK with dropping them since they aren't necessary (except for the |
||
canceled = true; | ||
if (lastTimeout) clearTimeout(lastTimeout); | ||
reject(new Error('Wait for CPU idle cancelled')); | ||
reject(new Error('Wait for CPU idle canceled')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. interesting that we reject here but not in the others |
||
}; | ||
}); | ||
|
||
|
@@ -731,7 +730,10 @@ class Driver { | |
}; | ||
this.once('Page.loadEventFired', loadListener); | ||
|
||
let canceled = false; | ||
cancel = () => { | ||
if (canceled) return; | ||
canceled = true; | ||
this.off('Page.loadEventFired', loadListener); | ||
loadTimeout && clearTimeout(loadTimeout); | ||
}; | ||
|
@@ -743,6 +745,45 @@ class Driver { | |
}; | ||
} | ||
|
||
/** | ||
* Return a promise that resolves when an insecure security state is encountered | ||
* and a method to cancel internal listeners. | ||
* @return {{promise: Promise<LH.Crdp.Security.SecurityStateExplanation[]>, cancel: function(): void}} | ||
* @private | ||
*/ | ||
_waitForSecurityCheck() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rename since it resolves only in the case of insecurity (and the check isn't singular, it just goes until it isn't needed)? |
||
/** @type {(() => void)} */ | ||
let cancel = () => { | ||
throw new Error('_waitForSecurityCheck.cancel() called before it was defined'); | ||
}; | ||
|
||
const promise = new Promise((resolve, reject) => { | ||
/** | ||
* @param {LH.Crdp.Security.SecurityStateChangedEvent} event | ||
*/ | ||
const securityStateChangedListener = ({securityState, explanations}) => { | ||
if (securityState === 'insecure') { | ||
cancel(); | ||
resolve(explanations); | ||
} | ||
}; | ||
let canceled = false; | ||
cancel = () => { | ||
if (canceled) return; | ||
canceled = true; | ||
this.off('Security.securityStateChanged', securityStateChangedListener); | ||
this.sendCommand('Security.disable').catch(() => {}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe add a TODO here for whatever @patrickhulce is planning with the refactor? |
||
}; | ||
this.on('Security.securityStateChanged', securityStateChangedListener); | ||
this.sendCommand('Security.enable').catch(() => {}); | ||
}); | ||
|
||
return { | ||
promise, | ||
cancel, | ||
}; | ||
} | ||
|
||
/** | ||
* Returns whether the page appears to be hung. | ||
* @return {Promise<boolean>} | ||
|
@@ -765,6 +806,7 @@ class Driver { | |
/** | ||
* Returns a promise that resolves when: | ||
* - All of the following conditions have been met: | ||
* - page has no security issues | ||
* - pauseAfterLoadMs milliseconds have passed since the load event. | ||
* - networkQuietThresholdMs milliseconds have passed since the last network request that exceeded | ||
* 2 inflight requests (network-2-quiet has been reached). | ||
|
@@ -793,6 +835,19 @@ class Driver { | |
// CPU listener. Resolves when the CPU has been idle for cpuQuietThresholdMs after network idle. | ||
let waitForCPUIdle = this._waitForNothing(); | ||
|
||
const waitForSecurityCheck = this._waitForSecurityCheck(); | ||
const securityCheckPromise = waitForSecurityCheck.promise.then(explanations => { | ||
return function() { | ||
const insecureDescriptions = explanations | ||
.filter(exp => exp.securityState === 'insecure') | ||
.map(exp => exp.description); | ||
const err = new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, { | ||
securityMessages: insecureDescriptions.join(' '), | ||
}); | ||
throw err; | ||
}; | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ha, I think I disagree with @patrickhulce. The internals of If that's not convincing, maybe we could split the difference? Filter and map within const securityCheckPromise = waitForSecurityCheck.promise.then(securityMessages => {
return function() {
throw new new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, {securityMessages});
}
}); (using the cleanupFn to reject the promise is a little weird but w/e :P) |
||
|
||
// Wait for both load promises. Resolves on cleanup function the clears load | ||
// timeout timer. | ||
const loadPromise = Promise.all([ | ||
|
@@ -805,7 +860,6 @@ class Driver { | |
}).then(() => { | ||
return function() { | ||
log.verbose('Driver', 'loadEventFired and network considered idle'); | ||
maxTimeoutHandle && clearTimeout(maxTimeoutHandle); | ||
}; | ||
}); | ||
|
||
|
@@ -816,11 +870,6 @@ class Driver { | |
}).then(_ => { | ||
return async () => { | ||
log.warn('Driver', 'Timed out waiting for page load. Checking if page is hung...'); | ||
waitForFCP.cancel(); | ||
waitForLoadEvent.cancel(); | ||
waitForNetworkIdle.cancel(); | ||
waitForCPUIdle.cancel(); | ||
|
||
if (await this.isPageHung()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW for future refactoring, @paulirish mentioned he doesn't think this check is necessary anymore (errors are successfully being detected elsewhere, I guess) |
||
log.warn('Driver', 'Page appears to be hung, killing JavaScript...'); | ||
await this.sendCommand('Emulation.setScriptExecutionDisabled', {value: true}); | ||
|
@@ -830,11 +879,20 @@ class Driver { | |
}; | ||
}); | ||
|
||
// Wait for load or timeout and run the cleanup function the winner returns. | ||
// Wait for security issue, load or timeout and run the cleanup function the winner returns. | ||
const cleanupFn = await Promise.race([ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. having this |
||
securityCheckPromise, | ||
loadPromise, | ||
maxTimeoutPromise, | ||
]); | ||
|
||
maxTimeoutHandle && clearTimeout(maxTimeoutHandle); | ||
waitForFCP.cancel(); | ||
waitForLoadEvent.cancel(); | ||
waitForNetworkIdle.cancel(); | ||
waitForCPUIdle.cancel(); | ||
waitForSecurityCheck.cancel(); | ||
|
||
await cleanupFn(); | ||
} | ||
|
||
|
@@ -1007,26 +1065,6 @@ class Driver { | |
return result.body; | ||
} | ||
|
||
async listenForSecurityStateChanges() { | ||
this.on('Security.securityStateChanged', state => { | ||
this._lastSecurityState = state; | ||
}); | ||
await this.sendCommand('Security.enable'); | ||
} | ||
|
||
/** | ||
* @return {LH.Crdp.Security.SecurityStateChangedEvent} | ||
*/ | ||
getSecurityState() { | ||
if (!this._lastSecurityState) { | ||
// happens if 'listenForSecurityStateChanges' is not called, | ||
// or if some assumptions about the Security domain are wrong | ||
throw new Error('Expected a security state.'); | ||
} | ||
|
||
return this._lastSecurityState; | ||
} | ||
|
||
/** | ||
* @param {string} name The name of API whose permission you wish to query | ||
* @return {Promise<string>} The state of permissions, resolved in a promise. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,7 @@ const UIStrings = { | |
/** Error message explaining that Lighthouse could not load the requested URL and the steps that might be taken to fix the unreliability. */ | ||
pageLoadFailedWithDetails: '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. (Details: {errorDetails})', | ||
/** Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed. */ | ||
pageLoadFailedInsecure: 'The URL you have provided does not have valid security credentials. ({securityMessages})', | ||
pageLoadFailedInsecure: 'The URL you have provided does not have valid security credentials. {securityMessages}', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason for removing these? I feel like a list that might make no sense as a sentence like this belongs in parens. e.g. The URL...credentials. (Reason 1 Reason 2) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A realistic error message reads like this:
Which is a valid sentence. So...ignore this maybe. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correct-o, the sentences should make sense as - is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. at least, in english .. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to add something about |
||
/** Error message explaining that Chrome has encountered an error during the Lighthouse run, and that Chrome should be restarted. */ | ||
internalChromeError: 'An internal Chrome error occurred. Please restart Chrome and try re-running Lighthouse.', | ||
/** Error message explaining that fetching the resources of the webpage has taken longer than the maximum time. */ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boooo to one L :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer Mr. Webster's simplified spellings, but I could care more.