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

Stats: Fix a crash in a remote service when accessing date formatter #749

Merged

Conversation

staskus
Copy link
Contributor

@staskus staskus commented Mar 11, 2024

Fixes wordpress-mobile/WordPress-iOS#22796

Description

A crash with stack trace [NSDateFormatter _regenerateFormatter] and KERN_INVALID_ADDRESS at 0xb3 suggests that the crash may have happened due to date formatter being accessed from different threads and/or objects being deinitialized when being accessed.

I couldn't find ways to reproduce the issue but I found two potential fixes:

  • Creating a new instance of periodDataQueryDateFormatter every time it's used to avoid the same DateFormatter being referenced from different threads. The same is done with ISO8601DateFormatter() in StatsServiceRemoteV2 line 260.
  • Using [weak sef] in wordPressComRestApi.GET closure that could've caused issues with memory.

Testing Details

I tried finding ways to force the crash but couldn't do it.


  • Please check here if your pull request includes additional test coverage.
  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

Sorry, something went wrong.

staskus added 2 commits March 11, 2024 17:46
…QueryDateFormatter

A crash with [NSDateFormatter _regenerateFormatter] and KERN_INVALID_ADDRESS at 0xb3. suggests that crash may have happened due to date formatter being accessed from different threads and/or object being deinitialized when being accessed. I couldn't find ways to reproduce the issue but I found two potential fixes:

- Creating a new instance of periodDataQueryDateFormatter every time it's used to avoid the same DateFormatter being referenced from different threads
- Using [weak sef] in wordPressComRestApi.GET closure that could've caused issues with memory
@staskus staskus added the bug Something isn't working label Mar 11, 2024
@staskus staskus marked this pull request as ready for review March 11, 2024 15:51
@staskus staskus requested a review from guarani March 11, 2024 16:13
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't reproduce this either, but I think the fix is the right one.

First, I tried changing device regions language while using Stats but couldn't get it to crash. Then I enabled Thread Sanitizer in Xcode and used Stats again and it detected two threads accessing the date formatter at the same time. It didn't detect immediately and I did turn off my network around the time it detected it, so that might have helped trigger it.

I tested with the changes and it wasn't reported again (as expected since now each thread will create a new date picker each time it needs one).

I'm not sure why this crash started happening now since the date formatter and caller hasn't been changed in years. It might have something to do with recent changes to Stats, but AFAIK it's always used multiple thread.

See report
==================
WARNING: ThreadSanitizer: data race (pid=86887)
  Read of size 8 at 0x0001157f09e0 by thread T118:
    #0 WordPressKit.StatsServiceRemoteV2.(periodDataQueryDateFormatter in _03BF0BDC8A6D90BA03FF4F2C19EB64E5).getter : __C.NSDateFormatter <null> (Jetpack:arm64+0x103c81880)
    #1 WordPressKit.StatsServiceRemoteV2.getData<A where A: WordPressKit.StatsTimeIntervalData>(for: WordPressKit.StatsPeriodUnit, unit: Swift.Optional<WordPressKit.StatsPeriodUnit>, endingOn: Foundation.Date, limit: Swift.Int, completion: (Swift.Optional<A>, Swift.Optional<Swift.Error>) -> ()) -> () <null> (Jetpack:arm64+0x103c8538c)
    #2 WordPress.StatsPeriodAsyncOperation.main() -> () <null> (Jetpack:arm64+0x101a19830)
    #3 @objc WordPress.StatsPeriodAsyncOperation.main() -> () <null> (Jetpack:arm64+0x101a19c0c)
    #4 WordPress.AsyncOperation.start() -> () <null> (Jetpack:arm64+0x100b11ea0)
    #5 @objc WordPress.AsyncOperation.start() -> () <null> (Jetpack:arm64+0x100b11f10)
    #6 __NSOPERATIONQUEUE_IS_STARTING_AN_OPERATION__ <null> (Foundation:arm64+0x6051f4)
    #7 _dispatch_client_callout <null> (libdispatch.dylib:arm64+0x5938)

  Previous write of size 8 at 0x0001157f09e0 by thread T114:
    #0 WordPressKit.StatsServiceRemoteV2.(periodDataQueryDateFormatter in _03BF0BDC8A6D90BA03FF4F2C19EB64E5).getter : __C.NSDateFormatter <null> (Jetpack:arm64+0x103c8193c)
    #1 WordPressKit.StatsServiceRemoteV2.getData<A where A: WordPressKit.StatsTimeIntervalData>(for: WordPressKit.StatsPeriodUnit, unit: Swift.Optional<WordPressKit.StatsPeriodUnit>, endingOn: Foundation.Date, limit: Swift.Int, completion: (Swift.Optional<A>, Swift.Optional<Swift.Error>) -> ()) -> () <null> (Jetpack:arm64+0x103c8538c)
    #2 WordPress.StatsPeriodAsyncOperation.main() -> () <null> (Jetpack:arm64+0x101a19830)
    #3 @objc WordPress.StatsPeriodAsyncOperation.main() -> () <null> (Jetpack:arm64+0x101a19c0c)
    #4 WordPress.AsyncOperation.start() -> () <null> (Jetpack:arm64+0x100b11ea0)
    #5 @objc WordPress.AsyncOperation.start() -> () <null> (Jetpack:arm64+0x100b11f10)
    #6 __NSOPERATIONQUEUE_IS_STARTING_AN_OPERATION__ <null> (Foundation:arm64+0x6051f4)
    #7 _dispatch_client_callout <null> (libdispatch.dylib:arm64+0x5938)

  Location is heap block of size 80 at 0x0001157f09b0 allocated by main thread:
    #0 calloc <null> (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x4fc30)
    #1 _malloc_type_calloc_outlined <null> (libsystem_malloc.dylib:arm64+0x1e980)
    #2 WordPress.StatsPeriodStore.(initializeStatsRemote in _CB70DAE5B2AB7CEDDC6B2B157E517E3B)() -> () <null> (Jetpack:arm64+0x101e008c0)
    #3 WordPress.StatsPeriodStore.(statsRemote in _CB70DAE5B2AB7CEDDC6B2B157E517E3B)() -> Swift.Optional<WordPressKit.StatsServiceRemoteV2> <null> (Jetpack:arm64+0x101de9c94)
    #4 WordPress.StatsPeriodStore.(fetchPeriodOverviewChartData in _CB70DAE5B2AB7CEDDC6B2B157E517E3B)(date: Foundation.Date, period: WordPressKit.StatsPeriodUnit, unit: WordPressKit.StatsPeriodUnit) -> () <null> (Jetpack:arm64+0x101df5ed8)
    #5 closure #1 () -> () in WordPress.StatsPeriodStore.(refreshPeriodOverviewData in _CB70DAE5B2AB7CEDDC6B2B157E517E3B)(date: Foundation.Date, period: WordPressKit.StatsPeriodUnit, forceRefresh: Swift.Bool) -> () <null> (Jetpack:arm64+0x101df5ce4)
    #6 partial apply forwarder for closure #1 () -> () in WordPress.StatsPeriodStore.(refreshPeriodOverviewData in _CB70DAE5B2AB7CEDDC6B2B157E517E3B)(date: Foundation.Date, period: WordPressKit.StatsPeriodUnit, forceRefresh: Swift.Bool) -> () <null> (Jetpack:arm64+0x101e13604)
    #7 closure #1 @Sendable () -> () in closure #1 () -> () in WordPress.Scheduler.(configureJob in _EA43D07FCEECC3BEC2FFA4B724966740)(callbackQueue: __C.OS_dispatch_queue, block: () -> ()) -> () <null> (Jetpack:arm64+0x102442850)
    #8 partial apply forwarder for closure #1 @Sendable () -> () in closure #1 () -> () in WordPress.Scheduler.(configureJob in _EA43D07FCEECC3BEC2FFA4B724966740)(callbackQueue: __C.OS_dispatch_queue, block: () -> ()) -> () <null> (Jetpack:arm64+0x102442e44)
    #9 reabstraction thunk helper from @escaping @callee_guaranteed @Sendable () -> () to @escaping @callee_unowned @convention(block) @Sendable () -> () <null> (Jetpack:arm64+0x1002c4048)
    #10 __tsan::invoke_and_release_block(void*) <null> (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x77ee0)
    #11 _dispatch_client_callout <null> (libdispatch.dylib:arm64+0x5938)
    #12 <null> <null> (dyld:arm64+0x1540)

  Thread T118 (tid=14134203, running) is a GCD worker thread

  Thread T114 (tid=14134199, running) is a GCD worker thread

SUMMARY: ThreadSanitizer: data race (/Users/paulvonschrottky/Library/Developer/CoreSimulator/Devices/C94676B6-9A5C-470E-B8D2-E5514F08A99A/data/Containers/Bundle/Application/981C7160-BE1C-4C7C-80BA-3BC38174E5CA/Jetpack.app/Jetpack:arm64+0x103c81880) in WordPressKit.StatsServiceRemoteV2.(periodDataQueryDateFormatter in _03BF0BDC8A6D90BA03FF4F2C19EB64E5).getter : __C.NSDateFormatter+0x70
==================

@staskus
Copy link
Contributor Author

staskus commented Mar 13, 2024

@guarani

Then I enabled Thread Sanitizer in Xcode and used Stats again and it detected two threads accessing the date formatter at the same time. It didn't detect immediately and I did turn off my network around the time it detected it, so that might have helped trigger it.

That's a good idea, thanks for the thorough testing!

It might have something to do with recent changes to Stats, but AFAIK it's always used multiple thread.

That's true and yes, I couldn't figure out what exactly changed to cause this. I made some updates to StatsPeriodStore but the changes that affect current Days/../Years tabs are quite limited. Lets see if these changes help, if my understanding was correct that it's related to multithreading and/or memory then the crashes shouldn't happen anymore.

@staskus staskus merged commit 3c370cb into trunk Mar 13, 2024
9 checks passed
@staskus staskus deleted the bug/22796-StatsServiceRemoteV2-objectForKey-period-crash branch March 13, 2024 08:26
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
Development

Successfully merging this pull request may close these issues.

Stats: Crash in remote service when accessing date formatter
2 participants