Skip to content

When the delegate is unavailable in PlaceholderRenderingContextSource::setPlaceholderBuffer, save the image buffer and propagate it later to the delegate #42688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from May 22, 2025
Merged

When the delegate is unavailable in PlaceholderRenderingContextSource::setPlaceholderBuffer, save the image buffer and propagate it later to the delegate #42688

merged 1 commit into from May 22, 2025

Conversation

ghost
Copy link

@ghost ghost commented Mar 19, 2025 •
edited by webkit-early-warning-system
Loading

153da00

When the delegate is unavailable in `PlaceholderRenderingContextSource::setPlaceholderBuffer`, save the image buffer and propagate it later to the delegate
https://bugs.webkit.org/show_bug.cgi?id=290042

Reviewed by Matt Woodrow.

Ensures the image buffer is propagated to the delegate, when the former
becomes available before the latter. This happened reliably for
`canvas.2d.fillText-FontFace.html`.

Combined changes:
* LayoutTests/imported/w3c/web-platform-tests/html/canvas/offscreen/manual/text/canvas.2d.fillText-FontFace-expected-mismatch.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/canvas/offscreen/manual/text/canvas.2d.fillText-FontFace.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/canvas/offscreen/manual/text/canvas.2d.fillText-expected-mismatch.html: Added.

* LayoutTests/imported/w3c/web-platform-tests/html/canvas/offscreen/manual/text/canvas.2d.fillText.html:
This test passed already without the fix.
The actual issue could only reliably (dozens of times) be reproduced by
opening this test file in GTK's MiniBrowser with the mouse arrow symbol
next to the browser window. With the fix, the issue is still
reproducible.

* LayoutTests/imported/w3c/web-platform-tests/html/canvas/offscreen/manual/text/empty-ref.html: Added.

* LayoutTests/platform/mac-wk1/TestExpectations:
Tried reproducing the issue for WK1 with GTK, but GTK seems to lack
support for WK1: https://bugs.webkit.org/show_bug.cgi?id=290445.

* Source/WebCore/html/OffscreenCanvas.cpp:
(WebCore::OffscreenCanvas::commitToPlaceholderCanvas):
* Source/WebCore/html/canvas/PlaceholderRenderingContext.cpp:
(WebCore::PlaceholderRenderingContextSource::setPlaceholderBuffer):
(WebCore::PlaceholderRenderingContextSource::setContentsToLayer):
* Source/WebCore/html/canvas/PlaceholderRenderingContext.h:

Canonical link: https://commits.webkit.org/295259@main

441c35d

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@ghost ghost self-assigned this Mar 19, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Mar 19, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 05f6b14)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ❌ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 19, 2025
@ghost ghost removed the merging-blocked Applied to prevent a change from being merged label Mar 25, 2025
@ghost ghost changed the title Add WPT that fillText after transferControlToOffscreen for canvas draws text Override hasDeferredOperations for PlaceHolderRenderingContext Mar 25, 2025
@ghost ghost added the Canvas Bugs related to the canvas element. label Mar 25, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Mar 25, 2025 •
edited
Loading

EWS run on previous version of this PR (hash f80bc58)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ❌ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@aproskuryakov aproskuryakov requested review from smfr and shallawa March 25, 2025 15:40
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 25, 2025
@smfr smfr requested a review from mattwoodrow March 25, 2025 16:07
@ghost ghost removed the merging-blocked Applied to prevent a change from being merged label Mar 26, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Mar 26, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 215f887)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac ❌ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
  🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision   🧪 mac-AS-debug-wk2   🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress   🧪 api-gtk
✅ 🧪 vision-wk2 ⏳ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@ghost ghost marked this pull request as ready for review March 26, 2025 07:37
@ghost ghost requested review from cdumez and rniwa as code owners March 26, 2025 07:37
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Mar 26, 2025 •
edited
Loading

EWS run on previous version of this PR (hash d52a939)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@ghost ghost changed the title Override hasDeferredOperations for PlaceHolderRenderingContext Import WPTs from folder html/canvas/element/manual/text Mar 27, 2025
@ghost ghost closed this Mar 27, 2025
@ghost ghost reopened this Mar 27, 2025
@ghost ghost changed the title Import WPTs from folder html/canvas/element/manual/text Override hasDeferredOperations for PlaceHolderRenderingContext Mar 27, 2025
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 27, 2025
mattwoodrow
mattwoodrow reviewed Mar 28, 2025
View reviewed changes
@ghost ghost removed the merging-blocked Applied to prevent a change from being merged label Apr 3, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Apr 3, 2025 •
edited
Loading

EWS run on previous version of this PR (hash cf9cb83)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac ✅ 🧪 api-wpe
  🧪 ios-wk2-wpt   🧪 mac-wk1 ✅ 🛠 wpe-cairo
  🧪 api-ios   🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision   🧪 mac-AS-debug-wk2   🧪 gtk-wk2
✅ 🛠 vision-sim   🧪 mac-wk2-stress ✅ 🧪 api-gtk
  🧪 vision-wk2   🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@ghost ghost marked this pull request as draft April 3, 2025 13:28
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Apr 3, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 3304e5f)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac   🛠 wpe   🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2   🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac   🧪 api-wpe
  🧪 ios-wk2-wpt   🧪 mac-wk1   🛠 wpe-cairo
  🧪 api-ios   🧪 mac-wk2   🛠 gtk
✅ 🛠 vision   🧪 mac-AS-debug-wk2   🧪 gtk-wk2
✅ 🛠 vision-sim   🧪 mac-wk2-stress   🧪 api-gtk
  🧪 vision-wk2   🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Apr 3, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 995fd46)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ❌ 🧪 api-mac ✅ 🧪 api-wpe
❌ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ❌ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ❌ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 vision-sim ❌ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ❌ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 3, 2025
kkinnunen-apple
kkinnunen-apple reviewed Apr 3, 2025
View reviewed changes
@ghost ghost removed the merging-blocked Applied to prevent a change from being merged label May 14, 2025
@ghost ghost changed the title Override hasDeferredOperations for PlaceHolderRenderingContext When the delegate is unavailable in PlaceholderRenderingContextSource::setPlaceholderBuffer, save the image buffer and propagate it later to the delegate May 14, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 14, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 4394d2d)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac   🧪 api-wpe
  🧪 ios-wk2-wpt   🧪 mac-wk1 ✅ 🛠 wpe-cairo
  🧪 api-ios   🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2   🧪 gtk-wk2
✅ 🛠 vision-sim   🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2   🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 14, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 437d582)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
⏳ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
⏳ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@ghost ghost requested review from mattwoodrow and kkinnunen-apple May 15, 2025 07:09
@ghost ghost marked this pull request as ready for review May 15, 2025 07:10
mattwoodrow
mattwoodrow approved these changes May 19, 2025
View reviewed changes
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 21, 2025 •
edited
Loading

EWS run on current version of this PR (hash 441c35d)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@ghost ghost mentioned this pull request May 21, 2025
Merged
@ghost
Copy link
Author

ghost commented May 21, 2025

@mattwoodrow: can you or someone else please add one of the merge labels for this PR? I'm not a WebKit committer.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 21, 2025
@aperezdc aperezdc added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels May 22, 2025
@aperezdc
When the delegate is unavailable in `PlaceholderRenderingContextSourc…
153da00 
…e::setPlaceholderBuffer`, save the image buffer and propagate it later to the delegate

https://bugs.webkit.org/show_bug.cgi?id=290042

Reviewed by Matt Woodrow.

Ensures the image buffer is propagated to the delegate, when the former
becomes available before the latter. This happened reliably for
`canvas.2d.fillText-FontFace.html`.

Combined changes:
* LayoutTests/imported/w3c/web-platform-tests/html/canvas/offscreen/manual/text/canvas.2d.fillText-FontFace-expected-mismatch.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/canvas/offscreen/manual/text/canvas.2d.fillText-FontFace.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/canvas/offscreen/manual/text/canvas.2d.fillText-expected-mismatch.html: Added.

* LayoutTests/imported/w3c/web-platform-tests/html/canvas/offscreen/manual/text/canvas.2d.fillText.html:
This test passed already without the fix.
The actual issue could only reliably (dozens of times) be reproduced by
opening this test file in GTK's MiniBrowser with the mouse arrow symbol
next to the browser window. With the fix, the issue is still
reproducible.

* LayoutTests/imported/w3c/web-platform-tests/html/canvas/offscreen/manual/text/empty-ref.html: Added.

* LayoutTests/platform/mac-wk1/TestExpectations:
Tried reproducing the issue for WK1 with GTK, but GTK seems to lack
support for WK1: https://bugs.webkit.org/show_bug.cgi?id=290445.

* Source/WebCore/html/OffscreenCanvas.cpp:
(WebCore::OffscreenCanvas::commitToPlaceholderCanvas):
* Source/WebCore/html/canvas/PlaceholderRenderingContext.cpp:
(WebCore::PlaceholderRenderingContextSource::setPlaceholderBuffer):
(WebCore::PlaceholderRenderingContextSource::setContentsToLayer):
* Source/WebCore/html/canvas/PlaceholderRenderingContext.h:

Canonical link: https://commits.webkit.org/295259@main
@webkit-commit-queue
Copy link
Collaborator

Committed 295259@main (153da00): https://commits.webkit.org/295259@main

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

@webkit-commit-queue webkit-commit-queue merged commit 153da00 into WebKit:main May 22, 2025
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 22, 2025
@darinadler
Copy link
Member

This led to a regression because the thread safety of the new code was incorrect; @mattwoodrow pointed out that ImageBuffer should not be thread-safe reference counted. So we will be reverting this fix and doing a new corrected one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smfr smfr Awaiting requested review from smfr

@shallawa shallawa Awaiting requested review from shallawa

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@rniwa rniwa Awaiting requested review from rniwa rniwa is a code owner

@kkinnunen-apple kkinnunen-apple Awaiting requested review from kkinnunen-apple

Assignees
No one assigned
Labels
Canvas Bugs related to the canvas element.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@webkit-early-warning-system @webkit-commit-queue @darinadler @mattwoodrow @kkinnunen-apple @aperezdc @webkit-ews-buildbot