1420836 - Implement cdm::Host_9::RequestStorageId

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Description

[James Cheng[:JamesCheng]]

We should implement this function,

CDM expect us calling ContentDecryptionModule_9::OnStorageId [2] to let it know or GMP process will got stuck.

[1]
https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/dom/media/gmp/widevine-adapter/content_decryption_module.h#1194

[2]
https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/dom/media/gmp/widevine-adapter/content_decryption_module.h#895

Comment 1

[James Cheng[:JamesCheng]]

Attachment: [WIP] Implement cdm::Host_9::RequestStorageId

The patch is a hack way for making CDM 1.4.9.1056 run normally on Firefox.
I should ask for more implementation detail about the buffer passing to the OnStorageId.



[WIP] Implement cdm::Host_9::RequestStorageId.

MozReview-Commit-ID: 7qgNgbUDSfr

Comment 2

[James Cheng[:JamesCheng]]

Chromium compute their Id by

https://cs.chromium.org/chromium/src/chrome/browser/media/cdm_storage_id.cc?l=69&rcl=f54676dcee771be7e1271c60aa3440e9574b3c7c

The hash code is generated by ``machine_id + origin + storage_id_key + profile_salt``

I cannot easily find the corresponding factors in gecko.

Comment 3

[James Cheng[:JamesCheng]]

Hi Chris,

Regarding to comment 2,

Ideally, we should find some identifier to generate the hash value, then provide it into CDM.

Chromium uses ``machine_id + origin + storage_id_key + profile_salt`` that I'm not sure if gecko has the corresponding method to retreive those values.

Two questions need to consult you,

1. Do you know is there any similar facility/utility method in Gecko that we can leverage directly?
If not, do you know who might be the expert of this domain that I can ask for help?

2. What do you think about the implementation of this API? Am I in the right direction or we don't need to take reference of Chromium's impl.

I'll upload a patch as a prototype in Part1 for review, and do the complicated things in Part2.

Thank you!

Comment 4

[James Cheng[:JamesCheng]]

Attachment: Bug 1420836 - Part1 - Restore sha256.c from Bug 1332530 back.

Review commit: https://reviewboard.mozilla.org/r/204522/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204522/

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to James Cheng[:JamesCheng] from comment #3)
> Hi Chris,
> 
> Regarding to comment 2,
> 
> Ideally, we should find some identifier to generate the hash value, then
> provide it into CDM.
> 
> Chromium uses ``machine_id + origin + storage_id_key + profile_salt`` that
> I'm not sure if gecko has the corresponding method to retreive those values.
> 
> Two questions need to consult you,
> 
> 1. Do you know is there any similar facility/utility method in Gecko that we
> can leverage directly?

Chromium uses rlz to calculate the machine ID. We imported that code for use with Adobe EME, but removed it when we removed Adobe EME, in Bug 1332530. So you could start by restoring the code in Bug 1332530 which adds librlz, and then ensuring it's up to date with upstream.

You do *not* need to restore the code that ensured the device binding is linked into plugin-container; it can live in libxul now.

You may also need the Mac code added in bug 1214018.

The storage_id_key parameter ( https://cs.chromium.org/chromium/src/chrome/browser/media/cdm_storage_id.cc?l=101&rcl=e0e3bb53c23b95fff838908a8296dc84ce722b99 ) is tricker. That looks like it's not checked into Chromium's public repo ( https://cs.chromium.org/chromium/src/chrome/browser/media/cdm_storage_id_key.cc?l=14&rcl=e0e3bb53c23b95fff838908a8296dc84ce722b99 ) so we'll need to ask them about that.


> If not, do you know who might be the expert of this domain that I can ask
> for help?
> 
> 2. What do you think about the implementation of this API? Am I in the right
> direction or we don't need to take reference of Chromium's impl.

We should proceed to implement it.

> 
> I'll upload a patch as a prototype in Part1 for review, and do the
> complicated things in Part2.
> 
> Thank you!

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8933581 [details]
Bug 1420836 - Part1 - Restore sha256.c from Bug 1332530 back.

https://reviewboard.mozilla.org/r/204522/#review210418

r+ with leak fixed.

::: dom/media/gmp/ChromiumCDMParent.cpp:305
(Diff revision 1)
> +  GMP_LOG("ChromiumCDMParent::RecvRequestStorageId(this=%p, version=%u",
> +          this,
> +          aVersion);
> +
> +  nsAutoCString storageId;
> +  nsICryptoHash* hasher = nullptr;

Doesn't 'hasher' need to be an nsCOMPtr, otherwise you'll leak the instance?

::: dom/media/gmp/ChromiumCDMParent.cpp:307
(Diff revision 1)
> +          aVersion);
> +
> +  nsAutoCString storageId;
> +  nsICryptoHash* hasher = nullptr;
> +  nsresult rv;
> +  if (!hasher) {

You don't need the !hasher check here; you know it's null since you assign it to null on line 305 above.

Comment 7

[James Cheng[:JamesCheng]]

(In reply to Chris Pearce (:cpearce) from comment #5)
> (In reply to James Cheng[:JamesCheng] from comment #3)
> > Hi Chris,
> > 
> > Regarding to comment 2,
> > 
> > Ideally, we should find some identifier to generate the hash value, then
> > provide it into CDM.
> > 
> > Chromium uses ``machine_id + origin + storage_id_key + profile_salt`` that
> > I'm not sure if gecko has the corresponding method to retreive those values.
> > 
> > Two questions need to consult you,
> > 
> > 1. Do you know is there any similar facility/utility method in Gecko that we
> > can leverage directly?
> 
> Chromium uses rlz to calculate the machine ID. We imported that code for use
> with Adobe EME, but removed it when we removed Adobe EME, in Bug 1332530. So
> you could start by restoring the code in Bug 1332530 which adds librlz, and
> then ensuring it's up to date with upstream.
> 
> You do *not* need to restore the code that ensured the device binding is
> linked into plugin-container; it can live in libxul now.
> 
> You may also need the Mac code added in bug 1214018.

Thanks for your guidance. 
> 
> The storage_id_key parameter (
> https://cs.chromium.org/chromium/src/chrome/browser/media/cdm_storage_id.
> cc?l=101&rcl=e0e3bb53c23b95fff838908a8296dc84ce722b99 ) is tricker. That
> looks like it's not checked into Chromium's public repo (
> https://cs.chromium.org/chromium/src/chrome/browser/media/cdm_storage_id_key.
> cc?l=14&rcl=e0e3bb53c23b95fff838908a8296dc84ce722b99 ) so we'll need to ask
> them about that.
> 
Thanks,
I noticed that, and I will ask them about this parameter.

> 
> > If not, do you know who might be the expert of this domain that I can ask
> > for help?
> > 
> > 2. What do you think about the implementation of this API? Am I in the right
> > direction or we don't need to take reference of Chromium's impl.
> 
> We should proceed to implement it.
> 
> > 
> > I'll upload a patch as a prototype in Part1 for review, and do the
> > complicated things in Part2.
> > 
> > Thank you!

Comment 8

[James Cheng[:JamesCheng]]

Comment on attachment 8933581 [details]
Bug 1420836 - Part1 - Restore sha256.c from Bug 1332530 back.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204522/diff/1-2/

Comment 9

[James Cheng[:JamesCheng]]

Attachment: Bug 1420836 - Part2 - Add a class to compute the storage id.

Every version will have its own prerequisite factor to compute the storage id.
If we need to change the storage id computation algorithm, we should specialize the template class and increase the current version.
Now, we only have version 1 for computing storage id.

Review commit: https://reviewboard.mozilla.org/r/207884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207884/

Comment 10

[James Cheng[:JamesCheng]]

Attachment: Bug 1420836 - Part3 - Try to compute the storage id in GMP process and implement RequestStorageId for CDM.

GMPParent has computed the origin salt so I try to pass it to GMPChild.
Then, try to compute the storage id before sandboxing.
Pass the storage id to ChromiumCDMChild by its Init function.

Review commit: https://reviewboard.mozilla.org/r/207886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207886/

Comment 11

[James Cheng[:JamesCheng]]

Comment on attachment 8933581 [details]
Bug 1420836 - Part1 - Restore sha256.c from Bug 1332530 back.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204522/diff/2-3/

Comment 12

[James Cheng[:JamesCheng]]

Comment on attachment 8932750 [details] [diff] [review]
[WIP] Implement cdm::Host_9::RequestStorageId

># HG changeset patch
># User James Cheng <[e-mail]>
>
>Bug 1420836 - [WIP] Implement cdm::Host_9::RequestStorageId.
>
>MozReview-Commit-ID: 7qgNgbUDSfr
>
>diff --git a/dom/media/gmp/ChromiumCDMChild.cpp b/dom/media/gmp/ChromiumCDMChild.cpp
>index 519df1cdf373..b74b73712816 100644
>--- a/dom/media/gmp/ChromiumCDMChild.cpp
>+++ b/dom/media/gmp/ChromiumCDMChild.cpp
>@@ -471,16 +471,30 @@ ChromiumCDMChild::CreateFileIO(cdm::FileIOClient * aClient)
>   MOZ_ASSERT(IsOnMessageLoopThread());
>   GMP_LOG("ChromiumCDMChild::CreateFileIO()");
>   if (!mPersistentStateAllowed) {
>     return nullptr;
>   }
>   return new WidevineFileIO(aClient);
> }
> 
>+void
>+ChromiumCDMChild::RequestStorageId(uint32_t aVersion)
>+{
>+  MOZ_ASSERT(IsOnMessageLoopThread());
>+  GMP_LOG("ChromiumCDMChild::RequestStorageId() aVersion = %u", aVersion);
>+  if (aVersion >= 0x80000000) {
>+    mCDM->OnStorageId(aVersion, nullptr, 0);
>+    return;
>+  }
>+
>+  //TODO: Need to provide a menaingful buffer instead of a dummy one.
>+  mCDM->OnStorageId(aVersion, new uint8_t[1024*1024], 1024 * 1024);
>+}
>+
> ChromiumCDMChild::~ChromiumCDMChild()
> {
>   GMP_LOG("ChromiumCDMChild:: dtor this=%p", this);
> }
> 
> bool
> ChromiumCDMChild::IsOnMessageLoopThread()
> {
>diff --git a/dom/media/gmp/ChromiumCDMChild.h b/dom/media/gmp/ChromiumCDMChild.h
>index 4a0b08c5709a..4d47017c97e5 100644
>--- a/dom/media/gmp/ChromiumCDMChild.h
>+++ b/dom/media/gmp/ChromiumCDMChild.h
>@@ -66,17 +66,17 @@ public:
>                              uint32_t aServiceIdSize,
>                              const char* aChallenge,
>                              uint32_t aChallengeSize) override {}
>   void EnableOutputProtection(uint32_t aDesiredProtectionMask) override {}
>   void QueryOutputProtectionStatus() override {}
>   void OnDeferredInitializationDone(cdm::StreamType aStreamType,
>                                     cdm::Status aDecoderStatus) override {}
>   // cdm::Host_9 interface
>-  void RequestStorageId(uint32_t aVersion) override {}
>+  void RequestStorageId(uint32_t aVersion) override;
>   cdm::FileIO* CreateFileIO(cdm::FileIOClient* aClient) override;
> 
>   // cdm::Host_8 implementation
>   void OnSessionMessage(const char* aSessionId,
>                         uint32_t aSessionIdSize,
>                         cdm::MessageType aMessageType,
>                         const char* aMessage,
>                         uint32_t aMessageSize,
>

Comment 13

[James Cheng[:JamesCheng]]

Attachment: Bug 1420836 - Part1 - Restore sha256.c from Bug 1332530 back.

I'd like to compute the sha256 hash in GMP process.
The nsICryptoHash cannot be used in GMP process.
So I restore it from Bug 1332530.

Review commit: https://reviewboard.mozilla.org/r/207890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207890/

Comment 14

[James Cheng[:JamesCheng]]

Attachment: Bug 1420836 - Part2 - Add a class to compute the storage id.

Every version will have its own prerequisite factor to compute the storage id.
If we need to change the storage id computation algorithm, we should specialize the template class and increase the current version.
Now, we only have version 1 for computing storage id.

Review commit: https://reviewboard.mozilla.org/r/207892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207892/

Comment 15

[James Cheng[:JamesCheng]]

Attachment: Bug 1420836 - Part3 - Try to compute the storage id in GMP process and implement RequestStorageId for CDM.

GMPParent has computed the origin salt so I try to pass it to GMPChild.
Then, try to compute the storage id before sandboxing.
Pass the storage id to ChromiumCDMChild by its Init function.

Review commit: https://reviewboard.mozilla.org/r/207894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207894/

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8937189 [details]
Bug 1420836 - Part1 - Restore sha256.c from Bug 1332530 back.

https://reviewboard.mozilla.org/r/207890/#review214316

You should be able to compute the storage Id in the parent process before sending it to the GMP process. So then you can just use the SHA256 code in Gecko to calculate the storage Id.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8937190 [details]
Bug 1420836 - Part2 - Add a class to compute the storage id.

https://reviewboard.mozilla.org/r/207892/#review214296

I think that using a template class for this is overkill, and doesn't make the code easier to understand. However if you want to continue with the template class approach, you should at least move the template class into the .cpp file, and then only expose a single public function which takes as a parameter the origin salt and returns the storage id for version 1. You don't need the consumer of the storage id to know all the details of what objects are required to calculate the storage id.

::: dom/media/gmp/CDMStorageIdProvider.h:17
(Diff revision 1)
> +
> +#ifdef SUPPORT_STORAGE_ID
> +#include "rlz/lib/string_utils.h"
> +#include "rlz/sha256.h"
> +#endif
> +

See my comment in the next commit in your series, but it seems to me this interface is way more complicated that it needs to be.

It seems since you only use version 1, you can just publically expose a function that takes as parameter the origin salt, and returns the storage id for the latest version. The fact that you use a complicated template class to separate the calculation of the storage id for different algorithms doesn't need to be exposed to the user of your API. The template class stuff can be in the .cpp file, hidden behind a single public function which just returns the storage id.

::: dom/media/gmp/CDMStorageIdProvider.h:73
(Diff revision 1)
> +
> +  CDMStorageIdProvider(const CDMStorageIdProvider&) = delete;
> +  void operator=(const CDMStorageIdProvider&) = delete;
> +};
> +
> +//Current version.

Nit: space after // before comment.

::: dom/media/gmp/CDMStorageIdProvider.h:88
(Diff revision 1)
> +
> +  struct PrerequisiteFactors : PrerequisiteFactorsBase
> +  {
> +    PrerequisiteFactors(std::string aMachineId, std::string aOriginSalt)
> +      : mMachineId(aMachineId),
> +        mOriginSalt(aOriginSalt)

Layout of args should be with leading ',' i.e.:

  : mMachineId(aMachineId)
  , mOriginSalt(aOriginSalt)

::: dom/media/gmp/CDMStorageIdProvider.h:123
(Diff revision 1)
> +    MOZ_ASSERT_UNREACHABLE("This platform does not support computing storage id.");
> +    return false;
> +#endif
> +  }
> +
> +  int GetVersion() override { return 1; }

Optional nit: you write a literal 1 here and above in the template argument. Is it possible to have a constant used in both places?

::: dom/media/gmp/CDMStorageIdProvider.h:135
(Diff revision 1)
> +UniquePtr<CDMStorageIdProviderBase::PrerequisiteFactorsBase>
> +MakePrerequisiteFactors(int aVersion, Args&&... aArgs)
> +{
> +  switch (aVersion) {
> +      case CDMStorageIdProviderBase::kCDMRequestLatestVersion:
> +        return MakeUnique<CDMStorageIdProvider<CDMStorageIdProviderBase::kCurrentVersion>::PrerequisiteFactors>(Forward<Args>(aArgs)...);

The behaviour of LatestVersion and CurrentVersion is the same. I don't understand what value having this switch statement adds.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8937191 [details]
Bug 1420836 - Part3 - Try to compute the storage id in GMP process and implement RequestStorageId for CDM.

https://reviewboard.mozilla.org/r/207894/#review214310

This can be made a lot simpler.

::: commit-message-d077e:1
(Diff revision 1)
> +Bug 1420836 - Part3 - Try to compute the storage id in GMP process and implement RequestStorageId for CDM.

Either do, or do not, there is no try.

::: dom/media/gmp/ChromiumCDMChild.cpp:496
(Diff revision 1)
> +  }
> +  if (aVersion > CDMStorageIdProviderBase::kCurrentVersion) {
> +    mCDM->OnStorageId(aVersion, nullptr, 0);
> +    return;
> +  }
> +  // mCurrentStorageId will be empty if we compute storage failed.

What should the behaviour be if aVersion is less than kCurrentVersion?

::: dom/media/gmp/GMPChild.cpp:278
(Diff revision 1)
> +    LOGD("%s get machineId failed.", __FUNCTION__);
> +    return IPC_OK();
> +  }
> +  std::string originSalt(aOriginSalt.BeginReading(), aOriginSalt.Length());
> +  auto provider = CDMStorageIdProviderBase::CreateProvider(CDMStorageIdProviderBase::kCDMRequestLatestVersion);
> +  auto factors = MakePrerequisiteFactors(CDMStorageIdProviderBase::kCDMRequestLatestVersion,

I think this code is more complicated that it needs to be.

You only ever compute the storage id for the lastest version currently.

You're calling a number of functions here in order to get the storage Id, but really the only variable input to the procedure is the origin salt.

So I think it makes sense to wrap all of the code related to generating the storage id into a single function that takes just the origin salt as a parameter, and does all the intermediate work. Then it's simpler for the caller.

I also don't see why the storage Id needs to be calculated in the GMP process. It could be calculated in the parent process I think, and then sent pre-calculated to the GMP process. Then you'd not need to re-import the old SHA256 code, you could use the existing code in Gecko for calculating SHA256.

Comment 19

[James Cheng[:JamesCheng]]

Attachment: Bug 1420836 - Part1 - Provide a utility function to compute the storage id.

The CDM on Mac and Windows will ask the host for storage id.
We compute the storage id by hashing machine id, origin salt and browser id.

Review commit: https://reviewboard.mozilla.org/r/209834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209834/

Comment 20

[James Cheng[:JamesCheng]]

Attachment: Bug 1420836 - Part2 - Pass the storage id from content process to GMP process then provide it to CDM.

Review commit: https://reviewboard.mozilla.org/r/209836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209836/

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8939503 [details]
Bug 1420836 - Part2 - Pass the storage id from content process to GMP process then provide it to CDM.

https://reviewboard.mozilla.org/r/209836/#review218668

r+, but please can you check whether mStorageId.get() will be nullptr if we failed to generate the storage ID.

::: dom/media/gmp/ChromiumCDMChild.cpp:495
(Diff revision 1)
> +  }
> +  if (aVersion > CDMStorageIdProvider::kCurrentVersion) {
> +    mCDM->OnStorageId(aVersion, nullptr, 0);
> +    return;
> +  }
> +

If we fail to generate the storage ID in the parent, will mStorageId.get() be nullptr here? Maybe we need to also explicitly check whether mStorageId.IsEmpty() here, and pass nullptr if so?

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8939502 [details]
Bug 1420836 - Part1 - Provide a utility function to compute the storage id.

https://reviewboard.mozilla.org/r/209834/#review218670

That's a lot better, thanks!

Comment 23

[James Cheng[:JamesCheng]]

Comment on attachment 8939502 [details]
Bug 1420836 - Part1 - Provide a utility function to compute the storage id.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209834/diff/1-2/

Comment 24

[James Cheng[:JamesCheng]]

Comment on attachment 8939503 [details]
Bug 1420836 - Part2 - Pass the storage id from content process to GMP process then provide it to CDM.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209836/diff/1-2/

Comment 25

[James Cheng[:JamesCheng]]

Comment on attachment 8939503 [details]
Bug 1420836 - Part2 - Pass the storage id from content process to GMP process then provide it to CDM.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209836/diff/2-3/

Comment 26

[James Cheng[:JamesCheng]]

Comment on attachment 8939503 [details]
Bug 1420836 - Part2 - Pass the storage id from content process to GMP process then provide it to CDM.

https://reviewboard.mozilla.org/r/209836/#review218668

> If we fail to generate the storage ID in the parent, will mStorageId.get() be nullptr here? Maybe we need to also explicitly check whether mStorageId.IsEmpty() here, and pass nullptr if so?

Thanks for reminding. It will return non-null pointer from get() when mStorageId is empty. I will explicitly check it and pass nullptr instead of calling get().

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1595c6002aa6
Part1 - Provide a utility function to compute the storage id. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/5a46db143897
Part2 - Pass the storage id from content process to GMP process then provide it to CDM. r=cpearce

Comment 28

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1595c6002aa6
https://hg.mozilla.org/mozilla-central/rev/5a46db143897