REGRESSION (r257867): [GPUP] Use-after-move in RemoteCDMInstance::createSession()

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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>

[Radar WebKit Bug Importer]

<rdar://problem/61018795>

[David Kilzer (:ddkilzer)]

Created attachment 394852 [details]
Patch v1

[Darin Adler]

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 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]

Yes, keep this patch as small as possible. You can do other changes in another patch that is not fixing an obvious problem.

[EWS]

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)]

(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()