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

Fix TypeError in PubSubClass.unsubscribe #44

Closed
wants to merge 1 commit into from

Conversation

mgedmin
Copy link

@mgedmin mgedmin commented Mar 17, 2014

Modifying an array while iterating over it is tricky. If we delete an
element in the middle and continue to iterate using the old length,
we'll perform an access beyond the end of the array, get back an
undefined, and then get a TypeError when we try to [0] it.

I've seen this in the wild, when attempting to show a popup that
embeds jSignature for the second time on the same page. The
error is uncaught and disables all subsequent JavaScript on the
page.

I don't know if this method was meant to remove just the first matching
callback, or all of them. My modification assumes the first; if you
need the second, instead of break do i--; l--.

Modifying an array while iterating over it is tricky.  If we delete an
element in the middle and continue to iterate using the old length,
we'll perform an access beyond the end of the array, get back an
undefined, and then get a TypeError when we try to [0] it.

I don't know if this method was meant to remove just the first matching
callback, or all of them.  My modification assumes the first; if you
need the second, instead of break do `i--; l--`.
@mgedmin
Copy link
Author

mgedmin commented Mar 23, 2014

My fix is incomplete: I still get a TypeError when I try to show a popup with a signature the second time, only the new error is in a different place.

It looks like jSignature is trying to remove a PubSub subscriber from inside a PubSub callback handler, which makes the dispatch code iterate over more array indexes than remain. This one is harder to fix. Idea: copy the array before looping through it in the publish method.

mgedmin pushed a commit to mgedmin/jSignature that referenced this pull request Apr 18, 2014
@mgedmin mgedmin closed this Nov 15, 2018
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

1 participant