Closed Bug 1583462 Opened 5 years ago Closed 5 years ago

Remove expected failures for cross-origin iframes in webdriver/tests/take_*_screenshot/iframe.py with Fission enabled

Categories

(Testing :: geckodriver, task, P3)

Product:
Testing
Component:
geckodriver
Version:
Version 3
Type:
task

Tracking

(Fission Milestone:M4, firefox72 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla72
Project Flags:
Fission Milestone M4
Tracking Flags:
Tracking Status
firefox72 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(4 files, 2 obsolete files)

pytest failure output
34.82 KB, text/plain
Details
txt.txt
32.62 KB, text/plain
Details
log for repeated `n` and `i` characters
8.00 KB, text/plain
Details
Bug 1583462 - [wdspec] Fix webdriver/tests/take_*_screenshot/iframe.py for Fission. r=#webdriver!
47 bytes, text/x-phabricator-request
Details | Review

With bug 1559841 being fixed the navigation with Marionette when Fission is enabled works fine now! This is great. But the wdspec screenshot tests are still failing due to some strange reason. Comparing the screenshots manually both images seem to be identical.

Do you have the data uris of the screenshots? You can paste them into the reftest analyzer ( https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml ) to have it point out the differences.

Attached file pytest failure output —

The webdriver tests are driven by pytest and are not part of the reftest suite. When I try to paste the URIs I do not get any output, but it just clears the textarea. Maybe it only understands the reftest log format?

The test can be run locally via:

mach wpt testing/web-platform/tests/webdriver/tests/take_element_screenshot/iframe.py --webdriver-arg=-vv --setpref fission.autostart=true

If necessary also specify the path to the geckodriver binary by adding the option like --webdriver-binary target/debug/geckodriver.

Flags: needinfo?(tnikkel)
Attached file txt.txt (obsolete) —

So I just cut and pasted the base64 encoded data into another failing reftest log I had from try server. This works.

It looks like the reftest analyzer detects that the two images are the same.

Looking at the log it looks like it is comparing the base64 encoded strings. The strings are different. But two different png files can produce the same pixels, and it appears as though that is what is happening here.

Is there a difference in the way the two pngs are created before they are base64 encoded?

The test harness should maybe decode the image and compare the pixels rather than compare the strings.

Interesting point. I didn't know that even PNG files could have different data for the exact same image. We actually never faced that before since we started to run screenshot comparison tests for Marionette (via Python unit tests) [1]. The usage of self.assertEqual() never brought-up such a problem.

Timothy, please note that the test works fine all the time without Fission, and always fails with Fission enabled. So I actually would imagine that something is wrong with Fission here.

[1] https://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #4)

Interesting point. I didn't know that even PNG files could have different data for the exact same image. We actually never faced that before since we started to run screenshot comparison tests for Marionette (via Python unit tests) [1]. The usage of self.assertEqual() never brought-up such a problem.

Timothy, please note that the test works fine all the time without Fission, and always fails with Fission enabled. So I actually would imagine that something is wrong with Fission here.

Doesn't necessarily mean anything is wrong with it, just that the screenshots are being produced slightly differently. To track down why there is a difference we need to look at the place that generates the png files. Where is that? Then we can look at what options are being passed and why it might differ.

Flags: needinfo?(tnikkel) → needinfo?(hskupin)

The canvas which is used to draw the screenshot gets filled in capture.canvas(), whereby the data URL is getting created in capture.toBase64.

This code uses the new Fission compatible drawSnapshot() API (line128), and then draws the snapshot into the 2d context.

Also note that it works also fine with Fission enabled, and no OOP iframes.

Flags: needinfo?(hskupin) → needinfo?(tnikkel)
Summary: Remove expected failures for webdriver/tests/take_*_screenshot/iframe.py with Fission enabled → Remove expected failures for cross-origin iframes in webdriver/tests/take_*_screenshot/iframe.py with Fission enabled

Okay, thanks for that. It'll have to be debugged why it is different, nothing obvious pops out at me.

The chunk structure of the two pngs is the same except one chunk is 3 bytes shorter. Both have two IDATs, the second one being shorter. The first IDAT looks bit for bit identical. The second idat differs midway through. Not sure what to conclude from this.

Attached file txt.txt (obsolete) —

Turns out the two pngs actually have a pixel that is different. It's just that in the reftest-analyzer it's really hard to see. And the txt file I uploaded before must have had a mistake somewhere. Here is a fixed file. If you paste this in the reftest-analyzer and mouse over 101, 105 you can see that they are different slightly.

Attachment #9096872 - Attachment is obsolete: true
Flags: needinfo?(tnikkel)

Timothy, this new text file contains multiple images. When I copy and paste the base64 content of only the two into the reftest analyzer there is no way to hover, and details aren't shown in the top-left zooming like view. Also both look really identically, and do not show the red little square as your results do. Maybe I'm not using it correctly? I wish we would have some code to just put in two images instead of a full log.

When I use the digital colour meter on MacOS and check the small dot I can also see a difference. The first image (x-origin iframe) has RGB(209,209,209) while the second (div) has RGB(208,208,208). Without Fission they all have RGB(208,208,208) too.

So even it's only a single digit per channel difference, could something be wrong with rendering in Fission and x-origin iframes? Or is it only the generated base64 content which has this failure?

Flags: needinfo?(tnikkel)
Attached file txt.txt —

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #10)

Timothy, this new text file contains multiple images.

Oops, my mistake. I left some other stuff in the file. Try this new file.

When I copy and paste the base64 content of only the two into the reftest analyzer there is no way to hover, and details aren't shown in the top-left zooming like view. Also both look really identically, and do not show the red little square as your results do. Maybe I'm not using it correctly? I wish we would have some code to just put in two images instead of a full log.

Try this new file. Note that you still won't see the red outline for some reason. Not sure why, maybe a bug in the reftest analyzer. But mousing over works.

When I use the digital colour meter on MacOS and check the small dot I can also see a difference. The first image (x-origin iframe) has RGB(209,209,209) while the second (div) has RGB(208,208,208). Without Fission they all have RGB(208,208,208) too.

So even it's only a single digit per channel difference, could something be wrong with rendering in Fission and x-origin iframes? Or is it only the generated base64 content which has this failure?

It seems as though the rendering is different. So that should be investigated.

Pure speculation, but it might be that the iframe is not exactly at an integer coordinate in layout units, so when rendering the iframe in the same process it picks up that offset, but when it is rendering in a different process it starts at an integer offset. So perhaps try changing the offset of the iframe.

Attachment #9097560 - Attachment is obsolete: true
Flags: needinfo?(tnikkel)

Actually one interesting fact which I nearly missed is that I'm fairly sure that I was not able to see this failure when I wrote these tests as part of bug 1559592. I had to add a time.sleep() call to the Python code for sure (due to bug 1559841), but that doesn't help anymore.

So I assume that we most likely see a regression here which started within the last month. Let me have a look again if I can reproduce that, and if I can I will have a regression range.

Flags: needinfo?(hskupin)

So I was wrong. The same happened with such an older build.

(In reply to Timothy Nikkel (:tnikkel) from comment #11)

Timothy, this new text file contains multiple images.

Oops, my mistake. I left some other stuff in the file. Try this new file.

Yes, that works. Thanks.

When I use the digital colour meter on MacOS and check the small dot I can also see a difference. The first image (x-origin iframe) has RGB(209,209,209) while the second (div) has RGB(208,208,208). Without Fission they all have RGB(208,208,208) too.

So even it's only a single digit per channel difference, could something be wrong with rendering in Fission and x-origin iframes? Or is it only the generated base64 content which has this failure?

It seems as though the rendering is different. So that should be investigated.

I assume that we should file a new bug in which component? Graphics in general?

Pure speculation, but it might be that the iframe is not exactly at an integer coordinate in layout units, so when rendering the iframe in the same process it picks up that offset, but when it is rendering in a different process it starts at an integer offset. So perhaps try changing the offset of the iframe.

But why is it always the same pixel which connects the i with the n? It doesn't matter how large the iframe is, or where it is positioned. I always see the same failure. Could be related to anti-aliasing of the text? An indication is that when I remove the n so we have ... adipiscig elit as text, it works just fine. No more differences. As such I prepared another content which repeats n and i, and indeed multiple failures. Who knows most about text rendering those days?

Flags: needinfo?(hskupin) → needinfo?(tnikkel)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #13)

Pure speculation, but it might be that the iframe is not exactly at an integer coordinate in layout units, so when rendering the iframe in the same process it picks up that offset, but when it is rendering in a different process it starts at an integer offset. So perhaps try changing the offset of the iframe.

But why is it always the same pixel which connects the i with the n? It doesn't matter how large the iframe is, or where it is positioned. I always see the same failure. Could be related to anti-aliasing of the text? An indication is that when I remove the n so we have ... adipiscig elit as text, it works just fine. No more differences. As such I prepared another content which repeats n and i, and indeed multiple failures. Who knows most about text rendering those days?

Oh wow, that is a very interesting observation. Jonathan, do you know why we would see a slight difference in a single pixel on the letter 'i' but only when it's followed by an 'n'?

I also see antialiasing differences with my work to get reftests working with fission, I'm not sure if it is related or not. Example: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd5f6e44f6f56dfd113866025f751f0b9f28d840

Flags: needinfo?(tnikkel) → needinfo?(jfkthame)

(In reply to Timothy Nikkel (:tnikkel) from comment #14)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #13)

Pure speculation, but it might be that the iframe is not exactly at an integer coordinate in layout units, so when rendering the iframe in the same process it picks up that offset, but when it is rendering in a different process it starts at an integer offset. So perhaps try changing the offset of the iframe.

But why is it always the same pixel which connects the i with the n? It doesn't matter how large the iframe is, or where it is positioned. I always see the same failure. Could be related to anti-aliasing of the text? An indication is that when I remove the n so we have ... adipiscig elit as text, it works just fine. No more differences. As such I prepared another content which repeats n and i, and indeed multiple failures. Who knows most about text rendering those days?

Oh wow, that is a very interesting observation. Jonathan, do you know why we would see a slight difference in a single pixel on the letter 'i' but only when it's followed by an 'n'?

I don't know why we're seeing this, but I suspect it's probably got something to do with the antialiasing of the 'i' and that of the 'n' touching each other. Maybe the two glyphs sometimes get composited in a single operation by some level of the stack, and other times they're done separately, and the results vary slightly. Font antialiasing often shows subtle issues that we can't really control, but which are below the level of what's perceptible to the eye and so we're OK with just annotating them as "fuzzy".

I also see antialiasing differences with my work to get reftests working with fission, I'm not sure if it is related or not. Example: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd5f6e44f6f56dfd113866025f751f0b9f28d840

In this case, the difference seems to be that the iframe (used in the reference file) is being rendered with grayscale antialiasing, while the testcase has subpixel (cleartype) AA for its rendering of the same content.

Presumably without fission, they're both subpixel-antialiased, and moving the iframe out-of-process is preventing subpixel rendering. I don't know whether that's an expected outcome, or something we should try to fix.

Flags: needinfo?(jfkthame)

(In reply to Jonathan Kew (:jfkthame) from comment #15)

I also see antialiasing differences with my work to get reftests working with fission, I'm not sure if it is related or not. Example: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd5f6e44f6f56dfd113866025f751f0b9f28d840

In this case, the difference seems to be that the iframe (used in the reference file) is being rendered with grayscale antialiasing, while the testcase has subpixel (cleartype) AA for its rendering of the same content.

Presumably without fission, they're both subpixel-antialiased, and moving the iframe out-of-process is preventing subpixel rendering. I don't know whether that's an expected outcome, or something we should try to fix.

Thanks, I don't think it's expected. I filed bug 1585803 to investigate and/or fix.

Timothy, so what shall we do with my case? Given the feedback from Jonathan it sounds like we should accept it? I assume that for reftests we have a fuzzy threshold for comparison?

Given that we do not really do reftest-like tests in wdspec I would consider to just remove the word which triggers this problem. Does that sound fine?

Flags: needinfo?(tnikkel)

Matt informed me that the issues I was seeing on regular reftests were happening because the content of the iframe is transparent, only the background color in it's parent process are opaque. Try explicitly giving the document in the iframe an opaque background color by giving the html or body element a background color and see if that helps.

Flags: needinfo?(tnikkel)

That seems to work with the following change:

@@ -11,6 +11,7 @@ DEFAULT_CSS_STYLE = """
     <style>
       div, iframe {
         display: block;
+        background: green;
         border: 1px solid blue;
         width: 10em;
         height: 10em;
@@ -34,7 +35,7 @@ def test_source_origin(session, url, dom
     reference_screenshot = assert_success(response)
     assert png_dimensions(reference_screenshot) == viewport_dimensions(session)

-    iframe_content = "<style>body {{ margin: 0; }}</style>{}".format(DEFAULT_CONTENT)
+    iframe_content = "<style>body {{ margin: 0; background: green; }}</style>{}".format(DEFAULT_CONTENT)
     session.url = inline("""{0}{1}""".format(
         DEFAULT_CSS_STYLE, iframe(iframe_content, domain=domain)))

Interesting is that I explicitly have to set the background for the frame's body. The first change from above for the iframe is not enough. I assume this happens because both are different processes?

Flags: needinfo?(tnikkel)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #19)

Interesting is that I explicitly have to set the background for the frame's body. The first change from above for the iframe is not enough. I assume this happens because both are different processes?

Yeah, the background of the iframe element would be in the parent process and the background for the body would be in the child process. You should only need the background on the body in the child process. You can use a white background, any color works, just has to be opaque.

Flags: needinfo?(tnikkel)

(In reply to Timothy Nikkel (:tnikkel) from comment #18)

Matt informed me that the issues I was seeing on regular reftests were happening because the content of the iframe is transparent, only the background color in it's parent process are opaque. Try explicitly giving the document in the iframe an opaque background color by giving the html or body element a background color and see if that helps.

While this would work I strongly feel that this is a bug in Firefox with Fission and that we might want to get it fixed. Setting an explicit background color here would only be a workaround for us, but this is a cross-browser test and I'm not feeling that well doing it. As such I will change the text of the iframe, so that we do not end-up in those subtle differences.

Shall we file a bug for it? Maybe you could do with a better wording?

Flags: needinfo?(tnikkel)

Shorten the text as used in the iframe to prevent a rendering issue
between the letters "n" and "i" in Firefox when Fission is enabled.

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a478ac839016
[wdspec] Fix webdriver/tests/take_*_screenshot/iframe.py for Fission. r=webdriver-reviewers,maja_zf
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19995 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
See Also: → 1592631

https://hg.mozilla.org/mozilla-central/rev/a478ac839016

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox72: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Assignee: nobody → hskupin
Upstream PR merged by moz-wptsync-bot

Retroactively moving fixed bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to an appropriate Fission Milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Fission Milestone: --- → M4
Flags: needinfo?(tnikkel)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: