Bug 311812
| Summary: | [site-isolation] WTR does not correctly generate the expected.png for color-scheme related tests | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Carlos Alberto Lopez Perez <clopez> |
| Component: | Layout and Rendering | Assignee: | Kiet Ho <kiet.ho> |
| Status: | RESOLVED DUPLICATE | ||
| Severity: | Normal | CC: | bfulgham, kiet.ho, simon.fraser, webkit-bug-importer, zalan |
| Priority: | P2 | Keywords: | InRadar |
| Version: | WebKit Nightly Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=311766 https://bugs.webkit.org/show_bug.cgi?id=284973 https://bugs.webkit.org/show_bug.cgi?id=309611 |
||
Carlos Alberto Lopez Perez
This is a bug that i discovered meanwhile working on bug 311766
I can only reproduce on Mac, because on GTK/WPE site-isolation support seems broken and tests timeout.
To reproduce use the following 3 tests:
http/tests/css/prefers-color-scheme-in-cross-origin-iframe-follows-system-preference-without-parent-color-scheme.html
imported/w3c/web-platform-tests/css/css-color-adjust/rendering/dark-color-scheme/color-scheme-iframe-preferred-cross-origin.sub.html
imported/w3c/web-platform-tests/css/css-color-adjust/rendering/dark-color-scheme/color-scheme-iframe-preferred-page-dark-cross-origin.sub.html
And to the following
1) Ensure that the tests should fail
echo '<html><body><h1>I should fail!</h1></body></html>' > LayoutTests/http/tests/css/prefers-color-scheme-in-cross-origin-iframe-follows-system-preference-without-parent-color-scheme-expected.html
echo '<html><body><h1>I should fail!</h1></body></html>' > LayoutTests/imported/w3c/web-platform-tests/css/css-color-adjust/rendering/dark-color-scheme/color-scheme-iframe-preferred-cross-origin.sub-expected.html
echo '<html><body><h1>I should fail!</h1></body></html>' > LayoutTests/imported/w3c/web-platform-tests/css/css-color-adjust/rendering/dark-color-scheme/color-scheme-iframe-preferred-page-dark-cross-origin.sub-expected.html
That is, invalidate the expected file so it clearly should fail.
2) Now check the results before and after the fix from https://github.com/WebKit/WebKit/pull/62312 on ‎Source/WebCore/css/query/MediaQueryFeatures.cpp
Case A) With the fix from pull/62312 the 3 tests fail with site-isolation enabled. But the expected.png file rendered has nothing to do with the "I should fail" image that it should generate. It looks like instead of that -expected.html it is picking the test .html
Case B) Without the fix from pull/62312 all the 3 tests pass with site-isolation enabled! how that can be? clearly something wrong in the runner.
The command that I use to check this is the following:
./Tools/Scripts/run-webkit-tests --no-show-results --no-new-test-results --no-sample-on-timeout --no-build --results-directory layout-test-results --debug-rwt-logging --site-isolation --release --no-retry-failures --expect-pass http/tests/css/prefers-color-scheme-in-cross-origin-iframe-follows-system-preference-without-parent-color-scheme.html imported/w3c/web-platform-tests/css/css-color-adjust/rendering/dark-color-scheme/color-scheme-iframe-preferred-cross-origin.sub.html imported/w3c/web-platform-tests/css/css-color-adjust/rendering/dark-color-scheme/color-scheme-iframe-preferred-page-dark-cross-origin.sub.html
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Carlos Alberto Lopez Perez
Note: i'm only reporting the bug. I don't plan to work on fixing this.
Radar WebKit Bug Importer
<rdar://problem/174417711>
Kiet Ho
If I remember correctly, run-webkit-tests --site-isolation compares the rendering result of the test file only (not -expected.html) between SI-enabled and SI-disabled, it doesn't check with the expected file at all. In other words, if the same test renders differently between SI-enabled and SI-disabled, that's a fail.
Given that, the three test cases you cited should pass without pull/62312 (case B), and should fail with pull/62312 (case A), because your change checks ownerRenderer() of the frame, which is not available in SI-enabled.
Carlos Alberto Lopez Perez
(In reply to Kiet Ho from comment #3)
> If I remember correctly, run-webkit-tests --site-isolation compares the
> rendering result of the test file only (not -expected.html) between
> SI-enabled and SI-disabled, it doesn't check with the expected file at all.
Why is that?
That means that any test will pass meanwhile it renders the same thing with SI or without SI enabled. That makes the test meaningless, as the expected is not checked.
Kiet Ho
I don't make the tooling so I don't know. I think it's because --site-isolation is meant to test that SI and non-SI render identically.
I assume if non-SI render identically to the expected file, and SI renders the test identically to non-SI, then you could say that SI renders identically to the expected result.
Carlos Alberto Lopez Perez
(In reply to Kiet Ho from comment #5)
> I don't make the tooling so I don't know. I think it's because
> --site-isolation is meant to test that SI and non-SI render identically.
>
> I assume if non-SI render identically to the expected file, and SI renders
> the test identically to non-SI, then you could say that SI renders
> identically to the expected result.
That makes sense, but that is not what is happening here.
Kiet Ho
(sorry for the gap in replies, I just realized my bugzilla email was disabled so I didn't get any email notifications)
Let's back up a little bit:
> With the fix from pull/62312 the 3 tests fail with site-isolation enabled. But the expected.png file rendered has nothing to do with the "I should fail" image that it should generate. It looks like instead of that -expected.html it is picking the test .html
run-webkit-tests --site-isolation does not use -expected.html at all. Instead it renders the test page in SI and non-SI, and use the non-SI as expected rendering. Therefore it only checks that a test page renders identically in SI vs non-SI mode, it doesn't check that the test page in SI mode renders identically to the expected page. You modified the -expected.html file, but it doesn't get used by --site-isolation, hence you didn't see the "I should fail"
With your fix in pull/62312, it's expected that the three tests you mentioned would fail with --site-isolation. Your fix uses frame.ownerRenderer(), which when running in a cross-origin iframe, would return null (because the owner renderer object is in another web content process). This leads to the @prefers (media-color-scheme) code not calling protect(parent->virtualView())->ownerElementOfChildFrameUsesDarkAppearance() (note that ownerElementOfChildFrameUsesDarkAppearance() is safe to be called in Site Isolation, as we synchronize the dark appearance of owner renderer between web content processes)
> Case B) Without the fix from pull/62312 all the 3 tests pass with site-isolation enabled! how that can be? clearly something wrong in the runner.
This makes sense, since without the fix, we just call ownerElementOfChildFrameUsesDarkAppearance(), which is SI safe.
Kiet Ho
I've fixed the use of ownerRenderer() in 312212@main (bug 309611), so those tests should be passing now. I'll dupe this to bug 309611 then.
Kiet Ho
*** This bug has been marked as a duplicate of bug 309611 ***
Carlos Alberto Lopez Perez
I see, thanks for the explanation. I understand now better the issue.
I think is quite confusing this behavior of run-webkit-tests with --site-isolation, but at least now I know how it works. Thanks.