Bug 209712 - REGRESSION (r257867): [GPUP] Use-after-move in RemoteCDMInstance::createSession()
Summary: REGRESSION (r257867): [GPUP] Use-after-move in RemoteCDMInstance::createSessi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks: 210006
  Show dependency treegraph
 
Reported: 2020-03-28 19:44 PDT by David Kilzer (:ddkilzer)
Modified: 2020-04-04 10:01 PDT (History)
8 users (show)

See Also:


Attachments
Patch v1 (2.41 KB, patch)
2020-03-28 19:51 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 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>
Comment 1 Radar WebKit Bug Importer 2020-03-28 19:44:25 PDT
<rdar://problem/61018795>
Comment 2 David Kilzer (:ddkilzer) 2020-03-28 19:51:20 PDT
Created attachment 394852 [details]
Patch v1
Comment 3 Darin Adler 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.
Comment 4 David Kilzer (:ddkilzer) 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?
Comment 5 Darin Adler 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.
Comment 6 EWS 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].
Comment 7 David Kilzer (:ddkilzer) 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()