Closed Bug 1156560 Opened 5 years ago Closed 5 years ago

[EME] Persistent data is not shared between EME CDM updates

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- fixed
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: eflores, Assigned: eflores)

References

Details

Attachments

(2 files, 2 obsolete files)

Reported by Adobe.

Need to remove the version string from the node ID calculation from bug 1148071 and keep using the old CDM until all instances of it have died.
Attached patch use-old-gmps.patch (obsolete) — Splinter Review
Attachment #8595044 - Flags: review?(cpearce)
Duplicate of this bug: 1156564
Comment on attachment 8595044 [details] [diff] [review]
use-old-gmps.patch

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

::: dom/media/gmp/GMPServiceParent.cpp
@@ +583,5 @@
>          gmp->SetNodeId(aNodeId);
>          return gmp;
>        }
>  
> +      if (!gmpToClone || gmpToClone->State() < gmp->State() ||

I think this will cause us to prefer to re-use plugins that are already in-use, meaning we'll have the effect of prolonging the lifetime of out-of-date plugins. I think we should just remove the (gmpToClone->State() < gmp->State()) condition here.

::: dom/media/gmp/PGMPService.ipdl
@@ +18,5 @@
>    sync LoadGMP(nsCString nodeId, nsCString api, nsCString[] tags,
>                 ProcessId[] alreadyBridgedTo)
>      returns (ProcessId id, nsCString displayName, nsCString pluginId);
>    sync GetGMPNodeId(nsString origin, nsString topLevelOrigin,
> +                    bool inPrivateBrowsing)

You need to rev the uuid here.

::: dom/media/gmp/mozIGeckoMediaPluginService.idl
@@ +144,5 @@
>     */
>    [noscript]
>    void getNodeId(in AString origin,
>                   in AString topLevelOrigin,
>                   in bool inPrivateBrowsingMode,

You need to rev the uuid here.
Attachment #8595044 - Flags: review?(cpearce) → review+
Comment on attachment 8595047 [details] [diff] [review]
use-old-gmps-aurora.patch

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

::: dom/media/gmp/GMPService.cpp
@@ +907,5 @@
>          gmp->SetNodeId(aNodeId);
>          return gmp;
>        }
>  
> +      if (!gmpToClone || gmpToClone->State() < gmp->State() ||

Same as the other patch, namely:

I think this will cause us to prefer to re-use plugins that are already in-use, meaning we'll have the effect of prolonging the lifetime of out-of-date plugins. I think we should just remove the (gmpToClone->State() < gmp->State()) condition here.

::: dom/media/gmp/mozIGeckoMediaPluginService.idl
@@ +109,5 @@
>     * Gets the NodeId for a (origin, urlbarOrigin, isInprivateBrowsing) tuple.
>     */
>    ACString getNodeId(in AString origin,
>                       in AString topLevelOrigin,
> +                     in bool inPrivateBrowsingMode);

You need to rev the uuid here.
Attachment #8595047 - Flags: review?(cpearce) → review+
Attachment #8595044 - Attachment is obsolete: true
Will need uplift.
Flags: needinfo?(cpearce)
https://hg.mozilla.org/mozilla-central/rev/cefce495a0bf
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8595116 [details] [diff] [review]
use-old-gmps.patch

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: Without this patch, when the EME Plugin updates, it will effectively orphan all storage from the old version. This will mean any DRM licenses and individualization that have been cached will not be present, and so startup of EME sessions will be slower. Also any unfinished playback sessions remembered by the site (say in IndexedDB) will not be present in the EME Plugin's storage, so the site will see inconsistent state and could be confused. (Adobe's QE picked this up)
[Describe test coverage new/current, TreeHerder]: We have plenty of EME mochitests
[Risks and why]: Low; most of this patch is removing code, and there's only a small logic change in there to make us keep using something that already works for a little longer.
[String/UUID change made/needed]: UUID change of mozIGeckoMediaPluginService. Previously gfritzsche was OK with changing this.

(We need this to 38... I'm not actually sure what's going on with us having uplifted 38 to Release early, so I requested Release approval... we *don't* need to uplift this to 37).
Attachment #8595116 - Flags: approval-mozilla-release?
Attachment #8595116 - Flags: approval-mozilla-beta?
Attachment #8595116 - Flags: approval-mozilla-aurora?
Comment on attachment 8595116 [details] [diff] [review]
use-old-gmps.patch

Should be in 38 beta 7.
(no need to uplift to beta for now)
Attachment #8595116 - Flags: approval-mozilla-release?
Attachment #8595116 - Flags: approval-mozilla-release+
Attachment #8595116 - Flags: approval-mozilla-beta?
Attachment #8595116 - Flags: approval-mozilla-aurora?
Attachment #8595116 - Flags: approval-mozilla-aurora+
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.