Skip to content

Commit

Permalink
Fix UI freeze after swiping back from profile (#1449)
Browse files Browse the repository at this point in the history
On iOS 17.0, swiping back from a view that uses
`.navigationBarHidden(true)` caused the `NavigationStack` view to
freeze. This fixes the issue by creating a custom toolbar using
`.toolbar` instead.

Issue reproduction steps
------------------------

I found a very good clue to reproduce this issue from
[https://damus.io/note162rt4fctxepnj9cdtr9a82k7jtw3e33hj742ejly3q84tkwsfars4k9glr](https://damus.io/note162rt4fctxepnj9cdtr9a82k7jtw3e33hj742ejly3q84tkwsfars4k9glr)

**Device:** iPhone 13 mini (Physical device)
**iOS:** 17.0.1
**Damus:** 1.6 (18) 4377cf2
**Steps:**
1. Go to the home timeline view.
2. Click on a profile on any post
3. Swipe back to the home timeline view (Do not press "back" button)
4. Click on that same profile again

**Ideal behaviour:** On step 4, you should be taken to the profile view
**Actual behaviour:** The whole timeline view, top bar, etc seems to "freeze" and no longer respond.

Root causing investigation
--------------------------

I attempted various things until I could narrow it down. Here is a
summary of what I discovered:
1. First I attempted to investigate where the deadlock would live, by
   analyzing the thread states in the debugger. However:
    1. I did not find much differences between the thread states of a
       normal app running and the app running after the issue
    2. **I noticed that the tab bar at the bottom was still working, so
       unless those views are running on different threads, it might not
       have been a deadlock**
    3. NostrDB ingested and writer threads seemed to be waiting on a
       mutex most of the time I paused execution, but that also happened
       under normal conditions
    4. The crux of what made this difficult is that most of the UI
       related threads were in assembly, which was harder to interpret.
       However, the top of the stack in those threads were usually
       `mach_msg_trap`, which I believe is just the debugger
       interrupting execution. Below it, there were usually normal
       assembly instructions being run, such as `mov` and `ldp`
       instructions _(Move value and load a pair of registers)_, and
       stepping through some of those seemed to move the program
       counter. So I believe that the threads are running
    5. Running `thread info` on some of the threads (e.g. main) revealed
       that it seemed to be waiting on `mach_msg2_trap`, which again I
       believe is just the debugger pausing execution.
2. **After some more testing, I realized that swiping back only breaks
   when swiping back from the `ProfileView`, but not other views**
2. I tried to check if the issue was incorrect hashing of `Router`
   objects: `NavigationStack` uses `NavigationPath`, which needs a
   collection of hashable elements. I thought that if hashing was done
   incorrectly, the NavigationStack might have issues managing views.
   But that did not seem to be it either.
    1. I tried experimenting with the hashing logic for the Profile
       router. No changes
    2. I tried purposefully messing up with the hashing logic of a good,
       working view by adding random numbers into the hash. No issues on
       swiping out of that view either.
3. That lead me to the possibility that the issue is within the
   `ProfileView` body. I commented parts of the code out and tested each
   portion of it in a binary search fashion, and narrowed it down a
   specific line:

```
.navigationBarHidden(true)
```

Whenever I remove this line or set this to `false`, the freezing no
longer occurs. According to the Apple developer docs, this is
deprecated:
[https://developer.apple.com/documentation/swiftui/view/navigationbarhidden(_:)](https://developer.apple.com/documentation/swiftui/view/navigationbarhidden(_:))

I tried to replace it with its newer replacement: `.toolbar(.hidden,
for: .automatic)`, but that also causes the freeze.

So, just removing that line fixes the freeze, however it breaks the
layout by showing the unwanted back button.

Fix
---

I was able to fix it by implementing the custom toolbar under `.toolbar`
modifier, and hiding the back button (as opposed to the whole nav bar)

Testing of the fix
------------------

**PASS**

**Device:** iPhone 13 mini (Physical device)
**iOS:** iOS 17.0.1
**Damus:** This commit

**Special remarks:** Some entitlements removed locally to be able to
build to device without access to development certificate

**Test steps:** Same as reproduction steps

**Results:** Swiping back from profile does not cause any issues. View
layout of the custom navbar is unaltered in appearance.

iOS 16 smoke test
-----------------

**PASS**

**Device:** iPhone 14 Pro simulator
**iOS:** 16.4
**Damus:** This commit
**Special remarks:** Same as test above

**Test steps:** Same as reproduction steps. However here we are not
checking the freezing (as it was not reproducible in the simulator). We
are checking that the changes did not break navigation, nor layout, nor
caused any build issues.

**Results:** Working as expected

Closes: #1449
Changelog-Fixed: Fix UI freeze after swiping back from profile (#1449)
Signed-off-by: Daniel D’Aquino <[email protected]>
Signed-off-by: William Casarin <[email protected]>
  • Loading branch information
danieldaquino authored and jb55 committed Sep 23, 2023
1 parent 01c6e3e commit 6d055be
Showing 1 changed file with 6 additions and 3 deletions.
9 changes: 6 additions & 3 deletions damus/Views/Profile/ProfileView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,6 @@ struct ProfileView: View {
navActionSheetButton
}
.padding(.top, 5)
.padding(.horizontal)
.accentColor(DamusColors.white)
}

Expand Down Expand Up @@ -471,8 +470,12 @@ struct ProfileView: View {
}
.ignoresSafeArea()
.navigationTitle("")
.navigationBarHidden(true)
.overlay(customNavbar, alignment: .top)
.navigationBarBackButtonHidden()
.toolbar {
ToolbarItem(placement: .principal) {
customNavbar
}
}
.onReceive(handle_notify(.switched_timeline)) { _ in
dismiss()
}
Expand Down

0 comments on commit 6d055be

Please sign in to comment.