Bug 177803 – Fix MSVC build with ENCRYPTED_MEDIA enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Yoshiaki Jitsukawa]

Both MSVC 2015 and 2017 fail to compile MediaKeySession.cpp and CDMClearKey.cpp.

[Yoshiaki Jitsukawa]

Created attachment 322502 [details]
Patch

[Yoshiaki Jitsukawa]

MSVC fails to compile lambda capture lists like this:
 [this, weakThis = m_weakPtrFactory.createWeakPtr(*this), promise = WTFMove(promise)]
so with the patch weakThis is created outside of the capture list.

[Alex Christensen]

That ought to work.  It's valid C++14.  What is the error when compiling these files without this change?

[Chris Dumez]

Yes, this should build fine. Those changes do not look good. Are you building without c++14 support enabled?

[Yoshiaki Jitsukawa]

Created attachment 322632 [details]
compiler error log for MediaKeySession.cpp

For example I'm getting the attached errors when compiling MediaKeySession.cpp even with /std:c++14 option.

[Alex Christensen]

Comment on attachment 322502 [details]
Patch

I think we need to find a better solution than this.

[Yoshiaki Jitsukawa]

(In reply to Alex Christensen from comment #6)
> Comment on attachment 322502 [details]
> Patch
> 
> I think we need to find a better solution than this.

I agree to that. Thank you!

[Zan Dobersek]

Try adding a simple private `createWeakPtr()` method on the MediaKeySession class, and call that in the lambda's capture list:

> WeakPtr<MediaKeySession> createWeakPtr() { return m_weakPtrFactory.createWeakPtr(*this); }

The build error seems to indicate a compiler bug in how the `*this` object is deduced in the capture list. I also wonder whether capturing `weakThis = m_weakPtrFactory.createWeakPtr(*this)` before the plain `this` pointer would have any effect.

[Yoshiaki Jitsukawa]

(In reply to Zan Dobersek from comment #8)
> Try adding a simple private `createWeakPtr()` method on the MediaKeySession
> class, and call that in the lambda's capture list:
> 
> > WeakPtr<MediaKeySession> createWeakPtr() { return m_weakPtrFactory.createWeakPtr(*this); }
> 
> The build error seems to indicate a compiler bug in how the `*this` object
> is deduced in the capture list. I also wonder whether capturing `weakThis =
> m_weakPtrFactory.createWeakPtr(*this)` before the plain `this` pointer would
> have any effect.

Thanks, let me try those.

[Yoshiaki Jitsukawa]

Created attachment 323238 [details]
Patch

[Yoshiaki Jitsukawa]

(In reply to Zan Dobersek from comment #8)
> Try adding a simple private `createWeakPtr()` method on the MediaKeySession
> class, and call that in the lambda's capture list:
> 
> > WeakPtr<MediaKeySession> createWeakPtr() { return m_weakPtrFactory.createWeakPtr(*this); }
> 
> The build error seems to indicate a compiler bug in how the `*this` object
> is deduced in the capture list. I also wonder whether capturing `weakThis =
> m_weakPtrFactory.createWeakPtr(*this)` before the plain `this` pointer would
> have any effect.

Both of them didn't work and it seems that there's something wrong with deduction
in nested lambda so I attached another patch to create a weak ptr at the outermost
capture list.

[Alex Christensen]

Comment on attachment 323238 [details]
Patch

I think this approach looks ok.

[Yoshiaki Jitsukawa]

(In reply to Alex Christensen from comment #12)
> Comment on attachment 323238 [details]
> Patch
> 
> I think this approach looks ok.

Thank you!

[WebKit Commit Bot]

Comment on attachment 323238 [details]
Patch

Clearing flags on attachment: 323238

Committed r223148: <http://trac.webkit.org/changeset/223148>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34920322>

[Matt Lewis]

This caused internal failures. For more information, refer to the reviewer for more details.

[Matt Lewis]

Reverted r223148 for reason:

This caused build failures.

Committed r223153: <http://trac.webkit.org/changeset/223153>

[Yoshiaki Jitsukawa]

(In reply to Matt Lewis from comment #17)
> This caused internal failures. For more information, refer to the reviewer
> for more details.

So would you give me some information to help fix the failures?

[Don Olmstead]

Created attachment 419609 [details]
Patch

[Alex Christensen]

Comment on attachment 419609 [details]
Patch

The original patch introduced some unused lambda capture 'this' instances:

error:
.../OpenSource/Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:357:10: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
.../OpenSource/Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:461:10: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
.../OpenSource/Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:505:10: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]

What a great reason to delay this patch for 3.5 years.  This version doesn't touch that file, so it should be fine.  If you want to also change CDMClearKey.cpp, just remove 'this' if you can.

[Don Olmstead]

Created attachment 419633 [details]
Patch

[EWS]

Committed r272557: <https://commits.webkit.org/r272557>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419633 [details].

[Darin Adler]

Comment on attachment 419633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419633&action=review

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:326
> +        m_instanceSession->loadSession(m_sessionType, *sanitizedSessionId, origin, [this, weakThis = makeWeakPtr(*weakThis), promise = WTFMove(promise), sanitizedSessionId = *sanitizedSessionId, identifier = WTFMove(identifier)] (Optional<CDMInstanceSession::KeyStatusVector>&& knownKeys, Optional<double>&& expiration, Optional<CDMInstanceSession::Message>&& message, CDMInstanceSession::SuccessValue succeeded, CDMInstanceSession::SessionLoadFailure failure) mutable {

Is this:

    weakThis = makeWeakPtr(*weakThis)

Different from just writing this:

    weakThis

?

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:438
> +        m_instanceSession->updateLicense(m_sessionId, m_sessionType, sanitizedResponse.releaseNonNull(), [this, weakThis = makeWeakPtr(*weakThis), promise = WTFMove(promise), identifier = WTFMove(identifier)] (bool sessionWasClosed, Optional<CDMInstanceSession::KeyStatusVector>&& changedKeys, Optional<double>&& changedExpiration, Optional<CDMInstanceSession::Message>&& message, CDMInstanceSession::SuccessValue succeeded) mutable {

Ditto.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:559
> +        m_instanceSession->closeSession(m_sessionId, [this, weakThis = makeWeakPtr(*weakThis), promise = WTFMove(promise), identifier = WTFMove(identifier)] () mutable {

Ditto.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:603
> +        m_instanceSession->removeSessionData(m_sessionId, m_sessionType, [this, weakThis = makeWeakPtr(*weakThis), promise = WTFMove(promise), identifier = WTFMove(identifier)] (CDMInstanceSession::KeyStatusVector&& keys, Optional<Ref<SharedBuffer>>&& message, CDMInstanceSession::SuccessValue succeeded) mutable {

Ditto.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:490
> +                [weakThis = makeWeakPtr(*weakThis), callback = WTFMove(callback), sessionWasClosed, changedKeys = WTFMove(changedKeys), succeeded] () mutable {

Ditto.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:573
> +                [weakThis = makeWeakPtr(*weakThis), callback = WTFMove(callback), keyStatusVector = WTFMove(keyStatusVector), message = WTFMove(message), success]() mutable {

Ditto.