Skip to content

RawLabel: Fix crash when text is empty string.#4801

Merged
gnprice merged 1 commit intozulip:masterfrom
WesleyAC:rawlabel-empty-string
Jun 15, 2021
Merged

RawLabel: Fix crash when text is empty string.#4801
gnprice merged 1 commit intozulip:masterfrom
WesleyAC:rawlabel-empty-string

Conversation

@WesleyAC
Copy link
Copy Markdown
Contributor

@WesleyAC WesleyAC commented Jun 14, 2021

This was causing a crash when trying to autocomplete a user group with a
blank name. The empty string was passed into RawLabel, which caused this
invariant to incorrectly trigger.

Instead of using !! to check for undefined, we can do != null, which
will trigger for undefined and null, but not an empty string.

If you want to trigger this on CZO, just type @test into the composebox :)

@WesleyAC WesleyAC requested review from chrisbobbe and gnprice June 14, 2021 22:42
@WesleyAC WesleyAC force-pushed the rawlabel-empty-string branch from ab67152 to 0c70b5f Compare June 14, 2021 22:46
@WesleyAC WesleyAC changed the title RawLabel: Fix bug when text is empty string. RawLabel: Fix dev crash when text is empty string. Jun 14, 2021
@WesleyAC WesleyAC force-pushed the rawlabel-empty-string branch from 0c70b5f to 421f79e Compare June 14, 2021 22:51
@WesleyAC WesleyAC force-pushed the rawlabel-empty-string branch from 421f79e to a760a73 Compare June 14, 2021 23:08
@WesleyAC WesleyAC changed the title RawLabel: Fix dev crash when text is empty string. RawLabel: Fix crash when text is empty string. Jun 14, 2021
Copy link
Copy Markdown
Contributor

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

Thanks for catching this!! One tiny correctness hole that I think I've found, below; possibly not worth addressing, but also wouldn't be too hard to address if you chose to.

Comment thread src/common/RawLabel.js Outdated
const { text, children, style, ...restProps } = this.props;

invariant(!!text !== !!children, 'pass either `text` or `children`');
invariant((text != null) !== !!children, 'pass either `text` or `children`');
Copy link
Copy Markdown
Contributor

@chrisbobbe chrisbobbe Jun 14, 2021

Choose a reason for hiding this comment

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

It occurs to me that children can be a number or a string; it's typed ?Node (link) and I think the type Flow uses for Node includes strings and numbers (doc).

It'd be super weird, but I guess one could pass "foo" for text and 0 for children, and circumvent this check, ending up with "foo0" on the screen?

I'm trying to think whether that case is totally wacky and you'd really have to be trying to find it in order for it to be active. Or if you could stumble on it innocently.

I think it's not totally impossible to stumble on it innocently: there's this gotcha in this conditional rendering doc from React:

Note that returning a falsy expression will still cause the element after && to be skipped but will return the falsy expression. In the example below, <div>0</div> will be returned by the render method.

If you're passing children and you've accidentally stumbled on that gotcha there, and you're also passing a string for text, there could be a case like foo0.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, seems cleanest to use the same != null for children as well.

Thanks @WesleyAC for catching this!

@gnprice gnprice added P1 high-priority severe: crash The app quits, or stops responding. labels Jun 15, 2021
@WesleyAC WesleyAC force-pushed the rawlabel-empty-string branch from a760a73 to d79819c Compare June 15, 2021 21:37
@WesleyAC
Copy link
Copy Markdown
Contributor Author

Revision pushed :)

This was causing a crash when trying to autocomplete a user group with a
blank name. The empty string was passed into RawLabel, which caused this
invariant to incorrectly trigger.

This appears to have been introduced in 199a190.

Instead of using `!!` to check for undefined, we can do `!= null`, which
will trigger for undefined and null, but not an empty string.
@gnprice gnprice force-pushed the rawlabel-empty-string branch from d79819c to de91779 Compare June 15, 2021 21:46
@gnprice
Copy link
Copy Markdown
Member

gnprice commented Jun 15, 2021

Looks good, thanks -- merged!

@gnprice gnprice merged commit de91779 into zulip:master Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 high-priority severe: crash The app quits, or stops responding.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants