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

calling setToolModeForElement too many times can cause an exception #898

Closed
3 tasks done
mikehazell opened this issue Mar 25, 2019 · 1 comment
Closed
3 tasks done
Assignees
Labels
Bug: Report 🐛 Bug reported, but not verified. Work In Progress

Comments

@mikehazell
Copy link
Contributor

Prerequisites

  • Are you running the latest version?
  • Are you reporting to the correct repository?
  • Did you perform a cursory search?

Description

Bug

Calling any of the setToolMode methods too many time with options of { mouseButtonMask: 1 } will eventually cause the app to crash with a "RangeError: Invalid array length" error.

This is happening this line: https://github.com/cornerstonejs/cornerstoneTools/blob/master/src/store/setToolMode.js#L213

  // Keep the same if not an array (undefined)
  // Reset if empty array
  // Merge if array contains any bindings
  if (
    Array.isArray(options.mouseButtonMask) &&
    options.mouseButtonMask.length !== 0 &&
    Array.isArray(tool.options.mouseButtonMask)
  ) {
    options.mouseButtonMask = options.mouseButtonMask.concat(
      tool.options.mouseButtonMask
    );
  }

The code here is concatenating the mouseButtonMask array but not removing duplicates.

I'll create a PR shortly with tests showing the issue and a fix.

@dannyrb dannyrb added Bug: Report 🐛 Bug reported, but not verified. Work In Progress labels Mar 25, 2019
mikehazell added a commit to mikehazell/cornerstoneTools that referenced this issue Mar 25, 2019
…rray on a tool

Fixes a bug where the app can have poor performance or even crash when setToolModeForElement is
called too many times.

cornerstonejs#898
@dannyrb
Copy link
Member

dannyrb commented Mar 27, 2019

Merged 👍

@dannyrb dannyrb closed this as completed Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug: Report 🐛 Bug reported, but not verified. Work In Progress
Projects
None yet
Development

No branches or pull requests

2 participants