Skip to content

Conversation

@aestes
Copy link
Contributor

@aestes aestes commented Feb 22, 2024

d97f5a0

getUserMedia camera stream lost on history pushState in iOS 17.4 Beta 4
https://bugs.webkit.org/show_bug.cgi?id=269846
rdar://123381737

Reviewed by Eric Carlson and Youenn Fablet.

When WebKit adopted media capability grants for camera capture we chose to tie the lifetime of the
media environment to the top frame document's current URL, such that if the URL changes (ignoring
fragment identifiers) then the current media environment is destroyed and a new one is created. If
a capture session is active when the media environment changes then the system will pause the
capture session as it's no longer associated with the current media environment. The logic of
comparing URLs was meant as a proxy for detecting cross-document navigations but failed to account
for same-document navigations that changed the path of the current URL (e.g., via pushState). Since
the origin associated with the media environment does not change during a same-document navigation
there is no need to recreate the media environment.

Addressed this by moving the logic for creating and destroying media environments to
WebPageProxy::didChangeMainDocument. Further, since only the top document's origin is displayed to
the user in iOS's privacy accounting UI, changed MediaCapability to only track a SecurityOrigin.
The logic for when to activate and deactivate a media environment's capability remains unchanged.

Manually verified that this resolves the issue reported in bug #269846. Unfortunately no new tests
are possible since the underlying platform support for media capabilities is not available in
iOS Simulator.

* Source/WebKit/Platform/cocoa/MediaCapability.h:
* Source/WebKit/Platform/cocoa/MediaCapability.mm:
(WebKit::MediaCapability::MediaCapability):
(WebKit::m_mediaEnvironment):
(WebKit::MediaCapability::registrableDomain const): Deleted.
* Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:
(WebKit::WebPageProxy::setMediaCapability):
(WebKit::WebPageProxy::deactivateMediaCapability):
(WebKit::WebPageProxy::resetMediaCapability):
(WebKit::WebPageProxy::updateMediaCapability):
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::didCommitLoadForFrame):
(WebKit::WebPageProxy::didChangeMainDocument):
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:

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

0d35585

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@aestes aestes requested a review from cdumez as a code owner February 22, 2024 21:28
@aestes aestes self-assigned this Feb 22, 2024
@aestes aestes added the WebRTC For bugs in WebRTC label Feb 22, 2024
@aestes aestes force-pushed the eng/getUserMedia-camera-stream-lost-on-history-pushState-in-iOS-17-4-Beta-4 branch from 99fe242 to ef4db42 Compare February 22, 2024 21:30
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 22, 2024
@aestes aestes removed the merging-blocked Applied to prevent a change from being merged label Feb 23, 2024
@aestes aestes force-pushed the eng/getUserMedia-camera-stream-lost-on-history-pushState-in-iOS-17-4-Beta-4 branch from ef4db42 to 0d35585 Compare February 23, 2024 01:58
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 23, 2024
Copy link
Contributor

@youennf youennf left a comment

Choose a reason for hiding this comment

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

LGTM.
This is a preexisting issue but I wonder whether we should create (not in this patch) MediaCapability at the time we need it (for instance when starting to capture).
This might be more difficult for non capture related stuff.

@aestes aestes 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 Feb 23, 2024
@aestes
Copy link
Contributor Author

aestes commented Feb 23, 2024

LGTM. This is a preexisting issue but I wonder whether we should create (not in this patch) MediaCapability at the time we need it (for instance when starting to capture). This might be more difficult for non capture related stuff.

We likely could do this, but there isn't much harm in creating the MediaCapability eagerly. It simplifies the logic for ensuring the environment is propagated to the GPU process prior to creating an AVCaptureSession, and creating a MediaCapability in the deactivated state isn't expensive.

https://bugs.webkit.org/show_bug.cgi?id=269846
rdar://123381737

Reviewed by Eric Carlson and Youenn Fablet.

When WebKit adopted media capability grants for camera capture we chose to tie the lifetime of the
media environment to the top frame document's current URL, such that if the URL changes (ignoring
fragment identifiers) then the current media environment is destroyed and a new one is created. If
a capture session is active when the media environment changes then the system will pause the
capture session as it's no longer associated with the current media environment. The logic of
comparing URLs was meant as a proxy for detecting cross-document navigations but failed to account
for same-document navigations that changed the path of the current URL (e.g., via pushState). Since
the origin associated with the media environment does not change during a same-document navigation
there is no need to recreate the media environment.

Addressed this by moving the logic for creating and destroying media environments to
WebPageProxy::didChangeMainDocument. Further, since only the top document's origin is displayed to
the user in iOS's privacy accounting UI, changed MediaCapability to only track a SecurityOrigin.
The logic for when to activate and deactivate a media environment's capability remains unchanged.

Manually verified that this resolves the issue reported in bug #269846. Unfortunately no new tests
are possible since the underlying platform support for media capabilities is not available in
iOS Simulator.

* Source/WebKit/Platform/cocoa/MediaCapability.h:
* Source/WebKit/Platform/cocoa/MediaCapability.mm:
(WebKit::MediaCapability::MediaCapability):
(WebKit::m_mediaEnvironment):
(WebKit::MediaCapability::registrableDomain const): Deleted.
* Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:
(WebKit::WebPageProxy::setMediaCapability):
(WebKit::WebPageProxy::deactivateMediaCapability):
(WebKit::WebPageProxy::resetMediaCapability):
(WebKit::WebPageProxy::updateMediaCapability):
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::didCommitLoadForFrame):
(WebKit::WebPageProxy::didChangeMainDocument):
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:

Canonical link: https://commits.webkit.org/275244@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/getUserMedia-camera-stream-lost-on-history-pushState-in-iOS-17-4-Beta-4 branch from 0d35585 to d97f5a0 Compare February 23, 2024 17:54
@webkit-commit-queue
Copy link
Collaborator

Committed 275244@main (d97f5a0): https://commits.webkit.org/275244@main

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

@webkit-commit-queue webkit-commit-queue merged commit d97f5a0 into WebKit:main Feb 23, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 23, 2024
@aestes aestes deleted the eng/getUserMedia-camera-stream-lost-on-history-pushState-in-iOS-17-4-Beta-4 branch August 25, 2025 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WebRTC For bugs in WebRTC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants