209712 2020-03-28 19:44:03 -0700 REGRESSION (r257867): [GPUP] Use-after-move in RemoteCDMInstance::createSession() 2020-04-04 10:01:05 -0700 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 210006 1 ddkilzer ddkilzer darin eric.carlson ews-watchlist glenn jer.noble philipj sergio webkit-bug-importer oldest_to_newest 1635278 0 ddkilzer 2020-03-28 19:44:03 -0700 Use-after-move in RemoteCDMInstance::createSession() of `id` parameter: RefPtr<WebCore::CDMInstanceSession> RemoteCDMInstance::createSession() { if (!m_factory) return nullptr; RemoteCDMInstanceSessionIdentifier id; m_factory->gpuProcessConnection().connection().sendSync(Messages::RemoteCDMInstanceProxy::CreateSession(), Messages::RemoteCDMInstanceProxy::CreateSession::Reply(id), m_identifier); if (!id) return nullptr; auto session = RemoteCDMInstanceSession::create(makeWeakPtr(m_factory.get()), WTFMove(id)); m_factory->addSession(id, session.copyRef()); return session; } This regressed in: Bug 208090: [GPUP] Implement Modern EME API in the GPU Process <https://bugs.webkit.org/show_bug.cgi?id=208090> <rdar://problem/59955230> <https://trac.webkit.org/changeset/257867> 1635279 1 webkit-bug-importer 2020-03-28 19:44:25 -0700 <rdar://problem/61018795> 1635281 2 394852 ddkilzer 2020-03-28 19:51:20 -0700 Created attachment 394852 Patch v1 1635283 3 394852 darin 2020-03-28 19:52:35 -0700 Comment on attachment 394852 Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=394852&action=review > Source/WebKit/WebProcess/GPU/media/RemoteCDMInstance.cpp:94 > auto session = RemoteCDMInstanceSession::create(makeWeakPtr(m_factory.get()), WTFMove(id)); > - m_factory->addSession(id, session.copyRef()); > + m_factory->addSession(session->identifier(), session.copyRef()); How about just removing the WTFMove instead? > Source/WebKit/WebProcess/GPU/media/RemoteCDMInstanceSession.h:46 > + RemoteCDMInstanceSessionIdentifier identifier() const { return m_identifier; } Seems like this is pretty far to go just to fix the use after move. 1635286 4 394852 ddkilzer 2020-03-28 19:54:55 -0700 Comment on attachment 394852 Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=394852&action=review >> Source/WebKit/WebProcess/GPU/media/RemoteCDMInstanceSession.h:46 >> + RemoteCDMInstanceSessionIdentifier identifier() const { return m_identifier; } > > Seems like this is pretty far to go just to fix the use after move. Adding this getter makes it possible for RemoteLegacyCDMFactory::addSession() not to even need the RemoteCDMInstanceSessionIdentifier argument as it can get it from the session object passed in. I was focusing on making the smallest change possible to fix the use-after-move, but should I go ahead and make that change as well? 1635289 5 darin 2020-03-28 19:56:32 -0700 Yes, keep this patch as small as possible. You can do other changes in another patch that is not fixing an obvious problem. 1635321 6 ews-feeder 2020-03-29 01:52:44 -0700 Committed r259176: <https://trac.webkit.org/changeset/259176> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394852. 1637699 7 ddkilzer 2020-04-04 10:01:05 -0700 (In reply to Darin Adler from comment #5) > Yes, keep this patch as small as possible. You can do other changes in > another patch that is not fixing an obvious problem. Bug 210006: [GPUP] Remove unnecessary RemoteCDMInstanceSessionIdentifier argument from RemoteCDMFactory::addSession() 394852 2020-03-28 19:51:20 -0700 2020-03-29 01:52:45 -0700 Patch v1 bug-209712-20200328195119.patch text/plain 2468 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMjU5MTY0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDc0M2EzNDhjNTgxNmVlYmE4 Yzk3OGUyYzhmZGZiNTY0NjI5MzI4ZWIuLjY0ZjM3YmY0MTUxN2YzNzhkYmY2ZTJhMmRkZDI0ODJl YjUwMzAyOTcgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMjAtMDMtMjggIERhdmlkIEtp bHplciAgPGRka2lsemVyQGFwcGxlLmNvbT4KKworICAgICAgICBSRUdSRVNTSU9OIChyMjU3ODY3 KTogW0dQVVBdIFVzZS1hZnRlci1tb3ZlIGluIFJlbW90ZUNETUluc3RhbmNlOjpjcmVhdGVTZXNz aW9uKCkKKyAgICAgICAgPGh0dHBzOi8vd2Via2l0Lm9yZy9iLzIwOTcxMj4KKyAgICAgICAgPHJk YXI6Ly9wcm9ibGVtLzYxMDE4Nzk1PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgICogV2ViUHJvY2Vzcy9HUFUvbWVkaWEvUmVtb3RlQ0RNSW5zdGFuY2Uu Y3BwOgorICAgICAgICAoV2ViS2l0OjpSZW1vdGVDRE1JbnN0YW5jZTo6Y3JlYXRlU2Vzc2lvbik6 IEZpeCB1c2UtYWZ0ZXItbW92ZQorICAgICAgICBieSB1c2luZyBSZW1vdGVDRE1JbnN0YW5jZVNl c3Npb246OmlkZW50aWZpZXIoKS4KKyAgICAgICAgKiBXZWJQcm9jZXNzL0dQVS9tZWRpYS9SZW1v dGVDRE1JbnN0YW5jZVNlc3Npb24uaDoKKyAgICAgICAgKFJlbW90ZUNETUluc3RhbmNlU2Vzc2lv bjo6aWRlbnRpZmllcik6IEFkZCBnZXR0ZXIuCisKIDIwMjAtMDMtMjggIERhdmlkIEtpbHplciAg PGRka2lsemVyQGFwcGxlLmNvbT4KIAogICAgICAgICBVc2UtYWZ0ZXItbW92ZSBpbiBOZXR3b3Jr UHJvY2Vzczo6YWRkU2VydmljZVdvcmtlclNlc3Npb24oKQpkaWZmIC0tZ2l0IGEvU291cmNlL1dl YktpdC9XZWJQcm9jZXNzL0dQVS9tZWRpYS9SZW1vdGVDRE1JbnN0YW5jZS5jcHAgYi9Tb3VyY2Uv V2ViS2l0L1dlYlByb2Nlc3MvR1BVL21lZGlhL1JlbW90ZUNETUluc3RhbmNlLmNwcAppbmRleCBl NTFhMGUwYjUyZDJkYjViMzY2ZThhYjY2OTY4YzU2MTQ2MmVkZDljLi5mMWFkN2E2ZjJjMjZiMDA3 ODc2NmE5MWFmOTAyMDJiMGFmY2U0N2Q2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L1dlYlBy b2Nlc3MvR1BVL21lZGlhL1JlbW90ZUNETUluc3RhbmNlLmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0 L1dlYlByb2Nlc3MvR1BVL21lZGlhL1JlbW90ZUNETUluc3RhbmNlLmNwcApAQCAtOTEsNyArOTEs NyBAQCBSZWZQdHI8V2ViQ29yZTo6Q0RNSW5zdGFuY2VTZXNzaW9uPiBSZW1vdGVDRE1JbnN0YW5j ZTo6Y3JlYXRlU2Vzc2lvbigpCiAgICAgaWYgKCFpZCkKICAgICAgICAgcmV0dXJuIG51bGxwdHI7 CiAgICAgYXV0byBzZXNzaW9uID0gUmVtb3RlQ0RNSW5zdGFuY2VTZXNzaW9uOjpjcmVhdGUobWFr ZVdlYWtQdHIobV9mYWN0b3J5LmdldCgpKSwgV1RGTW92ZShpZCkpOwotICAgIG1fZmFjdG9yeS0+ YWRkU2Vzc2lvbihpZCwgc2Vzc2lvbi5jb3B5UmVmKCkpOworICAgIG1fZmFjdG9yeS0+YWRkU2Vz c2lvbihzZXNzaW9uLT5pZGVudGlmaWVyKCksIHNlc3Npb24uY29weVJlZigpKTsKICAgICByZXR1 cm4gc2Vzc2lvbjsKIH0KIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9XZWJQcm9jZXNzL0dQ VS9tZWRpYS9SZW1vdGVDRE1JbnN0YW5jZVNlc3Npb24uaCBiL1NvdXJjZS9XZWJLaXQvV2ViUHJv Y2Vzcy9HUFUvbWVkaWEvUmVtb3RlQ0RNSW5zdGFuY2VTZXNzaW9uLmgKaW5kZXggOTYyZDA1Zjhi Zjk2M2VjYzUxMjYzMmEzYWFjYzA5MDQ0OWViNWM3Yi4uMjZjMjhiZTc2NzUwMTQxNmY2Nzg0Mzhj MzYwNTVhNTNhZjFmMDdhYiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9XZWJQcm9jZXNzL0dQ VS9tZWRpYS9SZW1vdGVDRE1JbnN0YW5jZVNlc3Npb24uaAorKysgYi9Tb3VyY2UvV2ViS2l0L1dl YlByb2Nlc3MvR1BVL21lZGlhL1JlbW90ZUNETUluc3RhbmNlU2Vzc2lvbi5oCkBAIC00Myw2ICs0 Myw4IEBAIHB1YmxpYzoKICAgICBzdGF0aWMgUmVmPFJlbW90ZUNETUluc3RhbmNlU2Vzc2lvbj4g Y3JlYXRlKFdlYWtQdHI8UmVtb3RlQ0RNRmFjdG9yeT4mJiwgUmVtb3RlQ0RNSW5zdGFuY2VTZXNz aW9uSWRlbnRpZmllciYmKTsKICAgICB2aXJ0dWFsIH5SZW1vdGVDRE1JbnN0YW5jZVNlc3Npb24o KSA9IGRlZmF1bHQ7CiAKKyAgICBSZW1vdGVDRE1JbnN0YW5jZVNlc3Npb25JZGVudGlmaWVyIGlk ZW50aWZpZXIoKSBjb25zdCB7IHJldHVybiBtX2lkZW50aWZpZXI7IH0KKwogcHJpdmF0ZToKICAg ICBmcmllbmQgY2xhc3MgUmVtb3RlQ0RNRmFjdG9yeTsKICAgICBSZW1vdGVDRE1JbnN0YW5jZVNl c3Npb24oV2Vha1B0cjxSZW1vdGVDRE1GYWN0b3J5PiYmLCBSZW1vdGVDRE1JbnN0YW5jZVNlc3Np b25JZGVudGlmaWVyJiYpOwo=