Page MenuHomePhabricator

[lib] Fix fatal error where empty code blocks crash
ClosedPublic

Authored by rohan on Dec 8 2022, 6:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 15, 1:56 AM
Unknown Object (File)
Sun, Dec 15, 1:56 AM
Unknown Object (File)
Sun, Dec 15, 1:56 AM
Unknown Object (File)
Sun, Dec 15, 1:54 AM
Unknown Object (File)
Sun, Dec 15, 1:29 AM
Unknown Object (File)
Nov 9 2024, 11:51 AM
Unknown Object (File)
Nov 9 2024, 11:51 AM
Unknown Object (File)
Nov 9 2024, 11:51 AM
Subscribers

Details

Summary

There's a pretty fatal error where an empty code block will
trigger the throw Error statement. I suspect that it's because the
object for the message is { type: 'codeBlock', content: '\n' }.

It makes sense to just return the node if we have nothing to traverse within.

https://linear.app/comm/issue/ENG-2438/unexpected-markdown-in-dec-9-daily-update-message-causing-native-app

Test Plan

Reproduced the message that causes the crash, alongside other more edge cases. Didn't experience a crash

Screenshot 2022-12-08 at 9.58.04 PM.png (206×608 px, 28 KB)

Screenshot 2022-12-08 at 9.58.15 PM.png (302×524 px, 48 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rohan requested review of this revision.Dec 8 2022, 7:03 PM

I'd like to better understand why this solves the issue. I won't block this as I'm about to go away but the current explanation doesn't make sense to me

lib/shared/markdown.js
267–269 ↗(On Diff #19264)

I suspect that it's because the object for the message is { type: 'codeBlock', content: '\n' }.

I'm confused, wouldn't that trigger this base case?

lib/shared/markdown.js
267–269 ↗(On Diff #19264)

Here's what I found looking into an empty codeBlock.

content is basically an empty string, so whilst typeof content === 'string is true, we're also checking if (content), and that is considered falsy in javascript. So it passes by the base case. We can see this by logging content on any iteration of the empty codeblock, and it's empty (similarly, the length of the string evaluates to 0).

Then, it obviously is not a spoiler, so we skip that check, and it similarly doesn't have any items. The check else if (content) on line 276 similarly evaluates to a falsy, so the error is therefore thrown.

267–269 ↗(On Diff #19264)

Here's a sample log I setup to double check this.

function replaceSpoilersFromMarkdownAST(node: SingleASTNode): SingleASTNode {
  const { content, items, type } = node;
  console.log('1. node: ', node);
  if (content) {
    console.log('We hae content.');
  } else {
    console.log('We do not have content.');
  }

  ............ (rest of the function)

the output I get is:

[METRO]  LOG  1. node:  {"content": "", "type": "codeBlock"}
[METRO]  LOG  We do not have content.
ashoat requested changes to this revision.Dec 13 2022, 7:17 AM
ashoat added inline comments.
lib/shared/markdown.js
267–269 ↗(On Diff #19264)

content is basically an empty string, so whilst typeof content === 'string is true, we're also checking if (content), and that is considered falsy in javascript.

This doesn't sound right to me. I just checked in the Chrome debugger console and here's what I see:

> !!'\n'
true
> !!"\n"
true

I also did some Googling and found this on [MDN}(https://developer.mozilla.org/en-US/docs/Glossary/Truthy):

That is, all values are *truthy* except false, 0, -0, 0n, "", null, undefined, and NaN.

Is it possible that your earlier claim that "the object for the message is { type: 'codeBlock', content: '\n' }" was incorrect? If so, please call this out explicitly. Let me know if I'm missing anything

This revision now requires changes to proceed.Dec 13 2022, 7:17 AM
lib/shared/markdown.js
267–269 ↗(On Diff #19264)

I used the following sample message below. I think (?) there may be a disparity between web and native.

Screenshot 2022-12-14 at 1.11.22 PM.png (222×566 px, 31 KB)

In the web console, this is what I see for that code block:

1. Node:  {type: 'codeBlock', content: '\n'}
We have content

Though in native, I'm seeing the following in my terminal

[METRO]  LOG  1. Node:  {"content": "", "type": "codeBlock"}
[METRO]  LOG  We don't have content

If this is the case, where web is detecting a \n, that would explain why I can open that problematic thread on the web version with no issues, but native is only seeing an empty string and therefore it reaches the error. Perhaps that's why the app crashes.

lib/shared/markdown.js
267 ↗(On Diff #19264)

Sounds like we should update this check instead so that it's a proper base case. Is this what we should have?

lib/shared/markdown.js
267 ↗(On Diff #19264)

Yeah, makes sense to update the base case. I'll do some more thorough testing again before putting up the revision. This should also catch "" and "\n"

Update the base case instead of removing the error

iOS

Simulator Screen Shot - iPhone 14 Pro - 2022-12-14 at 15.04.22.png (2×1 px, 214 KB)

Simulator Screen Shot - iPhone 14 Pro - 2022-12-14 at 15.07.59.png (2×1 px, 199 KB)

Android

Screenshot_1671048676.png (1×1 px, 94 KB)

Screenshot_1671048681.png (1×1 px, 93 KB)

Screenshot_1671048682.png (1×1 px, 85 KB)

ashoat removed 1 blocking reviewer(s): atul.
This revision is now accepted and ready to land.Dec 14 2022, 2:15 PM

@rohan can you create an issue for @atul to release a new version of the app to resolve this issue? Would be great to get this pushed to production before I land my upcoming React Native stack

@rohan can you create an issue for @atul to release a new version of the app to resolve this issue? Would be great to get this pushed to production before I land my upcoming React Native stack

Yeah! https://linear.app/comm/issue/ENG-2453/create-new-app-version-for-d5849