RESOLVED FIXED 209712
REGRESSION (r257867): [GPUP] Use-after-move in RemoteCDMInstance::createSession()
https://bugs.webkit.org/show_bug.cgi?id=209712
Summary REGRESSION (r257867): [GPUP] Use-after-move in RemoteCDMInstance::createSessi...
David Kilzer (:ddkilzer)
Reported 2020-03-28 19:44:03 PDT
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>
Attachments
Patch v1 (2.41 KB, patch)
2020-03-28 19:51 PDT, David Kilzer (:ddkilzer)
no flags
Radar WebKit Bug Importer
Comment 1 2020-03-28 19:44:25 PDT
David Kilzer (:ddkilzer)
Comment 2 2020-03-28 19:51:20 PDT
Created attachment 394852 [details] Patch v1
Darin Adler
Comment 3 2020-03-28 19:52:35 PDT
Comment on attachment 394852 [details] 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.
David Kilzer (:ddkilzer)
Comment 4 2020-03-28 19:54:55 PDT
Comment on attachment 394852 [details] 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?
Darin Adler
Comment 5 2020-03-28 19:56:32 PDT
Yes, keep this patch as small as possible. You can do other changes in another patch that is not fixing an obvious problem.
EWS
Comment 6 2020-03-29 01:52:44 PDT
Committed r259176: <https://trac.webkit.org/changeset/259176> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394852 [details].
David Kilzer (:ddkilzer)
Comment 7 2020-04-04 10:01:05 PDT
(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()
Note You need to log in before you can comment on or make changes to this bug.