Open ebidel opened 7 years ago
@ebidel I looked at this for a while today, and wanted to ping you about a couple of things.
filename for the audit?
Watching network requests Since I'm still new to the codebase, I wanted to ask a couple of questions about implementing the checks for this.
beforeUnload
event, it's available to me through PageLevelEventListeners
on the artifacts
object thats passed into audit
. It seems like the best way to implement this would be, check for beforeUnload
or pagehide
events, if present - monitor for sync xhrs or fetch like you mentioned before. NetworkRecorder
inside of lighthouse-core/lib
will be of most help for this.Cheers
"uses-sendbeacon" sgtm.
For the network stuff, it looks like something clever like this could work. I didn't realize you could dispatching beforeunload
yourself works!
// Inject this into page in beforePass
const observer = new PerformanceObserver(list => {
const xhrsRequests = list.getEntries().filter(entry => {
// Note: fetch() does not have initiatorType - https://crbug.com/649571
return entry.initiatorType === 'xmlhttprequest';
});
if (xhrsRequests.length) {
console.log(xhrsRequests);
}
}).observe({entryTypes: ['resource']});
// Example of page code that you should add to lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
window.addEventListener('beforeunload', e => {
const xhr = new XMLHttpRequest();
xhr.open('GET', 'https://www.chromestatus.com/features', false);
xhr.send();
});
// Inject into page in `afterPass`
window.dispatchEvent(new CustomEvent('beforeunload'));
window.dispatchEvent(new CustomEvent('pagehide'));
You could wrap the observer and dispatching part in a function that gets injected into the page. Example. You might need to play around with delaying dispatching the events.
This would't tell you that the XHR was sync (unless you also grep the console output for the warning chrome throws when sync xhrs are used), but I don't think it would hurt to always suggest navigator.sendBeacon
for all requests made in beforeunload
and pagehide
handlers.
Does this need to be instrumented in the page itself? Instead, could you look at the initiator field in the trace event log and detect any !beacon requests initiated after unload?
PerfObserver does not guarantee synchronous delivery of these events, and since page is being unloaded, there is high likelihood that you may miss requests with the above pattern -- e.g. inject an image beacon and allow page to continue unloading, etc.
Just looking at the --save-artifacts
output from running against https://googlechrome.github.io/samples/beacon/....`navigator.sendBeaconrequests don't seem to show up in our network trace. Perhaps this is because they're not captured during the pass (e.g. the request is made after the fact in a
pagehide` handler).
The other thing we miss out on with using the network trace is line/col/source info:
@igrigorik I also found that navigator.sendBeacon
requests don't show up in PerformanceObserver
. Is that known? Should they?
Yes, they should. Sounds like a bug. If you have a test page.. can you please file a bug?
@ebidel @igrigorik is this something I can tackle without navigator.sendBeacon
exposed, or should I hold off until the bug is fixed?
@olingern the bug with PerformanceObserver
doesn't apply here. We don't care about pages using navigator.sendBeacon
. They're doing the right thing. We only care about pages that are using XHR on page unload :)
I'd experiment with the technique I highlighted in https://github.com/GoogleChrome/lighthouse/issues/871#issuecomment-260062573 to see if we catch PerformanceObserver
callbacks in unbeforeunload
. They're not guaranteed to be delivered on page unload, but I believe they were when I tried this out quickly.
Definitely add some tests to https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html and let us know.
Need to get a complete new audit proposal on this one: https://github.com/GoogleChrome/lighthouse/blob/master/docs/new-audits.md
It's definitely worthwhile, but will take some work.
in m75, sync xhr in "page dismissal" is attempted to be removed. https://bugs.chromium.org/p/chromium/issues/detail?id=827324
I'm pretty sure there is a console.log/violation we could use.
Seems good, though we'd have to dismiss the page during gathering. We could try to fire their handlers, though..
But if this removal sticks, then it already did our job for us.
window.addEventListener('beforeunload', function () {
var request = new XMLHttpRequest();
request.open('GET', '/bar/foo.txt', false); // `false` makes the request synchronous
request.send(null);
if (request.status === 200) {
console.log(request.responseText);
}
});
VM26:1 Uncaught DOMException: Failed to execute 'send' on 'XMLHttpRequest': Failed to load 'https://developer.mozilla.org/bar/foo.txt': Synchronous XHR in page dismissal.
How to check:
beforeunload
/pagehide
handlers (can reuse https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/gather/gatherers/dobetterweb/document-event-listeners.js)fetch
calls in the handlerhttps://developer.mozilla.org/en-US/docs/Web/API/Navigator/sendBeacon