Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Second call of player.configure fails #784

Closed
Ross-cz opened this issue May 5, 2017 · 2 comments
Closed

Second call of player.configure fails #784

Ross-cz opened this issue May 5, 2017 · 2 comments
Assignees
@joeyparrish
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone
v2.2.0

Comments

@Ross-cz
Copy link
Contributor

Ross-cz commented May 5, 2017

  • What version of Shaka Player are you using?
    395cc17

    • Can you reproduce the issue with our latest release version?
      y
    • Can you reproduce the issue with the latest code from master?
      y

  • Are you using the demo app or your own custom app?
    y

    • If custom app, can you reproduce the issue using our demo app?
      y

  • What browser and OS are you using?
    any

  • What did you do?
    Load configuration using pseudo script called twice using console:
    window.shakaDemo.localPlayer_.configure({"drm":{"advanced":{"com.widevine.alpha":{"serverCertificate":new Uint8Array(1)}}}})

  • What did you expect to happen?
    no exception

  • What actually happened?
    first call result: undefined
    second call result:
    Uncaught TypeError: Cannot read property '0' of null
@TheModMaker TheModMaker added type: bug Something isn't working correctly flag: good first issue This might be a relatively easy issue; good for new contributors labels May 5, 2017
@TheModMaker TheModMaker added this to the v2.2.0 milestone May 5, 2017
@joeyparrish joeyparrish self-assigned this May 8, 2017
@joeyparrish joeyparrish removed the flag: good first issue This might be a relatively easy issue; good for new contributors label May 8, 2017
@joeyparrish
Copy link
Member

It is easy enough to fix the failure, but we will lose type-checking on the serverCertificate parameter.

Therefore I'm going to fix this in two parts:

  1. Fix the immediate failure in a very simple way, which can be cherry-picked to v2.1.1. "serverCertificate" will be treated like "abr.manager" in ConfigUtils, and we will copy it directly, as opposed to recursively. This means, though, that we will not be checking its type.
  2. Apply a more detailed fix that will only appear in v2.2. When we refactor ABR configuration for Make AbrManager targets configurable #744, "abr.manager" will be removed from the configuration. This will make it easier to clean up and fix config merging in a way that allows type-checking of serverCertificate as a Uint8Array.

Part 2 will only be merged after #744 is fixed.

shaka-bot pushed a commit that referenced this issue May 8, 2017
@joeyparrish
Don't recurse on serverCertificate in config merge
f992851 
Treating serverCertificate as an Object and recursing causes an
exception the second time you set the serverCertificate config.

As a quick fix that can be cherry-picked for v2.1.x, do not recurse
on serverCertificate.  This has the side-effect of not type-checking
the serverCertificate field on input.

A more detailed fix will be made later, for inclusion in v2.2.

Issue #784

Change-Id: I84c05ee3dd370a4b83e9ce2337d2326ec36532c2
joeyparrish added a commit that referenced this issue May 8, 2017
@joeyparrish
Don't recurse on serverCertificate in config merge
63121b1 
Treating serverCertificate as an Object and recursing causes an
exception the second time you set the serverCertificate config.

As a quick fix that can be cherry-picked for v2.1.x, do not recurse
on serverCertificate.  This has the side-effect of not type-checking
the serverCertificate field on input.

A more detailed fix will be made later, for inclusion in v2.2.

Issue #784

Change-Id: I84c05ee3dd370a4b83e9ce2337d2326ec36532c2
joeyparrish added a commit that referenced this issue May 10, 2017
@joeyparrish
Don't recurse on serverCertificate in config merge
195594f 
Treating serverCertificate as an Object and recursing causes an
exception the second time you set the serverCertificate config.

As a quick fix that can be cherry-picked for v2.1.x, do not recurse
on serverCertificate.  This has the side-effect of not type-checking
the serverCertificate field on input.

A more detailed fix will be made later, for inclusion in v2.2.

Issue #784

Change-Id: I84c05ee3dd370a4b83e9ce2337d2326ec36532c2
@joeyparrish
Copy link
Member

The first part of the fix has been cherry-picked to v2.1.1 and v2.0.9. The type of the serverCertificate field will not be checked by configure(). Type-checking will be fixed in v2.2.0, as it depends on some refactoring in-progress.

@shaka-project shaka-project locked and limited conversation to collaborators Mar 22, 2018
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@joeyparrish joeyparrish

Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Milestone
v2.2.0
Development

No branches or pull requests
4 participants
@Ross-cz @TheModMaker @joeyparrish @shaka-bot