core(driver): assert securityIssues before getPageLoadError

[paulirish]

I think we want to throw on security issues before we consider pageLoadError situations. wdyt?

[patrickhulce]

Makes sense to me

[connorjclark]

What's the difference in behavior? Seems that either way, on a security issue an error is thrown that is uncaught, going all the way to runner.js (And beyond).

Only difference I see is not logging some page load error + doing some small extra work (that wouldn't be used). is this meant to change something else too?

[exterkamp]

LGTM subtle but should expose more security issues as errors instead of ERROR DOC or FAILED DOC requests., right?

[brendankenny]

I would think that the danger would be getting NO or FAILED document requests if there was a security error, so we'd never get to the security check, but clearly we do get security errors, so seems like this won't make a difference?

If it doesn't make a difference, seems fine either way?

[connorjclark]

The current code pushes the page load error onto the current passContext, but if there is a security error, that will throw an error, exiting the flow of the entire runner. Nothing is done with the passContext no matter the order of things (other than a log.error of the page load error)

... now I see that log.error does more than just log, and it actually does throw an error, so I retract all that I've said :P

[brendankenny]

now I see that log.error does more than just log, and it actually does throw an error

I don't think that's right? Log.error

The current code pushed the page load error onto the current passContext, but if there is a security error, that will throw an error, exiting the flow of the entire runner.

ah, right. So if there was a security error, the pageLoadError gets ignored whether the security error check is before or after it.

From the perspective of reading the code it makes sense to have it first (and maybe even earlier), but I don't think this is going to change the error type distribution :)

[connorjclark]

Ah, I was looking at the render logger. OK, so yeah, no change really in this PR except for avoiding logging a page load error when really it was a security issue. LGTM