Skip to content

Conversation

@janicduplessis
Copy link
Contributor

Passing undefined or null to clearImmediate caused apps to crash. It it caused because we try to find the index of the null/undefined timer when we should just do nothing when passed these values.

It is already handled properly in the other Timer functions.

Test plan
Calling clearImmediate with undefined or null should do nothing.

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @spicyj, @vjeux and @nicklockwood to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Feb 27, 2016
@vjeux
Copy link
Contributor

vjeux commented Feb 27, 2016

@facebook-github-bot shipit

Good catch!

@janicduplessis
Copy link
Contributor Author

@vjeux I think the bot is sleeping :(

JSTimersExecution.immediates.indexOf(timerID),
1
);
if (timerID != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be !== ? @vjeux

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want != here so it works will null and undefined. It's the same check the is used in other places in this file.

@vjeux
Copy link
Contributor

vjeux commented Feb 28, 2016

Actually, it would still crash if you clearImmediate a value that has already been removed.

Could you do

var index = x.indexOf()
if (index !== -1) {
  x.splice()

@facebook-github-bot
Copy link
Contributor

@janicduplessis updated the pull request.

@vjeux
Copy link
Contributor

vjeux commented Feb 28, 2016

why do you still check for timer != null? Index === -1 check should catch this no?

@janicduplessis
Copy link
Contributor Author

I wasn't sure if there could be null values in the immediates array since there was a check for it for the timerID array (https://github.com/janicduplessis/react-native/blob/clear-immediate/Libraries/JavaScriptAppEngine/System/JSTimers/JSTimers.js#L120). I double checked if it was possible and it is not so you are right I can remove the null check.

@facebook-github-bot
Copy link
Contributor

@janicduplessis updated the pull request.

@vjeux
Copy link
Contributor

vjeux commented Feb 28, 2016

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in 44fcf22 Feb 28, 2016
@janicduplessis janicduplessis deleted the clear-immediate branch March 14, 2016 03:04
pglotov pushed a commit to pglotov/react-native that referenced this pull request Mar 15, 2016
Summary:Passing `undefined` or `null` to `clearImmediate` caused apps to crash. It it caused because we try to find the index of the null/undefined timer when we should just do nothing when passed these values.

It is already handled properly in the other Timer functions.

**Test plan**
Calling `clearImmediate` with `undefined` or `null` should do nothing.
Closes facebook#6192

Differential Revision: D2987778

Pulled By: vjeux

fb-gh-sync-id: 6fd38cfa3c10012caa2afb27cbdab95df696a769
shipit-source-id: 6fd38cfa3c10012caa2afb27cbdab95df696a769
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants