Summary: | [MediaStream] Don't request user permission for a device if it has already been granted in the current browsing context | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, dino, jer.noble, rniwa, ryanhaddad, thiago.lacerda, webkit-bug-importer, youennf | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | Other | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Eric Carlson
2016-11-14 19:39:27 PST
Created attachment 294834 [details]
Proposed patch.
Comment on attachment 294834 [details] Proposed patch. Attachment 294834 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2520184 New failing tests: fast/mediastream/MediaStream-MediaElement-srcObject.html Created attachment 294837 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 294838 [details]
Proposed patch.
Comment on attachment 294838 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=294838&action=review > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:51 > + if (deviceUID.isEmpty()) Can this case happen? > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:72 > + } while (0); Is it left-over code, or were you planning to add something like ASSERT(!securityOriginsAreEqual(request))? > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:81 > + if (!m_userMediaDocumentSecurityOrigin || !m_userMediaDocumentSecurityOrigin->equal(request.userMediaDocumentSecurityOrigin())) I am unclear whether SecurityOrigin::equal is the right routine to call, in particular the check for m_domainWasSetInDOM. It seems the best choice AFAICS though. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:92 > + state->reset(request); I guess reset and securityOriginsAreEqual could be grouped into one function. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:121 > + Ref<UserMediaPermissionRequestProxy> request = UserMediaPermissionRequestProxy::create(*this, userMediaID, frameID, userMediaDocumentOriginIdentifier, topLevelDocumentOriginIdentifier, audioDeviceUIDs, videoDeviceUIDs); Use auto here. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:256 > auto topLevelOrigin = API::SecurityOrigin::create(SecurityOriginData::fromDatabaseIdentifier(topLevelDocumentOriginIdentifier)->securityOrigin()); userMediaOrigin is a RefPtr I guess while it should be a Ref. That would allow removing the *userMediaOrigin.get() below. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:262 > + for (auto deviceUID : audioDeviceUIDs) { Would "for (const auto&" be more efficient? > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:266 > + } I wonder whether this pattern could be done at vector level, something like T* firstMatching(const MatchFunction&) and its const version. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:40 > + FrameAuthorizationState(UserMediaPermissionRequestProxy&); Should be explicit. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:41 > + ~FrameAuthorizationState() { } Do we need it? > Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp:38 > + , m_topLevelDocumentSecurityOrigin(SecurityOriginData::fromDatabaseIdentifier(topLevelDocumentOriginIdentifier)->securityOrigin()) Has this strategy of persistent granting the potential to be reused in other APIs? If so, I would introduce a structure for frameID and origins. That would also reduce the number of passed parameters which might improve readability. > LayoutTests/fast/mediastream/MediaDevices-getUserMedia.html:48 > + // successfully created a stream for the mock audio device. Have we tests that exercice the case of no-persistency? Like the case of equal returning false for obvious reasons (port change) but also non obvious (dom flag I talked above) Comment on attachment 294838 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=294838&action=review >> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:51 >> + if (deviceUID.isEmpty()) > > Can this case happen? Yes, because userMediaAccessWasDenied() is sometimes called before a request is created (e.g. when the UIClient doesn't implement decidePolicyForUserMediaPermissionRequest). >> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:72 >> + } while (0); > > Is it left-over code, or were you planning to add something like ASSERT(!securityOriginsAreEqual(request))? I combined reset and securityOriginsAreEqual. >> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:92 >> + state->reset(request); > > I guess reset and securityOriginsAreEqual could be grouped into one function. Good idea, fixed. >> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:121 >> + Ref<UserMediaPermissionRequestProxy> request = UserMediaPermissionRequestProxy::create(*this, userMediaID, frameID, userMediaDocumentOriginIdentifier, topLevelDocumentOriginIdentifier, audioDeviceUIDs, videoDeviceUIDs); > > Use auto here. Fixed. >> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:262 >> + for (auto deviceUID : audioDeviceUIDs) { > > Would "for (const auto&" be more efficient? Fixed. >> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:266 >> + } > > I wonder whether this pattern could be done at vector level, something like T* firstMatching(const MatchFunction&) and its const version. I will leave that for another day. >> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:40 >> + FrameAuthorizationState(UserMediaPermissionRequestProxy&); > > Should be explicit. Fixed. >> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:41 >> + ~FrameAuthorizationState() { } > > Do we need it? No, removed. >> LayoutTests/fast/mediastream/MediaDevices-getUserMedia.html:48 >> + // successfully created a stream for the mock audio device. > > Have we tests that exercice the case of no-persistency? > Like the case of equal returning false for obvious reasons (port change) but also non obvious (dom flag I talked above) I modified WTR to track the number of times a gUM call makes it to the UA, and added methods so it can be read and reset from script, and added a test. Created attachment 295500 [details]
Patch for landing.
Comment on attachment 295500 [details] Patch for landing. Rejecting attachment 295500 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 295500, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/2584177 Created attachment 295509 [details]
Updated patch for landing.
Comment on attachment 295509 [details] Updated patch for landing. Clearing flags on attachment: 295509 Committed r209008: <http://trac.webkit.org/changeset/209008> All reviewed patches have been landed. Closing bug. (In reply to comment #11) > Comment on attachment 295509 [details] > Updated patch for landing. > > Clearing flags on attachment: 295509 > > Committed r209008: <http://trac.webkit.org/changeset/209008> This change appears to have caused two fast/mediastream test issues: Failure: fast/mediastream/MediaDevices-getUserMedia.html Timeout: fast/mediastream/delayed-permission-allowed.html https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r209012%20(11295)/results.html Reverted r209008 for reason: This change appears to have caused two fast/mediastrem LayoutTests to fail. Committed r209024: <http://trac.webkit.org/changeset/209024> Created attachment 295605 [details]
Another updated patch for landing.
Created attachment 295606 [details]
Updated patch for landing.
Comment on attachment 295606 [details] Updated patch for landing. Clearing flags on attachment: 295606 Committed r209082: <http://trac.webkit.org/changeset/209082> All reviewed patches have been landed. Closing bug. |