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.

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

handle already defined crypto object in webworker #68

Closed
wants to merge 1 commit into from

Conversation

sualko
Copy link
Contributor

@sualko sualko commented Jun 11, 2015

With the latest version of Chrome (43.0.2357.124 (64-bit)) I get an error in line 7 for dsa-webworker.js:

Uncaught TypeError: Cannot set property crypto of #<WorkerGlobalScope> which has only a getter

Console.log for root.crypto prints:

Crypto {}
  subtle: SubtleCrypto

Patch tested in Chrome 43, Chromium 41.0.2272.76 and Firefox 38.0.

@arlolra
Copy link
Owner

arlolra commented Jun 11, 2015

Thanks for pointing this is out!

Console.log there is only enumerating the own properties but getRandomValues is available on the prototype chain, so we should use that where possible.

How does this look to you? f52a756

@sualko
Copy link
Contributor Author

sualko commented Jun 12, 2015

but getRandomValues is available on the prototype chain

You are right, I overlooked that and therefore your implementation is far better. 👍

@arlolra
Copy link
Owner

arlolra commented Jun 12, 2015

Ok, great. Merged. I'll try and cut a release soon.

@arlolra arlolra closed this Jun 12, 2015
@sualko
Copy link
Contributor Author

sualko commented Jun 17, 2015

I am not sure, why this did not pop up in my tests, but we have an error. We have to import does scripts, even if we have a crypto function available.

if (data.imports) imports = data.imports
importScripts.apply(root, imports);

if (!hasCrypto) {
      // use salsa20 when there's no prng in webworkers
      var state = new root.Salsa20(data.seed.slice(0, 32), data.seed.slice(32))
      root.crypto.randomBytes = function (n) {
        return state.getBytes(n)
      }
    }

Should I create a new pr?

arlolra added a commit that referenced this pull request Jun 17, 2015
@arlolra
Copy link
Owner

arlolra commented Jun 17, 2015

@sualko Thanks for catching that. I pushed a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants