Attachment 346486 Details for Bug 188303 – Patch

[patch] Patch

bug-188303-20180803161601.patch (text/plain), 16.36 KB, created by Charlie Turner on 2018-08-03 08:16:02 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Charlie Turner

Created: 2018-08-03 08:16:02 PDT

Size: 16.36 KB

patch

obsolete

>Subversion Revision: 234497
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 419e3343144262850361ede2b9351df9299a6ffc..426bb35f1e9e9820c768572ba1383318356f3a1d 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,45 @@
>+2018-08-03  Charlie Turner  <cturner@igalia.com>
>+
>+        Return extracted key ids as an optional
>+        https://bugs.webkit.org/show_bug.cgi?id=188303
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        An empty list of extracted key ids was being considered a failure
>+        case before this patch. In the PSSH boxes from the CENC standard,
>+        it's not uncommon for the box to be version 0, meaning it has no
>+        embedded key ids, so the case when there's an empty list should
>+        not be treated as an error. Given this, the interface should be
>+        more general and allow for a sentinel value indicating a parsing
>+        error rather than an absence of key ids.
>+
>+        Covered by existing tests.
>+
>+        * Modules/encryptedmedia/InitDataRegistry.cpp:
>+        (WebCore::extractKeyIDsKeyids): Change return type to be wrapped
>+        in an optional, and make parsing errors return a nullopt rather
>+        than an empty vector.
>+        (WebCore::sanitizeKeyids): Use the new optional interface, return
>+        a null RefPtr in the case of a parsing error, this method may now
>+        return an empty vector.
>+        (WebCore::extractKeyIDsCenc): Not implemented, so return an error
>+        value rather than an empty vector.
>+        (WebCore::extractKeyIDsWebM): Ditto.
>+        (WebCore::InitDataRegistry::extractKeyIDs): Ditto.
>+        * Modules/encryptedmedia/InitDataRegistry.h: Update the interface
>+        to use an optional return type.
>+        * platform/graphics/avfoundation/CDMFairPlayStreaming.cpp:
>+        (WebCore::CDMPrivateFairPlayStreaming::extractKeyIDsSinf): Update
>+        to use the new interface.
>+        (WebCore::CDMPrivateFairPlayStreaming::extractKeyIDsSkd): Ditto.
>+        * platform/graphics/avfoundation/CDMFairPlayStreaming.h: Ditto.
>+        * platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:
>+        (WebCore::CDMInstanceFairPlayStreamingAVFObjC::keyIDs): Ditto.
>+        * testing/MockCDMFactory.cpp:
>+        (WebCore::MockCDMInstance::requestLicense): Only return an error
>+        if there really was a parsing error, rather than the case of there
>+        being zero key ids in the init data payload.
>+
> 2018-08-02  Charlie Turner  <cturner@igalia.com>
> 
>         Handle zero-sized ISOMP4 boxes appropriately
>diff --git a/Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp b/Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp
>index c29b038ad1d2925c438472f3adf08f4a54b0a3a1..1d67ed66bc32a4e06b3c67955b54bbdcc46795d8 100644
>--- a/Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp
>+++ b/Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp
>@@ -37,25 +37,25 @@
> 
> namespace WebCore {
> 
>-static Vector<Ref<SharedBuffer>> extractKeyIDsKeyids(const SharedBuffer& buffer)
>+static std::optional<Vector<Ref<SharedBuffer>>> extractKeyIDsKeyids(const SharedBuffer& buffer)
> {
>     // 1. Format
>     // https://w3c.github.io/encrypted-media/format-registry/initdata/keyids.html#format
>     if (buffer.size() > std::numeric_limits<unsigned>::max())
>-        return { };
>+        return std::nullopt;
>     String json { buffer.data(), static_cast<unsigned>(buffer.size()) };
> 
>     RefPtr<JSON::Value> value;
>     if (!JSON::Value::parseJSON(json, value))
>-        return { };
>+        return std::nullopt;
> 
>     RefPtr<JSON::Object> object;
>     if (!value->asObject(object))
>-        return { };
>+        return std::nullopt;
> 
>     RefPtr<JSON::Array> kidsArray;
>     if (!object->getArray("kids", kidsArray))
>-        return { };
>+        return std::nullopt;
> 
>     Vector<Ref<SharedBuffer>> keyIDs;
>     for (auto& value : *kidsArray) {
>@@ -78,13 +78,13 @@ static RefPtr<SharedBuffer> sanitizeKeyids(const SharedBuffer& buffer)
> {
>     // 1. Format
>     // https://w3c.github.io/encrypted-media/format-registry/initdata/keyids.html#format
>-    Vector<Ref<SharedBuffer>> keyIDBuffer = extractKeyIDsKeyids(buffer);
>-    if (keyIDBuffer.isEmpty())
>+    auto keyIDBuffer = extractKeyIDsKeyids(buffer);
>+    if (!keyIDBuffer)
>         return nullptr;
> 
>     auto object = JSON::Object::create();
>     auto kidsArray = JSON::Array::create();
>-    for (auto& buffer : keyIDBuffer)
>+    for (auto& buffer : keyIDBuffer.value())
>         kidsArray->pushString(WTF::base64URLEncode(buffer->data(), buffer->size()));
>     object->setArray("kids", WTFMove(kidsArray));
> 
>@@ -100,12 +100,12 @@ static RefPtr<SharedBuffer> sanitizeCenc(const SharedBuffer& buffer)
>     return buffer.copy();
> }
> 
>-static Vector<Ref<SharedBuffer>> extractKeyIDsCenc(const SharedBuffer&)
>+static std::optional<Vector<Ref<SharedBuffer>>> extractKeyIDsCenc(const SharedBuffer&)
> {
>     // 4. Common SystemID and PSSH Box Format
>     // https://w3c.github.io/encrypted-media/format-registry/initdata/cenc.html#common-system
>     notImplemented();
>-    return { };
>+    return std::nullopt;
> }
> 
> static RefPtr<SharedBuffer> sanitizeWebM(const SharedBuffer& buffer)
>@@ -116,12 +116,12 @@ static RefPtr<SharedBuffer> sanitizeWebM(const SharedBuffer& buffer)
>     return buffer.copy();
> }
> 
>-static Vector<Ref<SharedBuffer>> extractKeyIDsWebM(const SharedBuffer&)
>+static std::optional<Vector<Ref<SharedBuffer>>> extractKeyIDsWebM(const SharedBuffer&)
> {
>     // 1. Format
>     // https://w3c.github.io/encrypted-media/format-registry/initdata/webm.html#format
>     notImplemented();
>-    return { };
>+    return std::nullopt;
> }
> 
> InitDataRegistry& InitDataRegistry::shared()
>@@ -147,11 +147,11 @@ RefPtr<SharedBuffer> InitDataRegistry::sanitizeInitData(const AtomicString& init
>     return iter->value.sanitizeInitData(buffer);
> }
> 
>-Vector<Ref<SharedBuffer>> InitDataRegistry::extractKeyIDs(const AtomicString& initDataType, const SharedBuffer& buffer)
>+std::optional<Vector<Ref<SharedBuffer>>> InitDataRegistry::extractKeyIDs(const AtomicString& initDataType, const SharedBuffer& buffer)
> {
>     auto iter = m_types.find(initDataType);
>     if (iter == m_types.end() || !iter->value.sanitizeInitData)
>-        return { };
>+        return std::nullopt;
>     return iter->value.extractKeyIDs(buffer);
> }
> 
>diff --git a/Source/WebCore/Modules/encryptedmedia/InitDataRegistry.h b/Source/WebCore/Modules/encryptedmedia/InitDataRegistry.h
>index 39bfa0453546da577ba3d9f8f1a494891b868174..d1e3584dfe3509dfd2b4b9487068918d45b89223 100644
>--- a/Source/WebCore/Modules/encryptedmedia/InitDataRegistry.h
>+++ b/Source/WebCore/Modules/encryptedmedia/InitDataRegistry.h
>@@ -29,6 +29,7 @@
> 
> #include <wtf/Function.h>
> #include <wtf/HashMap.h>
>+#include <wtf/Optional.h>
> #include <wtf/Ref.h>
> #include <wtf/RefPtr.h>
> #include <wtf/Vector.h>
>@@ -45,11 +46,11 @@ public:
>     friend class NeverDestroyed<InitDataRegistry>;
> 
>     RefPtr<SharedBuffer> sanitizeInitData(const AtomicString& initDataType, const SharedBuffer&);
>-    WEBCORE_EXPORT Vector<Ref<SharedBuffer>> extractKeyIDs(const AtomicString& initDataType, const SharedBuffer&);
>+    WEBCORE_EXPORT std::optional<Vector<Ref<SharedBuffer>>> extractKeyIDs(const AtomicString& initDataType, const SharedBuffer&);
> 
>     struct InitDataTypeCallbacks {
>         using SanitizeInitDataCallback = Function<RefPtr<SharedBuffer>(const SharedBuffer&)>;
>-        using ExtractKeyIDsCallback = Function<Vector<Ref<SharedBuffer>>(const SharedBuffer&)>;
>+        using ExtractKeyIDsCallback = Function<std::optional<Vector<Ref<SharedBuffer>>>(const SharedBuffer&)>;
> 
>         SanitizeInitDataCallback sanitizeInitData;
>         ExtractKeyIDsCallback extractKeyIDs;
>diff --git a/Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.cpp b/Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.cpp
>index d2086848de9e0367b29a702d6451e0ee8f2674a8..8c64de1e51536cceb2b4f22e1efe989f6103a9d0 100644
>--- a/Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.cpp
>+++ b/Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.cpp
>@@ -158,9 +158,8 @@ static SchemeAndKeyResult extractSchemeAndKeyIdFromSinf(const SharedBuffer& buff
>     return result;
> }
> 
>-Vector<Ref<SharedBuffer>> CDMPrivateFairPlayStreaming::extractKeyIDsSinf(const SharedBuffer& buffer)
>+std::optional<Vector<Ref<SharedBuffer>>> CDMPrivateFairPlayStreaming::extractKeyIDsSinf(const SharedBuffer& buffer)
> {
>-
>     Vector<Ref<SharedBuffer>> keyIDs;
>     auto results = extractSchemeAndKeyIdFromSinf(buffer);
> 
>@@ -187,7 +186,7 @@ RefPtr<SharedBuffer> CDMPrivateFairPlayStreaming::sanitizeSkd(const SharedBuffer
>     return buffer.copy();
> }
> 
>-Vector<Ref<SharedBuffer>> CDMPrivateFairPlayStreaming::extractKeyIDsSkd(const SharedBuffer& buffer)
>+std::optional<Vector<Ref<SharedBuffer>>> CDMPrivateFairPlayStreaming::extractKeyIDsSkd(const SharedBuffer& buffer)
> {
>     // In the 'skd' scheme, the init data is the key ID.
>     Vector<Ref<SharedBuffer>> keyIDs;
>diff --git a/Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.h b/Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.h
>index 8ad3689a9a7423a339d08df4c83ae02b13d2bf5d..144c021a2f4cf7756776d5ed4e006e6814f1992d 100644
>--- a/Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.h
>+++ b/Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.h
>@@ -68,11 +68,11 @@ public:
>     std::optional<String> sanitizeSessionId(const String&) const override;
> 
>     static const AtomicString& sinfName();
>-    static Vector<Ref<SharedBuffer>> extractKeyIDsSinf(const SharedBuffer&);
>+    static std::optional<Vector<Ref<SharedBuffer>>> extractKeyIDsSinf(const SharedBuffer&);
>     static RefPtr<SharedBuffer> sanitizeSinf(const SharedBuffer&);
> 
>     static const AtomicString& skdName();
>-    static Vector<Ref<SharedBuffer>> extractKeyIDsSkd(const SharedBuffer&);
>+    static std::optional<Vector<Ref<SharedBuffer>>> extractKeyIDsSkd(const SharedBuffer&);
>     static RefPtr<SharedBuffer> sanitizeSkd(const SharedBuffer&);
> };
> 
>diff --git a/Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.h b/Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.h
>index 83b9d5023cc18a9e4293ff625af1533ca2fa010f..857dd9e21688877b9a24d17aa6b7ee55cca5d938 100644
>--- a/Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.h
>+++ b/Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.h
>@@ -29,6 +29,7 @@
> 
> #include "CDMInstance.h"
> #include <wtf/Function.h>
>+#include <wtf/Optional.h>
> #include <wtf/RetainPtr.h>
> #include <wtf/WeakPtr.h>
> #include <wtf/text/WTFString.h>
>@@ -84,7 +85,7 @@ public:
> private:
>     bool isLicenseTypeSupported(LicenseType) const;
> 
>-    Vector<Ref<SharedBuffer>> keyIDs();
>+    std::optional<Vector<Ref<SharedBuffer>>> keyIDs();
> 
>     RefPtr<SharedBuffer> m_serverCertificate;
>     bool m_persistentStateAllowed { true };
>diff --git a/Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm b/Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm
>index e105486cbf5f065fb36e26cf042234bf015a43ff..8b31d3e3200ba377ed915258070cb68aa7c5c499 100644
>--- a/Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm
>+++ b/Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm
>@@ -233,7 +233,7 @@ bool CDMInstanceFairPlayStreamingAVFObjC::isLicenseTypeSupported(LicenseType lic
>     }
> }
> 
>-Vector<Ref<SharedBuffer>> CDMInstanceFairPlayStreamingAVFObjC::keyIDs()
>+std::optional<Vector<Ref<SharedBuffer>>> CDMInstanceFairPlayStreamingAVFObjC::keyIDs()
> {
>     // FIXME(rdar://problem/35597141): use the future AVContentKeyRequest keyID property, rather than parsing it out of the init
>     // data, to get the keyID.
>@@ -308,8 +308,8 @@ void CDMInstanceFairPlayStreamingAVFObjC::updateLicense(const String&, LicenseTy
>         callback(false, std::nullopt, std::nullopt, std::nullopt, Failed);
>         return;
>     }
>-    Vector<Ref<SharedBuffer>> keyIDs = this->keyIDs();
>-    if (keyIDs.isEmpty()) {
>+    auto keyIDs = this->keyIDs();
>+    if (!keyIDs || (keyIDs && keyIDs.value().isEmpty())) {
>         callback(false, std::nullopt, std::nullopt, std::nullopt, Failed);
>         return;
>     }
>@@ -321,7 +321,7 @@ void CDMInstanceFairPlayStreamingAVFObjC::updateLicense(const String&, LicenseTy
>     if (!method.name) {
>         KeyStatusVector keyStatuses;
>         keyStatuses.reserveInitialCapacity(1);
>-        keyStatuses.uncheckedAppend(std::make_pair(WTFMove(keyIDs.first()), KeyStatus::Usable));
>+        keyStatuses.uncheckedAppend(std::make_pair(WTFMove(keyIDs.value().first()), KeyStatus::Usable));
>         callback(false, std::make_optional(WTFMove(keyStatuses)), std::nullopt, std::nullopt, Succeeded);
>         return;
>     }
>@@ -446,15 +446,15 @@ void CDMInstanceFairPlayStreamingAVFObjC::didProvideRequest(AVContentKeyRequest
>         return;
> 
>     RetainPtr<NSData> appIdentifier = m_serverCertificate ? m_serverCertificate->createNSData() : nullptr;
>-    Vector<Ref<SharedBuffer>> keyIDs = this->keyIDs();
>+    auto keyIDs = this->keyIDs();
> 
>-    if (keyIDs.isEmpty()) {
>+    if (!keyIDs || (keyIDs && keyIDs.value().isEmpty())) {
>         m_requestLicenseCallback(SharedBuffer::create(), m_sessionId, false, Failed);
>         m_requestLicenseCallback = nullptr;
>         return;
>     }
> 
>-    RetainPtr<NSData> contentIdentifier = keyIDs.first()->createNSData();
>+    RetainPtr<NSData> contentIdentifier = keyIDs.value().first()->createNSData();
>     [m_request makeStreamingContentKeyRequestDataForApp:appIdentifier.get() contentIdentifier:contentIdentifier.get() options:nil completionHandler:[this, weakThis = makeWeakPtr(*this)] (NSData *contentKeyRequestData, NSError *error) mutable {
>         callOnMainThread([this, weakThis = WTFMove(weakThis), error = retainPtr(error), contentKeyRequestData = retainPtr(contentKeyRequestData)] {
>             if (!weakThis || !m_requestLicenseCallback)
>@@ -493,10 +493,10 @@ void CDMInstanceFairPlayStreamingAVFObjC::requestDidSucceed(AVContentKeyRequest
>     if (!m_updateLicenseCallback)
>         return;
> 
>-    Vector<Ref<SharedBuffer>> keyIDs = this->keyIDs();
>+    auto keyIDs = this->keyIDs();
>     KeyStatusVector keyStatuses;
>     keyStatuses.reserveInitialCapacity(1);
>-    keyStatuses.uncheckedAppend(std::make_pair(WTFMove(keyIDs.first()), KeyStatus::Usable));
>+    keyStatuses.uncheckedAppend(std::make_pair(WTFMove(keyIDs.value().first()), KeyStatus::Usable));
>     m_updateLicenseCallback(false, std::make_optional(WTFMove(keyStatuses)), std::nullopt, std::nullopt, Succeeded);
> }
> 
>@@ -521,7 +521,9 @@ void CDMInstanceFairPlayStreamingAVFObjC::sessionIdentifierChanged(NSData *sessi
> 
> void CDMInstanceFairPlayStreamingAVFObjC::outputObscuredDueToInsufficientExternalProtectionChanged(bool obscured)
> {
>-    if (!m_client || !m_request)
>+    auto maybeKeyIDs = keyIDs();
>+
>+    if (!m_client || !m_request || !maybeKeyIDs)
>         return;
> 
>     CDMKeyStatus status;
>@@ -546,7 +548,7 @@ void CDMInstanceFairPlayStreamingAVFObjC::outputObscuredDueToInsufficientExterna
>         }
>     }
> 
>-    auto keyStatuses = keyIDs().map([status] (const Ref<SharedBuffer>& keyID) -> KeyStatusVector::ValueType {
>+    auto keyStatuses = maybeKeyIDs.value().map([status] (const Ref<SharedBuffer>& keyID) -> KeyStatusVector::ValueType {
>         return { keyID.copyRef(), status };
>     });
>     m_client->updateKeyStatuses(WTFMove(keyStatuses));
>diff --git a/Source/WebCore/testing/MockCDMFactory.cpp b/Source/WebCore/testing/MockCDMFactory.cpp
>index a7433c6cf8225c758dbdf22a12363924008c3b22..51d77756bac55532d3fa752cee7c74de45cf3d25 100644
>--- a/Source/WebCore/testing/MockCDMFactory.cpp
>+++ b/Source/WebCore/testing/MockCDMFactory.cpp
>@@ -283,13 +283,13 @@ void MockCDMInstance::requestLicense(LicenseType licenseType, const AtomicString
>     }
> 
>     auto keyIDs = InitDataRegistry::shared().extractKeyIDs(initDataType, initData);
>-    if (keyIDs.isEmpty()) {
>+    if (!keyIDs || (keyIDs && keyIDs.value().isEmpty())) {
>         callback(SharedBuffer::create(), emptyString(), false, SuccessValue::Failed);
>         return;
>     }
> 
>     String sessionID = createCanonicalUUIDString();
>-    factory->addKeysToSessionWithID(sessionID, WTFMove(keyIDs));
>+    factory->addKeysToSessionWithID(sessionID, WTFMove(keyIDs.value()));
> 
>     CString license { "license" };
>     callback(SharedBuffer::create(license.data(), license.length()), sessionID, false, SuccessValue::Succeeded);

Actions: View | Formatted Diff | Diff

Attachments on bug 188303: 346473 | 346475 | 346482 | 346486 | 346488 | 346490 | 346624