Skip to content

REGRESSION(296514@main): Triggering assertions in debug iOS tests #47130

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 25, 2025
Merged

REGRESSION(296514@main): Triggering assertions in debug iOS tests #47130

merged 1 commit into from
Jun 25, 2025

Conversation

pvollan
Copy link
Contributor

@pvollan pvollan commented Jun 24, 2025 •
edited by webkit-early-warning-system
Loading

5c3ce74

REGRESSION(296514@main): Triggering assertions in debug iOS tests
https://bugs.webkit.org/show_bug.cgi?id=294916
rdar://154213939

Reviewed by Sihui Liu.

After 296514@main, we need to be sending a MachSendRightAnnotated object instead of a MachSendRight
when updating bounds and position of a RemoteSampleBufferDisplayLayer.

* Source/WebCore/platform/graphics/avfoundation/SampleBufferDisplayLayer.h:
* Source/WebCore/platform/graphics/avfoundation/objc/LocalSampleBufferDisplayLayer.h:
* Source/WebCore/platform/graphics/avfoundation/objc/LocalSampleBufferDisplayLayer.mm:
(WebCore::LocalSampleBufferDisplayLayer::updateBoundsAndPosition):
* Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:
(WebCore::MediaPlayerPrivateMediaStreamAVFObjC::setVideoLayerSizeFenced):
* Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:
(WebKit::GPUConnectionToWebProcess::updateSampleBufferDisplayLayerBoundsAndPosition):
* Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h:
* Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in:
* Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.h:
* Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.mm:
(WebKit::RemoteSampleBufferDisplayLayer::updateBoundsAndPosition):
* Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayerManager.cpp:
(WebKit::RemoteSampleBufferDisplayLayerManager::updateSampleBufferDisplayLayerBoundsAndPosition):
* Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayerManager.h:
* Source/WebKit/WebProcess/GPU/webrtc/SampleBufferDisplayLayer.cpp:
(WebKit::SampleBufferDisplayLayer::updateBoundsAndPosition):
* Source/WebKit/WebProcess/GPU/webrtc/SampleBufferDisplayLayer.h:

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

4a783bf

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

@pvollan pvollan requested a review from cdumez as a code owner June 24, 2025 19:10
@pvollan pvollan self-assigned this Jun 24, 2025
@pvollan pvollan added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Jun 24, 2025
@webkit-early-warning-system
Copy link
Collaborator

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

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

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 Jun 24, 2025
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Jun 24, 2025
@pvollan pvollan force-pushed the eng/REGRESSION-296514-main-Triggering-assertions-in-debug-iOS-tests branch from a5b4c53 to 0d2cf20 Compare June 24, 2025 20:39
@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on previous version of this PR (hash 0d2cf20)

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

@pvollan pvollan force-pushed the eng/REGRESSION-296514-main-Triggering-assertions-in-debug-iOS-tests branch from 0d2cf20 to 9ac3062 Compare June 24, 2025 20:52
@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on previous version of this PR (hash 9ac3062)

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

@pvollan pvollan force-pushed the eng/REGRESSION-296514-main-Triggering-assertions-in-debug-iOS-tests branch from 9ac3062 to 9761851 Compare June 24, 2025 21:28
@webkit-early-warning-system
Copy link
Collaborator

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

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

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

basuke
basuke reviewed Jun 24, 2025
View reviewed changes
Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.mm Outdated
[hostingUpdateCoordinator addLayerHierarchy:m_layerHostingContext->hostable().get()];
}
protectedSampleBufferDisplayLayer()->updateBoundsAndPosition(bounds, { });
[hostingUpdateCoordinator commit];
Copy link
Contributor

Choose a reason for hiding this comment

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

hostingUpdateCoordinator may be nil. Is it okay? Sending message to nil is acceptable but smelling bad behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point! I added checks and an error message.

Thanks for reviewing!

@pvollan pvollan force-pushed the eng/REGRESSION-296514-main-Triggering-assertions-in-debug-iOS-tests branch from 9761851 to 4a783bf Compare June 24, 2025 23:44
@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on current version of this PR (hash 4a783bf)

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

@pvollan pvollan requested review from basuke and szewai June 24, 2025 23:45
szewai
szewai approved these changes Jun 25, 2025
View reviewed changes
Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.mm
@@ -110,12 +110,30 @@
protectedSampleBufferDisplayLayer()->updateDisplayMode(hideDisplayLayer, hideRootLayer);
}

void RemoteSampleBufferDisplayLayer::updateBoundsAndPosition(CGRect bounds, std::optional<WTF::MachSendRight>&& fence)
void RemoteSampleBufferDisplayLayer::updateBoundsAndPosition(CGRect bounds, std::optional<WTF::MachSendRightAnnotated>&& fence)
Copy link
Contributor

Choose a reason for hiding this comment

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

@youennf I am going to r+ since the fix is needed for bot to run; you might want to take a look when you have time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing!

@pvollan pvollan requested a review from youennf June 25, 2025 01:29
@pvollan pvollan added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 25, 2025
@pvollan
REGRESSION(296514@main): Triggering assertions in debug iOS tests
5c3ce74 
https://bugs.webkit.org/show_bug.cgi?id=294916
rdar://154213939

Reviewed by Sihui Liu.

After 296514@main, we need to be sending a MachSendRightAnnotated object instead of a MachSendRight
when updating bounds and position of a RemoteSampleBufferDisplayLayer.

* Source/WebCore/platform/graphics/avfoundation/SampleBufferDisplayLayer.h:
* Source/WebCore/platform/graphics/avfoundation/objc/LocalSampleBufferDisplayLayer.h:
* Source/WebCore/platform/graphics/avfoundation/objc/LocalSampleBufferDisplayLayer.mm:
(WebCore::LocalSampleBufferDisplayLayer::updateBoundsAndPosition):
* Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:
(WebCore::MediaPlayerPrivateMediaStreamAVFObjC::setVideoLayerSizeFenced):
* Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:
(WebKit::GPUConnectionToWebProcess::updateSampleBufferDisplayLayerBoundsAndPosition):
* Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h:
* Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in:
* Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.h:
* Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.mm:
(WebKit::RemoteSampleBufferDisplayLayer::updateBoundsAndPosition):
* Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayerManager.cpp:
(WebKit::RemoteSampleBufferDisplayLayerManager::updateSampleBufferDisplayLayerBoundsAndPosition):
* Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayerManager.h:
* Source/WebKit/WebProcess/GPU/webrtc/SampleBufferDisplayLayer.cpp:
(WebKit::SampleBufferDisplayLayer::updateBoundsAndPosition):
* Source/WebKit/WebProcess/GPU/webrtc/SampleBufferDisplayLayer.h:

Canonical link: https://commits.webkit.org/296603@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/REGRESSION-296514-main-Triggering-assertions-in-debug-iOS-tests branch from 4a783bf to 5c3ce74 Compare June 25, 2025 04:16
@webkit-commit-queue
Copy link
Collaborator

Committed 296603@main (5c3ce74): https://commits.webkit.org/296603@main

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

@webkit-commit-queue webkit-commit-queue merged commit 5c3ce74 into WebKit:main Jun 25, 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 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szewai szewai szewai approved these changes

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

@basuke basuke Awaiting requested review from basuke

@youennf youennf Awaiting requested review from youennf

Assignees

@pvollan pvollan

Labels
New Bugs Unclassified bugs are placed in this component until the correct component can be determined.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pvollan @webkit-early-warning-system @webkit-commit-queue @basuke @szewai @webkit-ews-buildbot