Bug 131871 - Enable snapshot tests on mac wk2
Summary: Enable snapshot tests on mac wk2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 133227 132294
Blocks:
  Show dependency treegraph
 
Reported: 2014-04-18 18:07 PDT by Roger Fong
Modified: 2014-07-17 14:58 PDT (History)
6 users (show)

See Also:


Attachments
patch (30.76 KB, patch)
2014-04-30 00:39 PDT, Roger Fong
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roger Fong 2014-04-18 18:07:48 PDT
I'm currently working on enabling these tests, modifying each test to use the new API and adjusting the expected results.
I've run into an issue with the autoplay tests however.

When I run the test via layout-tests the primary plugin is not found.
However, when I just load the test in the browser, the primary plugin is found as expected...
Comment 1 Roger Fong 2014-04-18 18:16:18 PDT
If i set the maximum number of snapshot attempts to 1 the test passes.
Comment 2 Roger Fong 2014-04-21 21:18:24 PDT
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)
Comment 3 Roger Fong 2014-04-22 18:20:10 PDT
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.
Comment 4 Roger Fong 2014-04-25 14:52:17 PDT
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.
Comment 5 Roger Fong 2014-04-30 00:28:39 PDT
<rdar://problem/14395065>
Comment 6 Roger Fong 2014-04-30 00:39:45 PDT
Created attachment 230467 [details]
patch
Comment 7 Roger Fong 2014-04-30 00:40:29 PDT
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 8 Simon Fraser (smfr) 2014-04-30 09:03:18 PDT
Comment on attachment 230467 [details]
patch

one second per test seems way too long.
Comment 9 Darin Adler 2014-04-30 09:18:30 PDT
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.
Comment 10 Roger Fong 2014-04-30 12:24:43 PDT
(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
Comment 11 Roger Fong 2014-04-30 14:55:48 PDT
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.
Comment 12 Roger Fong 2014-04-30 14:59:46 PDT
(In reply to comment #11)
> time1: 340397
> time2: 341137

guess those are ticks huh.
Comment 13 Roger Fong 2014-04-30 15:13:16 PDT
http://trac.webkit.org/changeset/168048

Keeping this bug open to observe how the bots do.
Comment 14 Roger Fong 2014-05-21 12:47:01 PDT
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.
Comment 15 Alexey Proskuryakov 2014-05-22 09:17:20 PDT
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 ]
Comment 16 Alexey Proskuryakov 2014-05-22 20:27:08 PDT
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.
Comment 17 Roger Fong 2014-05-23 11:00:53 PDT
(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.