Closed Bug 1360863 Opened 5 years ago Closed 5 years ago

EME prompt is not shown

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox-esr52 --- unaffected
firefox53 + wontfix
firefox54 + verified
firefox55 + verified

People

(Reporter: ajones, Assigned: cpearce)

References

Details

(Whiteboard: [bugday-20170510])

Attachments

(3 files, 1 obsolete file)

Steps to reproduce:

* Disable EME
* Attempt to play EME content on Netflix or YouTUbe

Expected results:

Drop down shown to enable DRM

Actual results:

No drop down. Unhelpful error message shown on Netflix/YouTube
Flags: needinfo?(cpearce)
A simpler test case is:

https://shaka-player-demo.appspot.com/demo/#asset=//yt-dash-mse-test.commondatastorage.googleapis.com/media/oops_cenc-20121114-signedlicenseurl-manifest.mpd;lang=en-US;play

This URL should play if "Play DRM Content" is checked, and show the "enable DRM" notification box if not.

I see this error logged in console:

*** Failed to format string emeNotifications.drmContentDisabled.message in bundle: chrome://browser/locale/browser.properties
JavaScript error: XStringBundle, line 37: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.formatStringFromName]

I bet something change in the front end.
Flags: needinfo?(cpearce)
Blocks: 1330001
Component: Audio/Video: Playback → General
Product: Core → Firefox
This bug causes us to not show the "Enable DRM playback" notification box when users visit Netflix.com or any other site that tries to use DRM playback while DRM playback is disabled. We ship with DRM playback disabled by default on Linux (only). So this bug means that Linux users won't be prompted to enable DRM playback, and so sites trying to use DRM will not work and there will be no obvious way to make them work!
Assignee: nobody → cpearce
Once this lands, let's verify the fix and uplift to at least 54.
Duplicate of this bug: 1360866
Comment on attachment 8863290 [details]
Bug 1360863 - Unremove emeNotifications.drmContentDisabled.message.

https://reviewboard.mozilla.org/r/135096/#review138512
Attachment #8863290 - Flags: review?(dao+bmo) → review+
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ec98c2a4824
Unremove emeNotifications.drmContentDisabled.message. r=dao
https://hg.mozilla.org/mozilla-central/rev/2ec98c2a4824
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #5)
> Once this lands, let's verify the fix and uplift to at least 54.

The patch for uplift should have the string hard-coded. The error it's already in release, which means we lost that string all over the place.
I have reproduced this bug with Nightly 55.0a1 (2017-04-29) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly !

Build   ID    20170503030212
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170503]
Hi :cpearce,
Could you please nominate patch for uplift to Bet54?
Flags: needinfo?(cpearce)
Comment on attachment 8865382 [details] [diff] [review]
Beta rebase with string hardcoded

(In reply to Francesco Lodolo [:flod] from comment #10)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #5)
> > Once this lands, let's verify the fix and uplift to at least 54.
> 
> The patch for uplift should have the string hard-coded. The error it's
> already in release, which means we lost that string all over the place.

Like this?
Flags: needinfo?(francesco.lodolo)
I don't think I've ever seen a nested ternary operator, but yes, it should do the right: read the hard-coded string if the ID is emeNotifications.drmContentDisabled.message, otherwise fall back to the "proper" code path.
Flags: needinfo?(francesco.lodolo)
Comment on attachment 8865382 [details] [diff] [review]
Beta rebase with string hardcoded

I have tested this code and it works as expected, but would be good to get a once over from a front end reviewer before requesting uplift to beta.
Flags: needinfo?(cpearce)
Attachment #8865382 - Flags: review?(jaws)
Comment on attachment 8865382 [details] [diff] [review]
Beta rebase with string hardcoded

Review of attachment 8865382 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-media.js
@@ +103,5 @@
>  
>      let msgPrefix = "emeNotifications." + notificationId + ".";
>      let msgId = msgPrefix + "message";
>  
> +    let message = (notificationId == "drmContentDisabled") ?

We should try to load the string from the .properties file first, and if that fails then fall back to the hard coded string. As this patch is written, locales won't be able to fix it if they want to.

Can we wrap the getFormattedString call in a try/catch and then if the msgId is "emeNotifications.drmContentDisabled.message" and an exception is thrown we will use the hard-coded string?
Attachment #8865382 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> We should try to load the string from the .properties file first, and if
> that fails then fall back to the hard coded string. As this patch is
> written, locales won't be able to fix it if they want to.

This patch is only for Beta (maybe release), we don't have the string available there, so there's not much of a point in trying to load it.
Attachment #8865382 - Attachment is obsolete: true
Comment on attachment 8866018 [details] [diff] [review]
Beta rebase with string hardcoded

Review of attachment 8866018 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8866018 - Flags: review?(jaws) → review+
Comment on attachment 8866018 [details] [diff] [review]
Beta rebase with string hardcoded

Approval Request Comment

[Feature/Bug causing the regression]: DRM protected video playback (i.e. Netflix). This is a regression from Bug 1330001 which accidentally removed a string.

[User impact if declined]: Users who have "Play DRM Content" unchecked in Firefox's preferences (which is by default all Linux users) will see sites that use DRM playback like Netflix fail with no indication from our UI as to why they had failed.

[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: No. I have verified with a local build. The fix to Nightly re-added the string. But because we can't take string changes to beta (right?), this patch hard codes the string in English. So it's a completely different patch to what landed on Nightly.

[Needs manual test from QE? If yes, steps to reproduce]: Yes please. Disable DRM playback in about:preferences, load https://shaka-player-demo.appspot.com/ and you should see a notification box drop down "You must enable DRM to play some audio or video on this page."

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: No

[Why is the change risky/not risky?]: It's simple.

[String changes made/needed]: Yes, this patch hard codes a string was removed accidentally with an English string. This patch also checks to see whether the string is there first, so if our translators could re-add the fix we'd be able to gracefully start using the localized string.
Attachment #8866018 - Flags: approval-mozilla-beta?
We should really get a test for this... maybe a browser chrome mochitest? I thought we had one already for this feature...
Flags: in-testsuite?
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: qe-verify+
Flags: needinfo?(brindusa.tot)
¡Hola Gerry!

FWIW I do get the prompt.

Nombre 	Firefox
Versión 	55.0a1
Id. de compilación 	20170510183715

¡Gracias!
Alex
Status: RESOLVED → VERIFIED
Whiteboard: [bugday-20170510]
Comment on attachment 8866018 [details] [diff] [review]
Beta rebase with string hardcoded

Fix an EME prompt issue and was verified. Beta54+. Should be in 54 beta 7.
Attachment #8866018 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(brindusa.tot)
Sorry, yesterday was a full day.

Verified as fixed on latest Nightly 55.0a1, Build ID 20170510030301 on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.
See Also: → 1366167
I have managed to reproduce this issue on Firefox 53 using STR from comment 0.

This is verified fixed on 54 beta 9 across platforms: Windows 10 x64, Ubuntu 16.04 x64 LTS and Mac OS X 10.10.5.
Flags: qe-verify+
I added a test for this feature in bug 1366167.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.