Summary: | Enable snapshot tests on mac wk2 | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Roger Fong <roger_fong> | ||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ap, dino, jonlee, roger_fong, simon.fraser, thorton | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | 132294, 133227 | ||||||
Bug Blocks: | |||||||
Attachments: |
|
Description
Roger Fong
2014-04-18 18:07:48 PDT
If i set the maximum number of snapshot attempts to 1 the test passes. After some extensive logging I believe that the following is happening: The plugin is initialized before the primary plugin has been determined. If the maximum number of snapshot retries is 0 then we immediately set display state to DisplayingSnapshot. Now we hit HTMLPlugInImageElement::createElementRenderer and hit if (displayState() == DisplayingSnapshot) { auto renderSnapshottedPlugIn = createRenderer<RenderSnapshottedPlugIn>(*this, std::move(style)); renderSnapshottedPlugIn->updateSnapshot(m_snapshotImage); return std::move(renderSnapshottedPlugIn); } We then get into determining the primary plugin, which is where (if a candidate plugin is found) we will restart the plugin appropriately. However, the hit test fails, presumably due to the code that we hit earlier. Commenting out the code block above causes us to fallback to creating a RenderImage renderer, which seems to fair better with hit testing for some reason. Perhaps we should not create the element renderer until the plugin has been initialized, since we should know the initial state of the plugin anyways before deciding what type of renderer to create. Or we shouldn't set the display state to DisplayingSnapshot at all until we know for sure the plugin is meant to be snapshotted (defer the setting of the display state until the plugin has been initialized) The dimension of the plugin element in question comes out to 139 by 18, the testing spot is 300, 300. The actual width and height is 600 by 600. After diving down into the rabbit hole that is layout code, I've become thoroughly convinced that we should never be snapshotting a plugin before we determine whether or not it is the primary one. Snapshotting a plugin causes all sorts of problems to arise. In this particular case it seems like we run into RenderFlexibleBox layout code which sets the logical override dimensions of the pluginImageElement. However, these dimensions are not the dimensions of the original plugin element. I am not savvy enough with layout code to know what's going on here but it seems fundamentally wrong to try to hittest with something that isn't what we originally intended to hit test against in the first place. Created attachment 230467 [details]
patch
500 ms didn't seem to be enough for the layout tests to reliably go thru the hit testing and snapshotting process, I gave each a full second to do so and haven't managed to get any failing results yet. Comment on attachment 230467 [details]
patch
one second per test seems way too long.
Comment on attachment 230467 [details]
patch
All these tests say window.internals, but they could just say internals.
It’s really bad that these tests run so slowly and that they are inherently subject to race conditions. It’s not good to have multiple tests that require waiting 1 second no matter how fast the computer is or how heavily loaded it is.
(In reply to comment #8) > (From update of attachment 230467 [details]) > one second per test seems way too long. I at first thought this was because of the timer delays, but looking at them now the delay for primary snapshot detection is a not so whopping 3ms. I also noticed that quicktime only takes 500 ms. The quicktime plugin is also quite simple, just an orange background. the lines.swf test file we use is a bit more complicated, so maybe it'll be better if we use a simpler plugin time1: 340397 time2: 341137 clock() call between subframeLoaderWillInitializePlugin and subframeLoaderDidInitializePlugin. Looks like I could get away with a little less than 1000 ms, but that doesn't really solve the issue does it. So basically it seems like the plugin takes a while to load fully. Dean says there's not much that we can do about that. I'm going to enable these tests to see how they fare on the bots. (In reply to comment #11) > time1: 340397 > time2: 341137 guess those are ticks huh. http://trac.webkit.org/changeset/168048 Keeping this bug open to observe how the bots do. Didn't notice some earlier mistakes I made. Fixes are here: http://trac.webkit.org/changeset/169173 Now the tests should actually run and we can see how flakey they are. These tests are failing every time on WebKit2. plugins/snapshotting/set-plugin-size-to-tiny.html [ Failure ] plugins/snapshotting/simple.html [ Failure ] plugins/snapshotting/snapshot-plugin-not-quite-blocked-by-image.html [ Failure ] These tests are still broken. Is there any reason why it is ok to make everyone's work harder for so long? EWS takes a really long time to finish, and even lies sometimes because of this. (In reply to comment #16) > These tests are still broken. Is there any reason why it is ok to make everyone's work harder for so long? > > EWS takes a really long time to finish, and even lies sometimes because of this. Skipped https://bugs.webkit.org/show_bug.cgi?id=133227 for now. |