Closed Bug 1390739 Opened 5 years ago Closed 5 years ago

Dispatch the task to main thread if the callback of CDM does not on main thread

Categories

(Core :: Audio/Video: GMP, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox56 --- verified
firefox57 --- verified

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

Details

Attachments

(1 file)

We've tested the widevine 984 CDM and it seems they do some implementation change by calling the callback with it's own non-main-thread threads.

We will bump assertion and need to fix it by dispatching the task to main thread.
Comment on attachment 8897711 [details]
Bug 1390739 - Dispatch the task to main thread if the callback of CDM does not on main thread.

https://reviewboard.mozilla.org/r/169026/#review174328

::: dom/media/gmp/ChromiumCDMChild.cpp:206
(Diff revision 1)
> +        this,
> +        &ChromiumCDMChild::OnResolveNewSessionPromise,
> +        aPromiseId,
> +        aSessionId,
> +        aSessionIdSize));
> +  }

You need to return after posting the task, else you'll still run the body of the function off main thread, and then run it again on main thread when the callback runs (if you don't crash first!)

You also need to ensure you copy every data that you don't own (like the const char\* aSessionId here, and other pointers in other functions) so that the task has an owned copy. Else when the task runs, it may end up deferencing memory that's been deallocated by the CDM!

These to problems are present in most of your changes here.
Attachment #8897711 - Flags: review?(cpearce) → review-
Comment on attachment 8897711 [details]
Bug 1390739 - Dispatch the task to main thread if the callback of CDM does not on main thread.

https://reviewboard.mozilla.org/r/169026/#review174380

::: dom/media/gmp/ChromiumCDMChild.cpp:197
(Diff revision 2)
> -ChromiumCDMChild::OnResolveNewSessionPromise(uint32_t aPromiseId,
> +ChromiumCDMChild::CallMethod(MethodType aMethod, ParamType&&... aParams)
> -                                             const char* aSessionId,
> -                                             uint32_t aSessionIdSize)
>  {
>    MOZ_ASSERT(IsOnMessageLoopThread());
> -  GMP_LOG("ChromiumCDMChild::OnResolveNewSessionPromise(pid=%" PRIu32
> +  Unused << (this->*aMethod)(Forward<ParamType>(aParams)...);

What if the main thread receives and enqueues a Destroy() message, and then the CDM calls an OnXX function off main thread, so we enqueue a runnable in the main thread's event queue, then on the main thread we process the message for calling Destroy() and we shutdown the IPC connection, and then we run the OnXX runnable and try to send an IPC message. Then we'll end up with another fatal assertion because the IPC connection is shutdown.

You need to make sure that you do not try to run the methods after Destroy() has been called and the IPC connection has been shutdown.

You should set a flag in Destroy() which you check in CallMethod(). The original code in GMPDecrytporChild had this check.

::: dom/media/gmp/ChromiumCDMChild.cpp:202
(Diff revision 2)
> -  GMP_LOG("ChromiumCDMChild::OnResolveNewSessionPromise(pid=%" PRIu32
> -          ", sid=%s)",
> -          aPromiseId,
> -          aSessionId);
> +  Unused << (this->*aMethod)(Forward<ParamType>(aParams)...);
> +}
> +
> +template<typename MethodType, typename... ParamType>
> +void
> +ChromiumCDMChild::CallOnMessageLoopThread(const char* const aName ,

You've got a space after aName but before ",".

You should run `./mach clang-format` to auto format your code. It'e easier.
Attachment #8897711 - Flags: review?(cpearce) → review-
Comment on attachment 8897711 [details]
Bug 1390739 - Dispatch the task to main thread if the callback of CDM does not on main thread.

https://reviewboard.mozilla.org/r/169026/#review174380

> What if the main thread receives and enqueues a Destroy() message, and then the CDM calls an OnXX function off main thread, so we enqueue a runnable in the main thread's event queue, then on the main thread we process the message for calling Destroy() and we shutdown the IPC connection, and then we run the OnXX runnable and try to send an IPC message. Then we'll end up with another fatal assertion because the IPC connection is shutdown.
> 
> You need to make sure that you do not try to run the methods after Destroy() has been called and the IPC connection has been shutdown.
> 
> You should set a flag in Destroy() which you check in CallMethod(). The original code in GMPDecrytporChild had this check.

I use mCDM as my flag to check.
Comment on attachment 8897711 [details]
Bug 1390739 - Dispatch the task to main thread if the callback of CDM does not on main thread.

https://reviewboard.mozilla.org/r/169026/#review174714

Thanks!
Attachment #8897711 - Flags: review?(cpearce) → review+
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0dec0ba29bca
Dispatch the task to main thread if the callback of CDM does not on main thread. r=cpearce
https://hg.mozilla.org/mozilla-central/rev/0dec0ba29bca
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8897711 [details]
Bug 1390739 - Dispatch the task to main thread if the callback of CDM does not on main thread.

Approval Request Comment
[Feature/Bug causing the regression]:NO
[User impact if declined]: user could not play DRM content. When we update new CDM.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]:
Please follow this order for uplifting
1. Bug 1390453
2. This Bug 1390739
3. Bug 1390725
4. Bug 1392175 
[Is the change risky?]:No
[Why is the change risky/not risky?]:The change did not effect on the current CDM.  
[String changes made/needed]: No
Attachment #8897711 - Flags: approval-mozilla-beta?
James, on this and the related CDM bugs (bug 1390725, bug 1392175) Can you explain for QE how to verify the fix in nightly before we uplift this work to beta?  

Andrei, if your team can have a look to test (either what you can figure out or by talking with James) that would be great.
Flags: needinfo?(jacheng)
Flags: needinfo?(andrei.vaida)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #12)
> James, on this and the related CDM bugs (bug 1390725, bug 1392175) Can you
> explain for QE how to verify the fix in nightly before we uplift this work
> to beta?  
> 
Sure, I think QE should use the official Nightly(tomorrow not today's) to test.

The testing step is as followed,
1. Open Nightly,
2. Log into the Netflix and watch a movie.(Make sure we can play it)
3. Open a tab and go to "about:profiles", use the icon to open the currently used profile folder then open the "gmp-widevinecdm/1.4.8.970" folder. 
4. Close the Nightly entirely(Command + Q).
5. Download the 984 CDM from https://redirector.gvt1.com/edgedl/widevine-cdm/1.4.8.984-mac-x64.zip, and extract the zip into the "gmp-widevinecdm/1.4.8.970" to overwrite those files. (libwidevinecdm.dylib, manifest.json, libwidevinecdm.dylib.sig). Open manifest.json to make sure it shows "version": "1.4.8.984".
6. Open Nightly to watch Netflix again(Make sure we can play it).

Test done. But Ideally we could play Netflix with CDM 1008, so please continue testing.
7. Close the Nightly entirely(Command + Q).
8. Download 1008 CDM from https://redirector.gvt1.com/edgedl/widevine-cdm/1.4.8.1008-mac-x64.zip, and extract the zip into the "gmp-widevinecdm/1.4.8.970" to overwrite those files again. (libwidevinecdm.dylib, manifest.json, libwidevinecdm.dylib.sig). Open manifest.json to make sure it shows "version": "1.4.8.1008".
9. Open Nightly to watch Netflix again(Make sure we can play it).

If the instruction is not clear, please feel free to let me know.

Thank you.
> Andrei, if your team can have a look to test (either what you can figure out
> or by talking with James) that would be great.
Flags: needinfo?(jacheng)
I've managed to verify this issue on latest Nightly 57.0a1 (2017-08-22) with Mac OS X 10.11.6. Confirming that everything works as expected on Netflix using CDM version "1.4.8.984" and "1.4.8.1008".
Flags: needinfo?(andrei.vaida)
Ciprian,
Can you help test 984 CDM (step#1-6) on Windows 10?
Flags: needinfo?(ciprian.georgiu)
(In reply to Blake Wu [:bwu][:blakewu] from comment #15)
> Ciprian,
> Can you help test 984 CDM (step#1-6) on Windows 10?

Hi Ciprian,

For testgin Windows, you may need to download the CDM manually from

Windows 32bit
984 CDM: https://redirector.gvt1.com/edgedl/widevine-cdm/1.4.8.984-win-ia32.zip
1008 CDM: https://redirector.gvt1.com/edgedl/widevine-cdm/1.4.8.1008-win-ia32.zip

Windows 64bit
984 CDM: https://redirector.gvt1.com/edgedl/widevine-cdm/1.4.8.984-win-x64.zip
1008 CDM: https://redirector.gvt1.com/edgedl/widevine-cdm/1.4.8.1008-win-x64.zip
The step is the same as Mac

Please test the 32bit and 64bit version of Firefox on Windows.

Our expected result is

984 CDM works well and 1008 may encounter problems.

Thanks
I have tested this with latest Nightly 57.0a1 on Windows 10 x64/x86 as well. I can confirm that 984 CDM version is working as expected.

I encounter an issue with 1008 CDM, but per our offline discussion with James, that issue is going to be fixed in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1392485.   

Thanks James for your help!
Flags: needinfo?(ciprian.georgiu)
Comment on attachment 8897711 [details]
Bug 1390739 - Dispatch the task to main thread if the callback of CDM does not on main thread.

#2 of patches for CDM change in beta 6.
Attachment #8897711 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hi James,

I've re-tested this bug on 56 beta 6 (20170825011442) using Windows 10 x64/x86 and Mac OS X 10.11.6. It seems that 984 CDM is not working on Windows 10, after step #6 (comment 13) I get a page error when trying to watch a movie: http://imgur.com/a/fWKtb. It appears to be the same issue from bug 1392485, so I double checked again on latest Nightly 57 and 984 CMD works well there (as stated in comment 17). On OS X, I had the same results with both 984 CDM and 1008 CDM.

Can you please take a look at this?
Flags: needinfo?(jacheng)
Hi Ciprian,

For Beta,

Please see this comment,

https://bugzilla.mozilla.org/show_bug.cgi?id=1383771#c27 

The main reason that you cannot test new CDM on Beta is that we haven't provided ".sig" files in Beta.

You may notice that there are firefox.exe.sig / plugin-container.exe.sig / xul.dll.sig in the same folder as firefox.exe in Nightly but Beta.  

Please test it after bug 1393639 is resolved and bug 1383771 is uplifted.

Thanks
Depends on: 1393639
Flags: needinfo?(jacheng)
Thanks James!

Verified as fixed on 56.0b12 (20170914024831) under Windows 10 x64/x86 and Mac OS X 10.11.6.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.