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

ios: crashing when opening the app with notifications allowed #5100

Closed
teidesu opened this issue Sep 3, 2024 · 17 comments · Fixed by #5475
Closed

ios: crashing when opening the app with notifications allowed #5100

teidesu opened this issue Sep 3, 2024 · 17 comments · Fixed by #5475
Assignees
Labels
bug Something isn't working

Comments

@teidesu
Copy link

teidesu commented Sep 3, 2024

Describe the bug
/subj. no extra actions needed, it crashes as soon as the app is opened when notifications are allowed

crash log: Bluesky-2024-09-03-040430.ips.json

To Reproduce

  • log into the app
  • allow notifications

Expected behavior

it doesn't crash

Details

  • Platform: ios
  • Platform version: iOS 18.0 (22A5350a)
  • App version: 1.90.1

Additional context

disallowing notifications in ios settings fixes the crash on startup, but it still crashes when opening the notifications tab

@teidesu teidesu added the bug Something isn't working label Sep 3, 2024
@teidesu teidesu changed the title ios: crashing when opening an app after logging in ios: crashing when opening the app with notifications allowed Sep 3, 2024
@haileyok
Copy link
Contributor

is this still happening to you? i am unable to repro

@teidesu
Copy link
Author

teidesu commented Sep 19, 2024

is this still happening to you? i am unable to repro

yep, still happening on iOS 18.1 (22B5045g), app 1.91.1.464 2feb5be.

screencast: https://github.com/user-attachments/assets/15b8bbf3-0292-4cb2-b4cb-1c3463b6a264

@haileyok
Copy link
Contributor

Are you able to scroll down the notifications at all on web to see if there's a weird post or anything?

@haileyok
Copy link
Contributor

Ah, never mind. I'm able to see some logs now, though this is quite bizarre. Need to figure out how to possible reproduce this, not entirely sure how yet... Will keep you posted.

@haileyok haileyok self-assigned this Sep 20, 2024
@Erisa
Copy link

Erisa commented Sep 23, 2024

This happens to me exactly as described on my account with handle @erisa.mraow.party (using my own PDS) but not on my account @erisa.uk (using bsky-provided PDS). Web works perfectly fine. Would be more than willing to provide any debug information or logs if you can share how I can do that!

edit to add: I had previously tried reinstalling the app from scratch and trying both accounts, it only fails if using the @erisa.mraow.party and with notifications on or when viewing notifications page without notifications on.

@teidesu
Copy link
Author

teidesu commented Sep 23, 2024

yep, im using my own pds as well. though i did test with a bsky.social account and it happened it there as well so i figured it's not that relevant

@haileyok
Copy link
Contributor

@Erisa @teidesu Yea, that's what I observed as well. I'm trying to get to the bottom of that, but there's pretty much nothing that stands out as to why it would be effecting only your own PDSes...Still digging at it though!!

@haileyok
Copy link
Contributor

haileyok commented Sep 24, 2024

The strange thing is that I'm running my own PDS as well and don't have any issue there. What PDS versions are you all running? (https://pds.haileyok.com/xrpc/_health will tell you)

It's definitely something with the registerPush endpoint. If either of you could use something like Proxyman to see what the response is for that request it would be super helpful!

https://proxyman.io/

@teidesu
Copy link
Author

teidesu commented Sep 24, 2024

What PDS versions are you all running?

0.4.51 for me

It's definitely something with the registerPush endpoint. If either of you could use something like Proxyman to see what the response is for that request it would be super helpful!

image

with notifications disabled i don't see a registerPush at all

with them enabled the app seems to crash before the registerPush returns anything. the request also seems to be fine?

image

i can give you an account on my pds if that might help :>

@haileyok
Copy link
Contributor

Yea, that could actually be useful if you don't mind.

@teidesu
Copy link
Author

teidesu commented Sep 24, 2024

dmed on bluesky

@haileyok
Copy link
Contributor

haileyok commented Sep 24, 2024

Well, I found the issue - at least, what's causing the crash. I'm not sure what exactly is going on aside from that yet.

Anyway, there are two endpoints that are causing the crash: registerPush and updateSeen. Both of these have an empty response body.

There's a check in https://github.com/facebook/react-native/blob/1288e38423f93ed57737dd9b40ad55696494d6f4/packages/react-native/Libraries/Blob/RCTFileReaderModule.mm#L76 for type (I have not dug closely yet to see where exactly it comes from yet) which for me is of NSString and equals text/plain. However, on your PDS type is of NSNull.

Note that the type != nil check won't do anything here because its NSNull rather than nil 🙃. Wondering if something might have changed in RCTConvert that ended up causing this, not quite certain yet. This seems like a weird RN bug in general anyway regardless of what else we find out...

Anyway, I'm also curious why type is different for the two of us (and I'm assuming also for @Erisa). Not sure if you have any ideas.

@haileyok
Copy link
Contributor

After more closely looking, it looks like there's a x-content-type-options nosniff header on the response from your PDS @teidesu. This is probably what's tripping up ^

@Erisa, I'd be curious if you see the same header in your response. I'm going to patch/PR a fix for the RN crash itself, but also trying to figure out where that header is coming from since it shouldn't be there for these responses.

@teidesu
Copy link
Author

teidesu commented Sep 24, 2024

trying to figure out where that header is coming from since it shouldn't be there for these responses

i believe it's being added by cloudflare, since accessing the reverse proxy directly doesn't return that header:
image

@haileyok
Copy link
Contributor

Interesting! Well, if you figure out how to remove that header in the short term then that should fix the problem. Otherwise, I'm going to write a fix and it'll come out in the next release. Thanks for all your help in debugging this!

@Erisa
Copy link

Erisa commented Sep 24, 2024

I have no idea why Cloudflare adds that header, but this works:
image

on https://dash.cloudflare.com/?to=/:account/:zone/rules/transform-rules/modify-response-header

@haileyok
Copy link
Contributor

Also fixing this in #5475. Will be a couple days at least before it makes it out. Seriously appreciate all your help in getting this debugged.

@teidesu You can go ahead and delete that test account or whatever. Shouldn't need it anymore 🙏 (I can actually repro myself now by adding this header 😆)

facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Sep 25, 2024
…46635)

Summary:
This issue original arose out of bluesky-social/social-app#5100. Copying the description (with my general understanding of the problem) from the patch PR to here as well.

There's a crash that comes up in the following, pretty specific scenario:

- Have a response that has an empty body
- Do not include a `content-type` header in the response
- Set the `x-content-type-options` header to `nosniff`

RN handles the response for a request in this block of code: https://github.com/facebook/react-native/blob/303e0ed7641409acf2d852c077f6be426afd7a0c/packages/react-native/Libraries/Blob/RCTBlobManager.mm#L314-L326

Here, we see that values of `nil` - which `[response MIMEType]` will return when no `content-type` is provided in the response and the actual type cannot be determined (https://developer.apple.com/documentation/foundation/nsurlresponse/1411613-mimetype) - gets converted to `NSNull` by `RCTNullIfNil`.

When we get back over to `readAsDataURL`, we see that we grab the type from the dictionary and check if its `nil` before calling `length` on the string. https://github.com/facebook/react-native/blob/303e0ed7641409acf2d852c077f6be426afd7a0c/packages/react-native/Libraries/Blob/RCTFileReaderModule.mm#L74-L77

However, this check is dubious, because the value will never actually be `nil`. It will always either be `NSString` or `NSNull` because of the `RCTNullIfNil` call made above and `[RCTConvert NSString]` seems to just return the input if it is `NSNull`.

## Changelog:

[IOS] [FIXED] - Convert `NSNull` to `nil` before checking `type` in `readAsDataURL`

Pull Request resolved: #46635

Test Plan:
This is a little awkward to test, but essentially this comes up in the following scenario that is described (and "tested" as being fixed by tweaking) in bluesky-social/social-app#5100. I have personally tested by using Cloudflare rules to add/remove that particular header from an empty body response. You could also test this with a little local web server if you want.

### Before

https://github.com/user-attachments/assets/deb86c68-2251-4fef-9705-a1c93584e83e

### After

https://github.com/user-attachments/assets/9ffab11b-b2c8-4a83-afd6-0a55fed3ae9b

Reviewed By: dmytrorykun

Differential Revision: D63381947

Pulled By: cipolleschi

fbshipit-source-id: b2b4944d998133611592eed8d112faa6195587bd
blakef pushed a commit to facebook/react-native that referenced this issue Sep 30, 2024
…46635)

Summary:
This issue original arose out of bluesky-social/social-app#5100. Copying the description (with my general understanding of the problem) from the patch PR to here as well.

There's a crash that comes up in the following, pretty specific scenario:

- Have a response that has an empty body
- Do not include a `content-type` header in the response
- Set the `x-content-type-options` header to `nosniff`

RN handles the response for a request in this block of code: https://github.com/facebook/react-native/blob/303e0ed7641409acf2d852c077f6be426afd7a0c/packages/react-native/Libraries/Blob/RCTBlobManager.mm#L314-L326

Here, we see that values of `nil` - which `[response MIMEType]` will return when no `content-type` is provided in the response and the actual type cannot be determined (https://developer.apple.com/documentation/foundation/nsurlresponse/1411613-mimetype) - gets converted to `NSNull` by `RCTNullIfNil`.

When we get back over to `readAsDataURL`, we see that we grab the type from the dictionary and check if its `nil` before calling `length` on the string. https://github.com/facebook/react-native/blob/303e0ed7641409acf2d852c077f6be426afd7a0c/packages/react-native/Libraries/Blob/RCTFileReaderModule.mm#L74-L77

However, this check is dubious, because the value will never actually be `nil`. It will always either be `NSString` or `NSNull` because of the `RCTNullIfNil` call made above and `[RCTConvert NSString]` seems to just return the input if it is `NSNull`.

## Changelog:

[IOS] [FIXED] - Convert `NSNull` to `nil` before checking `type` in `readAsDataURL`

Pull Request resolved: #46635

Test Plan:
This is a little awkward to test, but essentially this comes up in the following scenario that is described (and "tested" as being fixed by tweaking) in bluesky-social/social-app#5100. I have personally tested by using Cloudflare rules to add/remove that particular header from an empty body response. You could also test this with a little local web server if you want.

### Before

https://github.com/user-attachments/assets/deb86c68-2251-4fef-9705-a1c93584e83e

### After

https://github.com/user-attachments/assets/9ffab11b-b2c8-4a83-afd6-0a55fed3ae9b

Reviewed By: dmytrorykun

Differential Revision: D63381947

Pulled By: cipolleschi

fbshipit-source-id: b2b4944d998133611592eed8d112faa6195587bd
blakef pushed a commit to facebook/react-native that referenced this issue Sep 30, 2024
…46635)

Summary:
This issue original arose out of bluesky-social/social-app#5100. Copying the description (with my general understanding of the problem) from the patch PR to here as well.

There's a crash that comes up in the following, pretty specific scenario:

- Have a response that has an empty body
- Do not include a `content-type` header in the response
- Set the `x-content-type-options` header to `nosniff`

RN handles the response for a request in this block of code: https://github.com/facebook/react-native/blob/303e0ed7641409acf2d852c077f6be426afd7a0c/packages/react-native/Libraries/Blob/RCTBlobManager.mm#L314-L326

Here, we see that values of `nil` - which `[response MIMEType]` will return when no `content-type` is provided in the response and the actual type cannot be determined (https://developer.apple.com/documentation/foundation/nsurlresponse/1411613-mimetype) - gets converted to `NSNull` by `RCTNullIfNil`.

When we get back over to `readAsDataURL`, we see that we grab the type from the dictionary and check if its `nil` before calling `length` on the string. https://github.com/facebook/react-native/blob/303e0ed7641409acf2d852c077f6be426afd7a0c/packages/react-native/Libraries/Blob/RCTFileReaderModule.mm#L74-L77

However, this check is dubious, because the value will never actually be `nil`. It will always either be `NSString` or `NSNull` because of the `RCTNullIfNil` call made above and `[RCTConvert NSString]` seems to just return the input if it is `NSNull`.

## Changelog:

[IOS] [FIXED] - Convert `NSNull` to `nil` before checking `type` in `readAsDataURL`

Pull Request resolved: #46635

Test Plan:
This is a little awkward to test, but essentially this comes up in the following scenario that is described (and "tested" as being fixed by tweaking) in bluesky-social/social-app#5100. I have personally tested by using Cloudflare rules to add/remove that particular header from an empty body response. You could also test this with a little local web server if you want.

### Before

https://github.com/user-attachments/assets/deb86c68-2251-4fef-9705-a1c93584e83e

### After

https://github.com/user-attachments/assets/9ffab11b-b2c8-4a83-afd6-0a55fed3ae9b

Reviewed By: dmytrorykun

Differential Revision: D63381947

Pulled By: cipolleschi

fbshipit-source-id: b2b4944d998133611592eed8d112faa6195587bd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants