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

Android - Workspace - App crashes when tapping on workspace share link as other user #55195

Open
2 of 8 tasks
IuliiaHerets opened this issue Jan 14, 2025 · 15 comments
Open
2 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Overdue

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Jan 14, 2025

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.84-5
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes, reproducible on both
If this was caught during regression testing, add the test name, ID and link from TestRail: Exp
Email or phone of affected tester (no customers): Samsung Galaxy Z Fold 4 / Android 14
Issue reported by: Applause Internal Team
Device used: [email protected]
App Component: Other

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to workspace settings > Workspace profile.
  3. Tap Share.
  4. Tap Copy URL.
  5. Launch ND or hybrid app.
  6. Open any chat.
  7. Paste the link and remove staging from the link.
  8. Send the message.
  9. Tap on the link.

Expected Result:

App will not crash.

Actual Result:

App crashes.
When tapping on the link for the second time, it does not crash anymore.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6713710_1736843501983.Screen_Recording_20250114_153950_Device_care.1.mp4

Bug6713710_1736840668489!log.txt

View all open jobs on GitHub

@IuliiaHerets IuliiaHerets added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Jan 14, 2025
Copy link

melvin-bot bot commented Jan 14, 2025

Triggered auto assignment to @laurenreidexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added the Overdue label Jan 16, 2025
@laurenreidexpensify
Copy link
Contributor

@IuliiaHerets can you confirm that this issue is reproducible on prod? I see two steps in the link that are staging specific -

Go to staging.new.expensify.com
Paste the link and remove staging from the link.

If this is a staging specific issue and not relevant to prod we can close. Thanks

@melvin-bot melvin-bot bot removed the Overdue label Jan 16, 2025
@BartoszGrajdek
Copy link
Contributor

I'll triage this issue today! 👀

@NJ-2020
Copy link
Contributor

NJ-2020 commented Jan 18, 2025

Proposal

Please re-state the problem that we are trying to solve in this issue.

Android - Workspace - App crashes when tapping on workspace share link as other user

What is the root cause of that problem?

When we click the link inside the report action, the openLink function got invoked

<AnchorForCommentsOnly
href={attrHref}
// Unless otherwise specified open all links in
// a new window. On Desktop this means that we will
// skip the default Save As... download prompt
// and defer to whatever browser the user has.
// eslint-disable-next-line react/jsx-props-no-multi-spaces
target={htmlAttribs.target || '_blank'}
rel={htmlAttribs.rel || 'noopener noreferrer'}
style={[style, parentStyle, textDecorationLineStyle, styles.textUnderlinePositionUnder, styles.textDecorationSkipInkNone]}
key={key}
// Only pass the press handler for internal links. For public links or whitelisted internal links fallback to default link handling
onPress={internalNewExpensifyPath || internalExpensifyPath ? () => Link.openLink(attrHref, environmentURL, isAttachment) : undefined}

and we navigate to that route if the it's internal new Expensify path and has the same origin
if (internalNewExpensifyPath && hasSameOrigin) {
if (Session.isAnonymousUser() && !Session.canAnonymousUserAccessRoute(internalNewExpensifyPath)) {
Session.signOutAndRedirectToSignIn();
return;
}
Navigation.navigate(internalNewExpensifyPath as Route);

And it will navigate to WorkspaceJoinUserPage.tsx page, and navigateAfterJoinRequest will got invoked after the user has been invited to the workspace
MemberAction.inviteMemberToWorkspace(policyID, inviterEmail);
isJoinLinkUsed = true;
Navigation.isNavigationReady().then(() => {
if (isUnmounted.current) {
return;
}
navigateAfterJoinRequest();

And inside navigateAfterJoinRequest function we navigate to settings > workspace_settings
const navigateAfterJoinRequest = () => {
Navigation.goBack(undefined, false, true);
Navigation.navigate(ROUTES.SETTINGS);
Navigation.navigate(ROUTES.SETTINGS_WORKSPACES);
};

The root cause that causing this issue is that we're trying to perform multiple navigations in almost at the same time that causing the app to crash/exits, and also this issue only occurring in Android native only, this issue is not reproducible in IOS native, mWeb, web and desktop

What changes do you think we should make in order to solve the problem?

We need to wait until the navigation transitions complete first, we can wrap the navigateAfterJoinRequest with runAfterInteractions to ensure the interaction queue is idle

 Navigation.isNavigationReady().then(() => { 
     if (isUnmounted.current) { 
         return; 
     } 
     InteractionManager.runAfterInteractions(() => {
         navigateAfterJoinRequest(); 
     });
})

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

Copy link
Contributor

⚠️ @NJ-2020 Thanks for your proposal. Please update it to follow the proposal template, as proposals are only reviewed if they follow that format (note the mandatory sections).

@melvin-bot melvin-bot bot added the Overdue label Jan 20, 2025
Copy link

melvin-bot bot commented Jan 20, 2025

@laurenreidexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@BartoszGrajdek
Copy link
Contributor

I'll triage this issue today! 👀

I didn't manage to find the time for it in the end, but from the proposal above it seems like it's not a live markdown related issue, so it's fine 👍🏻

@NJ-2020
Copy link
Contributor

NJ-2020 commented Jan 20, 2025

@BartoszGrajdek Hi, I just do more investigation on this issue and seems this issue caused by this PR, here's demo when using version before this PR is merged

Screen.Recording.2025-01-19.at.12.54.37.mov

And seems the issue is coming from react-native-screens, here's the error log when the app is crash:

SurfaceMountingManager: Unhandled SoftException

SurfaceMountingManager: java.lang.IllegalStateException: addViewAt: cannot insert view [3620] into parent [3622]: View already has a parent: [3626]  Parent: Screen View: ReactViewGroup

SurfaceMountingManager: 	at com.facebook.react.fabric.mounting.SurfaceMountingManager.addViewAt(SourceFile:139)

SurfaceMountingManager: 	at com.facebook.react.fabric.mounting.mountitems.IntBufferBatchMountItem.execute(SourceFile:248)

SurfaceMountingManager: 	at com.facebook.react.fabric.mounting.MountItemDispatcher.executeOrEnqueue(SourceFile:54)

SurfaceMountingManager: 	at com.facebook.react.fabric.mounting.MountItemDispatcher.dispatchMountItems(SourceFile:46)

SurfaceMountingManager: 	at com.facebook.react.fabric.mounting.MountItemDispatcher.tryDispatchMountItems(SourceFile:35)


SurfaceMountingManager: 	at com.facebook.react.fabric.FabricUIManager$DispatchUIFrameCallback.doFrameGuarded(SourceFile:95)

SurfaceMountingManager: 	at com.facebook.react.fabric.GuardedFrameCallback.doFrame(SourceFile:1)

SurfaceMountingManager: 	at com.facebook.react.modules.core.ReactChoreographer.frameCallback$lambda$1(SourceFile:37)

SurfaceMountingManager: 	at com.facebook.react.modules.core.ReactChoreographer.a(SourceFile:1)

SurfaceMountingManager: 	at com.facebook.react.modules.core.a.doFrame(SourceFile:26)

SurfaceMountingManager: 	at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1229)

SurfaceMountingManager: 	at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1239)

SurfaceMountingManager: 	at android.view.Choreographer.doCallbacks(Choreographer.java:899)

SurfaceMountingManager: 	at android.view.Choreographer.doFrame(Choreographer.java:827)

SurfaceMountingManager: 	at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:1214)


SurfaceMountingManager: 	at android.os.Handler.handleCallback(Handler.java:942)

SurfaceMountingManager: 	at android.os.Handler.dispatchMessage(Handler.java:99)

SurfaceMountingManager: 	at android.os.Looper.loopOnce(Looper.java:201)

SurfaceMountingManager: 	at android.os.Looper.loop(Looper.java:288)

SurfaceMountingManager: 	at android.app.ActivityThread.main(ActivityThread.java:7924)

SurfaceMountingManager: 	at java.lang.reflect.Method.invoke(Native Method)

SurfaceMountingManager: 	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)

SurfaceMountingManager: 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:936)

I think the error is coming specifically from this file and this file

And also I've opened an issue in react-native-screens here

@BartoszGrajdek
Copy link
Contributor

I'm react-native-live-markdown maintainer, not an internal/C+ so you don't need to worry about me 😄 @NJ-2020

This issue was initially sent to us as a possible react-native-live-markdown bug, but since that's not the case I'm no longer needed here 👍🏻

@NJ-2020

This comment has been minimized.

@NJ-2020
Copy link
Contributor

NJ-2020 commented Jan 20, 2025

After doing some investigation I found that:

  • The error that coming from react-native-live-markdown is not causing the app crashing, because I've fixed the error and the crash still happening
01-20 17:29:44.853  5496  5496 F libc  : /Expensify/node_modules/@expensify/react-native-live-markdown/cpp/MarkdownGlobal.cpp:40: std::shared_ptr<ShareableWorklet> expensify::livemarkdown::getMarkdownWorklet(const int): assertion "worklet != nullptr" failed
  • The app is crashing is caused by react-native-screens package and not for react-native-live-markdown because based on the error log
addViewAt: cannot insert view [3042] into parent [3044]: View already has a parent: [3048]  Parent: Screen View: ReactViewGroup

the issue occurs when trying to insert view into a parent view that already has a parent

@tomekzaw
Copy link
Contributor

I've fixed the error and the crash still happening

@NJ-2020 Could you please explain how you fixed this crash? MarkdownGlobal.cpp:40: std::shared_ptr<ShareableWorklet> expensify::livemarkdown::getMarkdownWorklet(const int): assertion "worklet != nullptr" failed

@NJ-2020
Copy link
Contributor

NJ-2020 commented Jan 20, 2025

@tomekzaw I fixed the issue by checking if the parserId exists or not in globalMarkdownShareableWorkletsMutex

std::shared_ptr<ShareableWorklet> getMarkdownWorklet(const int parserId) {
  std::unique_lock<std::mutex> lock(globalMarkdownShareableWorkletsMutex);
  auto it = globalMarkdownShareableWorklets.find(parserId);
  if (it == globalMarkdownShareableWorklets.end()) {
    // Return nullptr if parsedId does not exist
    return nullptr;
  }
  const auto &worklet = it->second;
  assert(worklet != nullptr);
  return worklet;
}

After doing this I can see the error is gone

@NJ-2020
Copy link
Contributor

NJ-2020 commented Jan 20, 2025

@tomekzaw The root cause of this issue is when the parser id is not exists in globalMarkdownShareableWorklets it will create new entry with nullptr value, but since in C++ if we use the assert function and pass false value as an argument the assert function will crash see here, for example:

// for example worklet value is nullptr
assert(worklet != nullptr); // it will crash see here https://stackoverflow.com/a/33908091

So when the worklet value is nullptr it will occur this error

MarkdownGlobal.cpp:40: std::shared_ptr<ShareableWorklet> expensify::livemarkdown::getMarkdownWorklet(const int): assertion "worklet != nullptr" failed

To solve this we can check first if the parser id exists or not

std::shared_ptr<ShareableWorklet> getMarkdownWorklet(const int parserId) {
  std::unique_lock<std::mutex> lock(globalMarkdownShareableWorkletsMutex);
  auto it = globalMarkdownShareableWorklets.find(parserId);
  if (it == globalMarkdownShareableWorklets.end()) {
    // Return nullptr if parserId does not exist
    return nullptr;
  }
  const auto &worklet = it->second;
  assert(worklet != nullptr);
  return worklet;
}

@tomekzaw
Copy link
Contributor

@NJ-2020 Thanks! But this also means that getMarkdownWorklet is called for some parserId before a parser with that ID is created. So basically getMarkdownWorklet(parserId) returns nullptr which is then passed to some other part of the code. I don't think that's safe, do you happen to know what happens next?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Overdue
Projects
None yet
Development

No branches or pull requests

5 participants