[HOLD for Payment 10/8] [$250] [Crashlytics] TypeError: undefined is not a function

[CortneyOfstad]

Coming from this GH — https://github.com/Expensify/App/issues/45054#issuecomment-2238548276

Reported by @TMisiukiewicz

Fatal Exception: com.facebook.react.common.JavascriptException: TypeError: undefined is not a function, js engine: hermes, stack:
init@1:2323334
_default@1:7673375
anonymous@1:180908
loadModuleImplementation@1:170989
guardedLoadModule@1:170179
metroRequire@1:169506
global@1:168441

       at com.facebook.react.modules.core.ExceptionsManagerModule.reportException(ExceptionsManagerModule.java:65)
       at com.facebook.jni.NativeRunnable.run(NativeRunnable.java)
       at android.os.Handler.handleCallback(Handler.java:958)
       at android.os.Handler.dispatchMessage(Handler.java:99)
       at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:29)
       at android.os.Looper.loopOnce(Looper.java:230)
       at android.os.Looper.loop(Looper.java:319)
       at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:234)
       at java.lang.Thread.run(Thread.java:1012)

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0147e5efa631c195d0
• Upwork Job ID: 1829521509135781835
• Last Price Increase: 2024-08-30

Issue Owner

Current Issue Owner: @paultsimura

[melvin-bot[bot]]

Current assignee @CortneyOfstad is eligible for the Bug assigner, not assigning anyone new.

[melvin-bot[bot]]

@CortneyOfstad Huh... This is 4 days overdue. Who can take care of this?

[CortneyOfstad]

@TMisiukiewicz it was indicated on one of the other Crashlytic GHs that the logs are truncated. Any way you could pull up the full logs? Thanks!

[melvin-bot[bot]]

@CortneyOfstad Eep! 4 days overdue now. Issues have feelings too...

[TMisiukiewicz]

can we change it to weekly?

[melvin-bot[bot]]

@CortneyOfstad this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot[bot]]

@CortneyOfstad Still overdue 6 days?! Let's take care of this!

[melvin-bot[bot]]

@CortneyOfstad 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[CortneyOfstad]

Sorry, was OoO, so adjusting the frequency now!

[hurali97]

Hey @CortneyOfstad 👋

To reproduce this I went through the Logs and Breadcrumbs section here in firebase crashlytics but I couldn't reproduce it. Also, for another similar event the logs and breadcrumbs were different, so I am not sure whether following are a correct reproduction steps but I am writing these from the link above:

• Create Submit Expense
• Set distance
• Select participants OR workspace
• Submit Expense
• Open the generated expense and view the details

After the last step, we get an error in firebase crashlytics. I tried it but I couldn't reproduce it.

---

Now, let's try to find the root cause in the code using the help from the stack trace. The function which is causing this error is getExtension which is a function in Str.ts in expensify-common. I opted to test this by isolating this function and below are the results:

We get an error url.split is not a function, which is similar to the error we get in crashlytics. This error only happens if the passed url is of another type than string, precisely it's of type number or array. On any other type like null or undefined, we get a different error.

To fix this, we can add a typeof operator in getExtension function, which will early return if url is not of string type. Below is the diff of suggested changes:

diff --git a/lib/str.ts b/lib/str.ts
index e1dd37c..432f8ed 100644
--- a/lib/str.ts
+++ b/lib/str.ts
@@ -992,6 +992,7 @@ const Str = {
      * without query parameters
      */
     getExtension(url: string): string | undefined {
+        if (typeof url !== 'string') return undefined;
         return url.split('.').pop()?.split('?')[0]?.toLowerCase();
     },

[CortneyOfstad]

Thanks for the detailed explanation @hurali97!

@TMisiukiewicz reviewing the comment above, there may potentially be an issue in the breadcrumb flow from the original issue that this was created from. Do you have any insight into what could potentially be missing or needs to be adjusted in order to recreate?

[TMisiukiewicz]

@CortneyOfstad currently breadcrumbs are tied only to navigation events only, and this, unfortunately, it gaves us very limited knowledge about users actions. When I have a bit more capacity, I am planning to review if there is a space for improvements in this area.

Also, the proposal from @hurali97 looks good to me 👍

[hurali97]

@CortneyOfstad do you think we can move forward with creating a PR to fix this?

[CortneyOfstad]

Sorry for the delay here — PR looks great! Thank you!

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~0147e5efa631c195d0

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @paultsimura (External)

[CortneyOfstad]

@paultsimura proposed solution is here

[paultsimura]

The explanation and the solution in the proposal look good to me.

🎀👀🎀 C+ reviewed

I'm a little uncertain about the testing method for this issue though – do we just create a PR inside expensify-common and add unit tests to cover the failure scenarios? Or are there specific steps to reproduce the original error?

[melvin-bot[bot]]

Triggered auto assignment to @francoisl, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[francoisl]

The explanation makes sense, but since the url param is already defined as a string in the TS function signature getExtension(url: string), wouldn't adding a check on typeof url be redundant?

[hurali97]

The explanation makes sense, but since the url param is already defined as a string in the TS function signature getExtension(url: string), wouldn't adding a check on typeof url be redundant?

@francoisl That's true but it will only take effect in debug builds. On release builds, TS is stripped and we only have plain JS, so there could be anything passed to getExtension. Since we are not aware of the reproduction steps, we can't test this locally and see what value is being passed to it and why.

Since typeof is a JS operator it will be present in the final JS build and guarantees that the no type other than string should fall through.

[hurali97]

The explanation and the solution in the proposal look good to me.

🎀👀🎀 C+ reviewed

I'm a little uncertain about the testing method for this issue though – do we just create a PR inside expensify-common and add unit tests to cover the failure scenarios? Or are there specific steps to reproduce the original error?

@paultsimura unfortunately, we don't have any reproduction steps. I agree that to give us more confidence we can add unit tests.

I will create a PR shortly and add unit tests for the implementation.

[iwiznia]

I got the PR to review and have the same concern as @francoisl. This should not be needed and should be caught by TS.

I see you say we don't have reproduction steps, but we do have some trace, can't we use that to figure out where this is coming from and why it is not being caught by TS?

[melvin-bot[bot]]

@francoisl, @CortneyOfstad, @paultsimura, @hurali97 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[paultsimura]

Doesn't look overdue to me 👀

[hurali97]

I got the PR to review and have the same concern as @francoisl. This should not be needed and should be caught by TS.

I see you say we don't have reproduction steps, but we do have some trace, can't we use that to figure out where this is coming from and why it is not being caught by TS?

@iwiznia I agree and I provided an explanation here. TS will only help us in debug environments, so it won't catch anything on release.

As for the trace, they don't appear to be useful in order to further narrow down this issue. Only thing I was able to deduce was that we have calls to isImage and isPDF, which internally calls getExtension. The former functions are invoked in getThumbnailAndImageURIs and MoneyRequestConfirmationListFooter respectively and they deduce the fileName from transaction object.

I expect transaction object to come from Onyx and it could be a reason of stale onyx data or some other thing. I can not dive further on how it is possible to receive anything other than the string or undefined here, because of lack of reproduction data.

We can argue that we can add additional checks in these two places instead of adding one in getExtension but it won't scale. If in future, we have crashes in other places we will need to add checks in those places as well.

[iwiznia]

TS will only help us in debug environments, so it won't catch anything on release.

The whole idea of TS is that it provides "compile" time checks. Why is it not catching this bad call then?

I'd be much more on board if we:

• Deployed a change that would log when the bad call is made (and avoid crashing)
• Wait for the line to get logged
• Figure out why TS was not catching it as it should and fix that
• Revert the PR in step 1
• Close this issue

[hurali97]

@iwiznia Since you're already saying that "TS provides compile time checks", so it won't help us in runtime which means you can pass any data to it.

Why is it not catching this bad call then?

Because in compile time, we are not invoking getExtension with bad data.

If you want, we can add a log of what data are we passing to getExtension, only in the case if it's type is not string | undefined. Also, if we go down this path, I think it won't be okay to add a firebase bread crumb from this function, so as to keep it decoupled. So this would mean to add a log wherever, getExtension is called from isPDF and isImage, that's a total of 19 places in the App codebase, so a lot of redundancy.

Anyways, Let me know what you think of the above.

---

From my experience, most of the open-source libraries use additional runtime checks in order to ensure safety, where there's a chance of type ambiguity or a union type. Also, I think the cost of doing above is more than adding a runtime check.

[iwiznia]

Because in compile time, we are not invoking getExtension with bad data.

I am confused, nothing is really invoked at compile time.

@francoisl curious for your thoughts about the PR...

[paultsimura]

I am confused, nothing is really invoked at compile time.

@iwiznia TS is aimed to handle the following cases:

function doSmth(param: string) { ... };

const num = 123;
doSmth(num);

TS will show you the following error both in IDE and during compilation, because it knows the type of num, and that it isn't what doSmth expects:

Argument of type number is not assignable to parameter of type string

However, there are cases when the passed parameter's type is not clearly known (e.g. it comes from some file parsing library). We assume it's a string, and we pass it as such into the function, and the TS compiler doesn't complain. But in reality, it may not be a string, so the explicit type check is useful here.

[iwiznia]

However, there are cases when the passed parameter's type is not clearly known (e.g. it comes from some file parsing library). We assume it's a string, and we pass it as such into the function, and the TS compiler doesn't complain.

But even in this case, wherever that parsing call is done the variables should be typed and thus should still get the error, no? If everything is ts and has proper types, the bug should be caught by ts somewhere.

[paultsimura]

In a perfect world - yes. But IIRC, there was a bug sometime ago where we'd expect the image URL to always be a string (which is natural), but iOS Native used numeric 1 for representing the URL of a locally stored image blob. This was quite unexpected and caused by a platform specificity. Maybe we're facing something similar here🤔

[francoisl]

Left a comment in the PR about the logs, but for now I'm not sure I understand the part about the firebase breadcrumb in this comment:

If you want, we can add a log of what data are we passing to getExtension, only in the case if it's type is not string | undefined. Also, if we go down this path, I think it won't be okay to add a firebase bread crumb from this function, so as to keep it decoupled. So this would mean to add a log wherever, getExtension is called from isPDF and isImage, that's a total of 19 places in the App codebase, so a lot of redundancy.

Would it be fine if we use Log.alert() with includeStackTrace: true directly in str.js? That way it's just one place to update, and we'd still get a stacktrace to get context about where the issue was from?

[melvin-bot[bot]]

@francoisl, @CortneyOfstad, @paultsimura, @hurali97 Huh... This is 4 days overdue. Who can take care of this?

[paultsimura]

There is an ongoing discussion in the PR

[melvin-bot[bot]]

@francoisl, @CortneyOfstad, @paultsimura, @hurali97 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[francoisl]

Oh, we still need a PR to bump the expensify-common version in package.json, @hurali97 do you mind sending one please?

[deetergp]

We are about to bump to 2.0.88 in this PR. Do you want us to wait, or just pull in your changes with ours?

[melvin-bot[bot]]

@francoisl, @CortneyOfstad, @paultsimura, @hurali97 Still overdue 6 days?! Let's take care of this!

[paultsimura]

@deetergp I think you can pull them in. There is nothing breaking here – just some extra precautions & logging.

[melvin-bot[bot]]

@francoisl, @CortneyOfstad, @paultsimura, @hurali97 Eep! 4 days overdue now. Issues have feelings too...

[francoisl]

Still waiting on https://github.com/Expensify/App/pull/49038 to be merged

[paultsimura]

Not overdue.

[melvin-bot[bot]]

@francoisl, @CortneyOfstad, @paultsimura, @hurali97 Eep! 4 days overdue now. Issues have feelings too...

[paultsimura]

@francoisl https://github.com/Expensify/App/pull/49038 was merged 4 days ago. What's the plan here now?

[francoisl]

Ah nice, and it was just deployed to production too.

I think we can issue payments and close this. With the new Log.warn, if the issue shows up again, we'll see it in our logs and an internal engineer will be assigned to investigate.

[paultsimura]

The corresponding version bump was deployed to production on Oct 1. Payment is due on 2024-10-08.

[paultsimura]

I don't think this GH is suitable for a BZ checklist: it is more like a preventive change with extra logging to detect and avoid previously uncaught crashes.

[CortneyOfstad]

@paultsimura — i sent you an offer in Upwork here. Please let me know once you accept, and I'll get that paid ASAP. Thanks!

[paultsimura]

Accepted, thanks @CortneyOfstad