Bug 292059 – REGRESSION(293789@main): div > img elements get incorrectly rendered / aligned

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

[Max Schmitt]

Created attachment 475046 [details]
Unexpected rendering

After https://commits.webkit.org/293789@main the rendering seems broken when we rendering grid.html in Playwright. It looks like a race, since it seems to only occur when I run it under load.

We encounter this with looking at the UI as well as when taking screenshots. 

- grid.html: https://github.com/microsoft/playwright/blob/main/tests/assets/grid.html
- See https://github.com/microsoft/playwright/issues/35740#issuecomment-2828907834 and the attachment on how it looks like

[Radar WebKit Bug Importer]

<rdar://problem/150037613>

[Alexey Proskuryakov]

293789@main was a revert of a fairly recent commit, 292662@main. Was this always broken in Playwright before 292662@main?

Also, what are the steps to reproduce? Does one need to use Playwright, or is this reproducible in e.g. MiniBrowser?

[Max Schmitt]

It happens across the platforms, I was checking on macOS and was able to reproduce as well. 1/300 times.

> Was this always broken in Playwright before 292662@main?

This test was not flaky / failing before. I checked one commit before and there it was fine. With this commit the failure was introduced. The line which made it work before was:

    if (cachedBaselineContextAncestor.value())
        return RelayoutBoundary::No;

It doesn't look like a clean revert - there are probably some changes in between and the combination of this and the revert might cause it.

> Also, what are the steps to reproduce? Does one need to use Playwright, or is this reproducible in e.g. MiniBrowser?

It should be reproducible with plain MiniBrowser. The issue is that it happens rarely and only under load. So I'd recommend e.g. a Selenium test or LayoutTest similar to:

it('repro', async ({ page, server }) => {
  await page.setViewportSize({ width: 500, height: 500 });
  await page.goto(server.PREFIX + '/grid.html');
  expect(await page.screenshot()).toMatchSnapshot('screenshot-sanity.png');
});

[Diego Pino]

The bug doesn't happen if commenting these two lines in Source/WebCore/rendering/RenderObject.cpp:

```
diff --git a/Source/WebCore/rendering/RenderObject.cpp b/Source/WebCore/rendering/RenderObject.cpp
index f999dc45f611..64dbdb747272 100644
--- a/Source/WebCore/rendering/RenderObject.cpp
+++ b/Source/WebCore/rendering/RenderObject.cpp
@@ -656,8 +656,10 @@ RenderElement* RenderObject::markContainingBlocksForLayout(RenderElement* layout
             auto boundary = isLayoutBoundary(*ancestor);
             if (boundary == RelayoutBoundary::Yes)
                 return ancestor.get();
+            /*
             if (boundary == RelayoutBoundary::OverflowOnly)
                 simplifiedNormalFlowLayout = true;
+            */
         }
 
         if (auto* renderGrid = dynamicDowncast<RenderGrid>(container.get()); renderGrid && renderGrid->isExtrinsicallySized())

```

I figured this out by reverting 292724@main before reverting 292662@main. With both commits reverted I could not reproduce the bug. Then, I went back to main and I started undoing the changes introduced by 292724@main in Source/WebCore/rendering/RenderObject.cpp. Eventually I figured out with those two lines commented the PlaywRight test passes:

```
WKPATH=/home/pwuser/playwright-browsers/browser_patches/webkit/pw_run.sh xvfb-run -a npm run wtest -- library/emulation-focus.spec.ts:104 --repeat-each 100


> playwright-internal@1.53.0-next wtest
> playwright test --config=tests/library/playwright.config.ts --project=webkit-* library/emulation-focus.spec.ts:104 --repeat-each 100

Using executable at /home/pwuser/playwright-browsers/browser_patches/webkit/pw_run.sh

Running 100 tests using 40 workers
  100 passed (18.1s)
```

[Matt Woodrow]

Pull request: https://github.com/WebKit/WebKit/pull/44646

[Matt Woodrow]

I'm going to revert 292724@main as well, since that's what started this sequence of breakages.

[EWS]

Committed 294318@main (8d86f0f62dd9): <https://commits.webkit.org/294318@main>

Reviewed commits have been landed. Closing PR #44646 and removing active labels.