Discussion #1
Comments
|
@samuong for ChromeDriver. Is it correct to assume that it's perfectly possible and valid to use ChromeDriver to execute async scripts before the page has finished loading? https://w3c.github.io/webdriver/webdriver-spec.html has things like "pageLoadStrategy", but this is all unfamiliar to me so I don't know where to start :) |
|
By default it is supposed to wait for the page to finish loading. However we also check the |
|
Oh, I didn't pay attention to what readyState "complete" actually means. @jugglinmike, can you confirm that the load event is in fact fired, perhaps add some bit of red that's removed in the load event? Also, can you check if the If it looks to scripts like everything has loaded normally and the problem still reproduces on the same runs where scripts verify this, then a repaint problem of some kind like @jgraham suggests seems likely. |
|
Sure thing, @foolip. I've extended the test script to capture more data. Here's
As you can see, the |
|
ChromeDriver will wait for pages to load before executing both the screenshot and the execute async script commands. It then hands it off to DevTools (Page.captureScreenshot) to take the screenshot. It's interesting that the naturalHeight/Width of the image is out of sync with what's being captured. I'll ask around within DevTools to see who can look into Page.captureScreenshot. |
|
From my local testing, this "appears to be avoidable" by forcing a repaint in --- a/index.html
+++ b/index.html
@@ -1,14 +1,15 @@
<!DOCTYPE html>
<html>
<head>
<script>
window.ONLOAD_FIRED = null;
onload = function() {
+ document.body.style.backgroundColor = 'hotpink';
window.ONLOAD_FIRED = 'the `onload` event fired';
};
</script>
</head>
<body style="outline: solid teal 2px;">
<img src="blue.png" style="border-width:50px; border-style:solid;"/>
</body>
</html>With that change in place, the image is rendered consistently through thousands A related-but-more-tenable approach would be to force that repaint in the --- a/main.py
+++ b/main.py
@@ -87,40 +87,41 @@ def take_reading(session_id):
script = '''
var callback = arguments[0];
function done(strategy) {
+ document.body.style.backgroundColor = 'hotpink';
callback({
callbackStrategy: strategy,
readyState: document.readyState,
scrollHeight: document.documentElement.scrollHeight,
onload_fired: window.ONLOAD_FIRED,
natural_dimensions: [
document.getElementsByTagName('img')[0].naturalWidth,
document.getElementsByTagName('img')[0].naturalHeight
]
});
}
if (document.readyState === 'complete') {
done('synchronous');
} else {
onload = done.bind(null, 'asynchronous');
}
'''
result = request('post', 'session/%s/execute_async' % session_id, dict(script=script, args=[]))This does not solve the problem. It actually makes it more pronounced,
What we're seeing here is that the repaint is not taking effect |
|
Chrome should already force a redraw before it captures a screenshot: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_impl.cc?l=1454. I think there's something else going wrong here... |
|
@foolip Sorry, I forgot to answer your original question. This is related to
There are other inconsistent reference tests that may be related. These do not
|
|
The first experiment #1 (comment) demonstrates clearly that the load event is in fact being fired before the screenshot is captured. However, it is perhaps not a given that the layout, even if invalidated, is actually recomputed before the screenshot is taken. While a script calling |
|
|
At @RByers's suggestion, I modified the "wait" script to defer for the next --- a/main.py
+++ b/main.py
@@ -116,7 +116,11 @@ try:
});
}
if (document.readyState === 'complete') {
- done('synchronous');
+ requestAnimationFrame(function() {
+ done('synchronous');
+ });
} else {
onload = done.bind(null, 'asynchronous');
}This modification did not address the instability: I observed the race --- a/main.py
+++ b/main.py
@@ -116,7 +116,11 @@ try:
});
}
if (document.readyState === 'complete') {
- done('synchronous');
+ requestAnimationFrame(function() {
+ requestAnimationFrame(function() {
+ done('synchronous');
+ });
+ });
} else {
onload = done.bind(null, 'asynchronous');
}This ran for almost three hours without exhibiting the race condition. While this is encouraging, it is by no means conclusive. There is no indication |
|
Yeah, we'll get a Chrome rendering expert to investigate and get back to you. But in the interim I do think it's a reasonable workaround, IIRC some blink LayoutTests follow such a double-rAF pattern already, eg. here. We recommend a different internal API which probably has stronger guarantees than just a double-rAF. Eg. it may be possible for rAF to fire while raster is still pending, so you could possible see "checkerboarding" (which today is just white, not an actual checkerboard pattern). But I suspect the WebDriver screenshot API blocks on raster (at least it should - probably doesn't make to generate a screenshot of something that hasn't finished rastering yet). |
|
@chrishtr, can you help us take a look here? I would be interesting to start from the question I posed to @eaenet in #1 (comment) about the order of events here. In |
|
Blink never paints with dirty layout. I'm trying to repro locally now. |
|
I am not yet able to reproduce this bug. I tried the steps in https://github.com/jugglinmike/chrome-screenshot-race/blob/master/readme.md, but when I run python main.py it errors out with: Traceback (most recent call last): As far as I can tell I have installed the "requests" module correctly. I also tried putting https://github.com/jugglinmike/chrome-screenshot-race/blob/master/index.html into the Blink Layout Test machinery, and was unable to make it fail in 1000 trials. (not that layout tests are the same as Chrome Driver, just one data point). |
|
@jugglinmike, just to ensure that we're not chasing after something that was recently fixed, can you try to repro also in Chrome Canary, and if successful note the exact version in case someone wants to try building and testing from the same source? |
|
Thanks for taking a look, @chrishtr!
I'm not sure what could be going wrong here, either. I've just removed and
That's a good idea, Philip. I'm unable to run Chrome Canary (no suitable |
|
I can now get it to run with the updated instructions. Should I expect the test to automatically fail with |
|
The script should capture an initial screenshot and then begin reloading the Here are the results of my last three executions (taken today):
This problem may be related to available system resources. In case that
|
|
Ok now I can get it to fail. Next I'm having trouble setting a custom Chrome binary, can you help with |
|
Sure thing. I just pushed an update that introduces a new command-line flag, You can also use the new |
|
I've just verified that this behavior persists in a more recent build of Chrome: 58.0.3029.6, released yesterday. |
|
Hi @chrishtr, is the new |
|
Hi, Yes it worked. Sorry for the delay. I still haven't found time to debug further. This week I got caught |
|
Got it. Just making sure it's still on your radar! |
|
Apologies, I still haven't found time to debug this in any depth this week. |
|
Got it. Thanks for keeping us posted |
|
FYI a similar/same bug got reported via a Google-internal channel, and a bisect to find |
|
Excellent! I'm excited to learn more :) |
|
For a workaround in the meantime, the double-rAF method mentioned in a comment on Feb 23 sounds like it would work (and I agree it should help). Does it? |
|
Yeah given that there are a bunch of questions about CSS test instability potentially impacted by this issue, I suggest we temporarily land the double-rAF workaround while we continue to investigate. @jugglinmike @gsnedders WDYT? |
|
@jgraham @jugglinmike we should try and land that in wptrunner ASAP |
Yes, see my comment from that day. As I mentioned, there, though, without some concrete justification, that technique is no better than (for instance)
I'm sensing some urgency here. I'm concerned that implementing an |
|
@jugglinmike It's not unsubstantiated insofar as it forces a paint and guarantees a paint has occurred, though it is a hypothesis that it is failing to paint the image… I don't think this will get deprioritised as a result of the workaround landing (as it likely effects far more than just this), hence I don't think that's a reason not to land this. Ideally, if we did make it an allowed failure, we'd do so for reftests only (which probably means in |
See <jugglinmike/chrome-screenshot-race#1>. This appears to avoid it from happening, and guarantees we've painted after the load event and (potentially) reftest-wait being removed.
|
@chrishtr @RByers Can the internal bug mentioned above be made public? @jgraham has pointed out that it would make a much better reference point for documenting this workaround. |
|
It cannot unfortunately, but I have filed this bug for you to use as a reference point instead: https://bugs.chromium.org/p/chromium/issues/detail?id=708757 |
|
Thank you, @chrishtr! I discussed this with @gsnedders and @jgraham on IRC, and we're going to move forward with the |
See <jugglinmike/chrome-screenshot-race#1>. This appears to avoid it from happening, and guarantees we've painted after the load event and (potentially) reftest-wait being removed. r=jgraham
|
Note that this shouldn't de-prioritise the upstream issue, at least until we know that the workaround (i.e., the double |
|
@jugglinmike @gsnedders @chrishtr Using a mutation observer and the Did you try your test under heavy CPU and GPU load? Any progress on the internal bug mentioned related to this? |
|
Please comment on crbug.com/708757. I don't think there has been any progress recently |
|
@chrishtr I just did. Good news! They are saying it is fixed and will be released in v60. Don't have access to that issue though so can't look into it further. "I thought this issue was fixed, maybe. https://bugs.chromium.org/p/chromium/issues/detailid=382170 So I am just waiting and hoping for the best. |
Filing an issue to have the discussion in public.
Which was the test that this flakiness was seen on? In IRC I see http://s92097608.onlinehome.us/tmp/17.02.17-flaky-reftest.html, is it http://w3c-test.org/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-dim.html that you minified?
Looks like https://github.com/w3c/wptrunner/blob/master/wptrunner/executors/reftest-wait_webdriver.js is the "wait" script and that this test doesn't use the "reftest-wait" mechanism.
I don't know WebDriver, but are the sequence of commands https://github.com/w3c/wptrunner/blob/38435bc6714ae83bbf759b04395fe13f08388396/wptrunner/executors/executorselenium.py#L253-L264 intended to wait until after the load event? (The load event on the window object is delayed until images are loaded.)
How/where is
webdriver.get(url)implemented?Even given the note in https://github.com/jugglinmike/chrome-screenshot-race/blob/master/LICENSE my guess is that there isn't anything that really guarantees the timing?
@jgraham
The text was updated successfully, but these errors were encountered: