Bug 112100 – REGRESSION (r145277) causes chromium gpu throughput tests to fail

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Shawn Singh]

bisected down to http://trac.webkit.org/changeset/145277.   Reverting locally also fixes the problem.

tl;dr - If I don't hear from you by Tuesday March 12 around lunch time (PST) I will go ahead and try reverting (as GPU Pixel Wrangler for chromium) to make the bots green.  If you feel confident that it can be fixed within 1 day, I can hold off on reverting, since there would be some lag anyway due to WK rolls.

After this rolled into chromium, the GPU throughput tests (part of performance_browser_tests) started failing.  Actually it probably has nothing to do with gpu / graphics, except that the test page being used is causing the problem.  The html just seems to hang halfway through loading, or maybe it just produces incomplete output, I can't tell.  Every once in a rare while, if I aggressively hold down the key shortcut to refresh for example, the page actually does load correctly.

Unfortunately, looking at some logs, it looks like some work is already going on very close to this code, for example http://trac.webkit.org/changeset/145464 by abarth@.

So my question is:  will this be safe to revert for now?  We should consider it more important to get the chromium gpu bots green than to avoid reverting here.

[Adam Barth]

Eric is about to land a change to this part of the code in bug 112069.  There's a good chance that it will fix this issue.

[Shawn Singh]

OK, sure - eric@ can you please make sure that it fixes the problem before landing?  It will get harder to revert a sequence of patches, of course.  Thanks very much =)

[Eric Seidel (no email)]

You can revert if you need.  I'm on vacation today.

https://bugs.webkit.org/show_bug.cgi?id=112069 is written, but it seemed possibly flaky on the EWS bots, so I'm going to run it again and fix it.  I may get to that during my vacation today, but it probably won't land until tomorrow.

You can also just mark the tests as flaky until I get a chance to fix them tomorrow.  Completely up to you.

Sorry for the trouble.

[Eric Seidel (no email)]

I've rolled the revision out in bug 112170 as a preventative measure.

[Shawn Singh]

Eric - thank you for your understanding =)  I'm marking this as fixed since the bots are green.  Please let me know if you have any questions about how I reproduce the issue if you needed to locally re-test it.

[Eric Seidel (no email)]

I'm about to land bug 112069, but I would like to make sure that it does not regress these tests.  How can I make sure?

[Eric Seidel (no email)]

bug 112069 is related to this code, but hopefully a better implementation than previous. :)

[James Robinson]

(In reply to comment #6)
> I'm about to land bug 112069, but I would like to make sure that it does not regress these tests.  How can I make sure?

I think Tony G knows how to run these (and other tests that busted when the first patch landed).

[Eric Seidel (no email)]

Actually, looking at the old code, it looks obviously wrong.

It was invalidating checkpoints even in the case where we were writing for an external script to load, which is wrong!  That external script could end up document.writing and then needing to resume from a previous checkpoint.

The new approach in bug 112069 takes a different (much simpler) approach and does not have this bug.

[Shawn Singh]

looks like it already is about to land.  I'll just check briefly on my machine locally immediately after it lands, since I had the repro locally just yesterday.

[Tony Gentilcore]

> I think Tony G knows how to run these (and other tests that busted when the first patch landed).

The scrolling_benchmark was among those broken. espn failed to load complete. To run locally:
$ tools/perf/run_multipage_benchmarks --browser=release scrolling_benchmark tools/perf/page_sets/top_25.json

[Eric Seidel (no email)]

The I believe the following test case would have failed with the old patch:

test.html:
<script src=slow-script.js"></script>
<PASS> <- Should say <PASS> before this.

slow-script.js:
document.write('<plaintext>')

[Eric Seidel (no email)]

I've added a test for that case to the patch I'm landing now.