Both MSVC 2015 and 2017 fail to compile MediaKeySession.cpp and CDMClearKey.cpp.
Created attachment 322502 [details] Patch
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.
That ought to work. It's valid C++14. What is the error when compiling these files without this change?
Yes, this should build fine. Those changes do not look good. Are you building without c++14 support enabled?
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.
Comment on attachment 322502 [details] Patch I think we need to find a better solution than this.
(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!
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.
(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.
Created attachment 323238 [details] Patch
(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.
Comment on attachment 323238 [details] Patch I think this approach looks ok.
(In reply to Alex Christensen from comment #12) > Comment on attachment 323238 [details] > Patch > > I think this approach looks ok. Thank you!
Comment on attachment 323238 [details] Patch Clearing flags on attachment: 323238 Committed r223148: <http://trac.webkit.org/changeset/223148>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34920322>
This caused internal failures. For more information, refer to the reviewer for more details.
Reverted r223148 for reason: This caused build failures. Committed r223153: <http://trac.webkit.org/changeset/223153>
(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?
Created attachment 419609 [details] Patch
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.
Created attachment 419633 [details] Patch
Committed r272557: <https://commits.webkit.org/r272557> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419633 [details].
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.