[Text][Crash] RCTText setTextStorage crashes #2001
Comments
|
Hmmm - is this definitely new? I'm fairly sure I've seen similar stack trace errors in previous releases. |
|
Pretty sure this is new or the old bug is back. |
|
We reverted dynamic text size stuff internally, but for different reasons... |
|
With the Jul 15 sync that includes the revert I'm not seeing the crash anymore. |
|
@ide @brentvatne I'm still seeing this occasionally on reload, same result with zombies: |
|
I started seeing this again too, although I haven't encountered it yet after pulling in yesterday's sync. |
|
Oh, cool, we're about just on 0.11-rc, hopefully it's contained to just refreshes :) |
|
I should actually ask, have you seen this manifest anywhere other than during a refresh? |
|
Just during reloading so far. |
|
I hit it again :/ |
|
Can you take a look at this, Alex? |
|
Someone was telling me that CoreText may not actually be threadsafe in a couple cases. That is you can use it off the main thread but it might not support concurrency. In this case the crash looks related to deallocation so maybe the module is getting dealloc'd from a thread/queue other than its own? |
|
Couple more text-related crashes (this time from iOS 9): |
|
I saw this a bunch the other day when reloading the bridge, profiling,
|
|
How often does this happen? I just talked to @tadeuzagallo and he says it's a race condition when ARC releases the memory. He saw it happen very rarely when reloading JS so it's fairly hard to repro, we're not sure about the right fix yet. |
|
I hit this issue only when running the test suite. No mather how I run it, one component will always crash, not always the same. |
|
@finalquest Does that still happen when you run your tests against the latest version from master? 9b1f6c9 has made this error disappear for us when running tests. |
|
I will try to use master and report to this thread |
|
Here are some more detailed stack traces, though they don't really point to the issue. The environment is master (0.13-dev) iPhone 6 simulator, iOS 9.0, Xcode 7, ASan enabled. Can also repro with ASan disabled and with the Chrome debugger. I find it strange that an RCTText object (that I have a strong reference to) has a strong reference to an NSTextStorage object that is deallocated. -[NSConcreteTextStorage release]: message sent to deallocated instance 0x612000518440 Async stack trace across queues -[NSConcreteTextStorage release]: message sent to deallocated instance 0x612002ddaa40 |
|
@nicklockwood Have you encountered dealloc race conditions before? Thought you may have some insight on what's going on here and how to fix it. I read up on this class of race conditions and there's not a whole lot out there, but most of the discussion was around how weak references aren't zeroed out before deallocation. It didn't look like that was the case here since the problematic NSTextStorage is strongly referenced. Maybe there's a weak reference to the RCTText and that's cascading down to affect the deallocation of the NSTextStorage? There were also a post about NSNotificationCenter and dealloc (http://inessential.com/2013/12/20/observers_and_thread_safety) but that didn't seem relevant to this problem since neither RCTText nor NSTextStorage is subscribing to |
|
I dug in some more and potentially narrowed it down to the fact that we share the same cached NSTextStorage between RCTShadowText's RCTMeasure and the main thread (RCTText). It makes sense to me if that's the issue since NSTextStorage isn't thread-safe and I'll keep investigating. |
|
That's true, but we shouldn't ever be accessing the same instance concurrently. The sequence is:
The shadow thread never modifies the same text storage instance after first creating it, and the main thread never gets access to the storage until the shadow thread has finished with it. At least, that's the way I designed it. It's possible there's a bug, or I've got some misconception. |
|
The reason I thought that the shadow thread could access the NSTextStorage after it was passed to the main thread was because I saw the crash go away when I commented out the code that get/set |
…lityManager Summary: When you reload and create a new bridge, one of the things that happens during setup is that the RCTAccessibilityManager fires a notification. The old bridge would receive this notification from the new bridge's RCTAccessibilityManager, which we don't want, especially because the two are running on different shadow queues. I believe this led to a gnarly crash in NSConcreteTextStorage because RCTMeasure in RCTShadowText.m was getting called for the old RCTText (getting destroyed) from a notification fired from the new shadow queue. The fix is for the UIManager to handle notifications only from its bridge's RCTAccessibilityManager. See #2001 for the kinds of crashes we were seeing. Closes #3279 Reviewed By: @svcscm Differential Revision: D2521652 Pulled By: @nicklockwood fb-gh-sync-id: a4ffe3ef8304667727e573e2a2e8b716e1d2f3e1
…lityManager Summary: When you reload and create a new bridge, one of the things that happens during setup is that the RCTAccessibilityManager fires a notification. The old bridge would receive this notification from the new bridge's RCTAccessibilityManager, which we don't want, especially because the two are running on different shadow queues. I believe this led to a gnarly crash in NSConcreteTextStorage because RCTMeasure in RCTShadowText.m was getting called for the old RCTText (getting destroyed) from a notification fired from the new shadow queue. The fix is for the UIManager to handle notifications only from its bridge's RCTAccessibilityManager. See facebook#2001 for the kinds of crashes we were seeing. Closes facebook#3279 Reviewed By: @svcscm Differential Revision: D2521652 Pulled By: @nicklockwood fb-gh-sync-id: a4ffe3ef8304667727e573e2a2e8b716e1d2f3e1
|
Closing this since we haven't seen the crash after a couple of days of regular use with the merged diff. |
…lityManager Summary: When you reload and create a new bridge, one of the things that happens during setup is that the RCTAccessibilityManager fires a notification. The old bridge would receive this notification from the new bridge's RCTAccessibilityManager, which we don't want, especially because the two are running on different shadow queues. I believe this led to a gnarly crash in NSConcreteTextStorage because RCTMeasure in RCTShadowText.m was getting called for the old RCTText (getting destroyed) from a notification fired from the new shadow queue. The fix is for the UIManager to handle notifications only from its bridge's RCTAccessibilityManager. See facebook#2001 for the kinds of crashes we were seeing. Closes facebook#3279 Reviewed By: @svcscm Differential Revision: D2521652 Pulled By: @nicklockwood fb-gh-sync-id: a4ffe3ef8304667727e573e2a2e8b716e1d2f3e1
…lityManager Summary: When you reload and create a new bridge, one of the things that happens during setup is that the RCTAccessibilityManager fires a notification. The old bridge would receive this notification from the new bridge's RCTAccessibilityManager, which we don't want, especially because the two are running on different shadow queues. I believe this led to a gnarly crash in NSConcreteTextStorage because RCTMeasure in RCTShadowText.m was getting called for the old RCTText (getting destroyed) from a notification fired from the new shadow queue. The fix is for the UIManager to handle notifications only from its bridge's RCTAccessibilityManager. See facebook#2001 for the kinds of crashes we were seeing. Closes facebook#3279 Reviewed By: @svcscm Differential Revision: D2521652 Pulled By: @nicklockwood fb-gh-sync-id: a4ffe3ef8304667727e573e2a2e8b716e1d2f3e1
This is on master after the Tuesday sync. Perhaps the text scaling diff revealed or introduced this bug. I don't have a repro case (seems to happen on reload) but this is the stack trace:
With zombies I get
[NSConcreteTextStorage release]: message sent to deallocated instance 0x7f99ff5bddf0. Maybe a bug in Core Text or iOS 8.4?The text was updated successfully, but these errors were encountered: