Skip to content

fillText after transferControlToOffscreen for canvas doesn't draw #47233

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
Jun 27, 2025
Merged

fillText after transferControlToOffscreen for canvas doesn't draw #47233

merged 1 commit into from
Jun 27, 2025

Conversation

kkinnunen-apple
Copy link
Contributor

@kkinnunen-apple kkinnunen-apple commented Jun 26, 2025 •
edited by webkit-early-warning-system
Loading

323a593

`fillText` after `transferControlToOffscreen` for canvas doesn't draw
https://bugs.webkit.org/show_bug.cgi?id=290042
rdar://147925254

Reviewed by Matt Woodrow.

PlaceholderRenderingContextSource::setPlaceholderBuffer can be called before the
delegate is provided (either due to a race, or due to the <canvas> element for
which this is a placeholder not being in a Document).

* LayoutTests/TestExpectations:
* Source/WebCore/html/canvas/PlaceholderRenderingContext.cpp:
(WebCore::PlaceholderRenderingContextSource::setPlaceholderBuffer):
(WebCore::PlaceholderRenderingContextSource::setContentsToLayer):
(WebCore::PlaceholderRenderingContext::setContentsToLayer):
* Source/WebCore/html/canvas/PlaceholderRenderingContext.h:
(WebCore::PlaceholderRenderingContextSource::WTF_GUARDED_BY_LOCK):
(WebCore::PlaceholderRenderingContextSource::WTF_GUARDED_BY_CAPABILITY):

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

69b1ded

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

@kkinnunen-apple kkinnunen-apple self-assigned this Jun 26, 2025
@kkinnunen-apple kkinnunen-apple added the Canvas Bugs related to the canvas element. label Jun 26, 2025
@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on previous version of this PR (hash 6c52b86)

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
Copy link
Collaborator

Safer C++ Build #41670 (6c52b86)

❌ Found 1 failing file with 2 issues. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming.
(cc @rniwa)

@kkinnunen-apple kkinnunen-apple force-pushed the offscreencanvas-delegate-first-frame-race-1 branch from 6c52b86 to 0330c20 Compare June 26, 2025 19:08
@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on previous version of this PR (hash 0330c20)

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

mattwoodrow
mattwoodrow approved these changes Jun 26, 2025
View reviewed changes
Source/WebCore/html/canvas/PlaceholderRenderingContext.h
@@ -48,14 +48,17 @@ class PlaceholderRenderingContextSource : public ThreadSafeRefCounted<Placeholde
void setPlaceholderBuffer(ImageBuffer&);

// Called by the placeholder context to attach to compositor layer.
void setContentsToLayer(GraphicsLayer&);
void setContentsToLayer(GraphicsLayer&, ImageBuffer*);

private:
explicit PlaceholderRenderingContextSource(PlaceholderRenderingContext&);

WeakPtr<PlaceholderRenderingContext> m_placeholder; // For main thread use.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add WTF_GUARDED_BY_CAPABILITY(mainThread) to this one too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot, the weakptr pointer itself is copied at worker thread to the main thread function closure.

Source/WebCore/html/canvas/PlaceholderRenderingContext.h Outdated

private:
explicit PlaceholderRenderingContextSource(PlaceholderRenderingContext&);

WeakPtr<PlaceholderRenderingContext> m_placeholder; // For main thread use.
Lock m_lock;
RefPtr<GraphicsLayerAsyncContentsDisplayDelegate> m_delegate WTF_GUARDED_BY_LOCK(m_lock);
unsigned m_bufferVersion { 0 };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can't use WTF_GUARDED_BY_CAPABILITY(!mainThread), but maybe just a comment that it's from the placeholdee's thread?

@kkinnunen-apple kkinnunen-apple force-pushed the offscreencanvas-delegate-first-frame-race-1 branch from 0330c20 to cb6dc77 Compare June 27, 2025 07:58
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 27, 2025 •
edited
Loading

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

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
✅ 🛠 🧪 unsafe-merge ✅ 🛠 tv ✅ 🛠 mac-safer-cpp
  🛠 tv-sim
✅ 🛠 watch
  🛠 watch-sim

@kkinnunen-apple kkinnunen-apple changed the title fillText after transferControlToOffscreen for canvas doesn't draw Placeholder canvas does not always propagate HDR content format correctly to the layer Jun 27, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 27, 2025 •
edited
Loading

EWS run on current version of this PR (hash 69b1ded)

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

@kkinnunen-apple kkinnunen-apple changed the title Placeholder canvas does not always propagate HDR content format correctly to the layer fillText after transferControlToOffscreen for canvas doesn't draw Jun 27, 2025
@kkinnunen-apple kkinnunen-apple force-pushed the offscreencanvas-delegate-first-frame-race-1 branch from 69b1ded to cb6dc77 Compare June 27, 2025 11:06
@kkinnunen-apple kkinnunen-apple added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 27, 2025
@kkinnunen-apple
fillText after transferControlToOffscreen for canvas doesn't draw
323a593 
https://bugs.webkit.org/show_bug.cgi?id=290042
rdar://147925254

Reviewed by Matt Woodrow.

PlaceholderRenderingContextSource::setPlaceholderBuffer can be called before the
delegate is provided (either due to a race, or due to the <canvas> element for
which this is a placeholder not being in a Document).

* LayoutTests/TestExpectations:
* Source/WebCore/html/canvas/PlaceholderRenderingContext.cpp:
(WebCore::PlaceholderRenderingContextSource::setPlaceholderBuffer):
(WebCore::PlaceholderRenderingContextSource::setContentsToLayer):
(WebCore::PlaceholderRenderingContext::setContentsToLayer):
* Source/WebCore/html/canvas/PlaceholderRenderingContext.h:
(WebCore::PlaceholderRenderingContextSource::WTF_GUARDED_BY_LOCK):
(WebCore::PlaceholderRenderingContextSource::WTF_GUARDED_BY_CAPABILITY):

Canonical link: https://commits.webkit.org/296714@main
@webkit-commit-queue webkit-commit-queue force-pushed the offscreencanvas-delegate-first-frame-race-1 branch from cb6dc77 to 323a593 Compare June 27, 2025 11:09
@webkit-commit-queue
Copy link
Collaborator

Committed 296714@main (323a593): https://commits.webkit.org/296714@main

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

@webkit-commit-queue webkit-commit-queue merged commit 323a593 into WebKit:main Jun 27, 2025
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattwoodrow mattwoodrow mattwoodrow approved these changes

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

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

Assignees

@kkinnunen-apple kkinnunen-apple

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

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