check message from webworker #9

Merged
merged 1 commit into from Nov 10, 2013

Conversation

Projects
None yet
2 participants
Contributor

javisantana commented Jul 31, 2013

facebook library calls onmessage and LZMA.decompress was being called (failing)

Collaborator

nmrugg commented Aug 1, 2013

I'm inclined to accept this as a temporary fix, but it looks like the real problem is that emulating Web Workers is conflicting with other libraries. I'll look into fixing that.

Contributor

javisantana commented Aug 1, 2013

yep, maybe is a temporally fix, but it's needed since right now onmessage is trying to compress or decompress every message.

Collaborator

nmrugg commented Nov 9, 2013

OK, I rewrote lzma.js to not pollute the global scope, so onmessage() should not get fired accidentally. Let me know if this issue persists.

nmrugg closed this Nov 9, 2013

Contributor

javisantana commented Nov 9, 2013

I don't know if i'm looking to the wrong place but it looks like the bug is still there (https://github.com/nmrugg/LZMA-JS/blob/master/src/lzma_worker.js#L3912) (onmessage is global)

I don't understand why assuming that if it's not a compression message it must be a decompression. The check of this pull request is still valid. At the end this pull request only adds a check that avoids an error if onmessage is called with wrong data

We're using your library with this pull request in cartodb.js (https://github.com/CartoDB/cartodb.js/) for months and there was no problem (millions of users).

Thanks for your time

nmrugg reopened this Nov 10, 2013

@nmrugg nmrugg merged commit 782a2d2 into LZMA-JS:master Nov 10, 2013

Collaborator

nmrugg commented Nov 10, 2013

Did you try it? It may look like the bug is still there, but this section is now wrapped in a function that is only called if we are in a Web Worker, so it shouldn't get called by some other code.

I originally used a simple else statement since only those two options should be called, and anything else should be an error (like this bug). But, now that it only uses the Web Worker API when it actually exists, we shouldn't be running into these types of bugs any more, so ignoring incorrect calls to onmessage() makes sense too.

So, I merged your pull request, but I also removed the part about accepting action_update since that action is never used that direction. It is used to send progress updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment