Fix misleading/scary stack trace shown when an assertion occurs during node measurement #1022
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rement - Currently, there is a pair of mutex unlock and unlock that wraps around `-_u_measureNodeWithBoundsIfNecessary:` in `__layout`. That is because this method must be called without the lock. - When an assertion occurs within that method, the runtime bails early without reacquire the lock (so the lock is free now). However, the runtime then hits the end of the outmost mutex locker scope and tries to release the lock that it no longer holds, causing another assertion in ASThread to be shown to user (TextureGroup#932). This makes it extremely hard to idenfity the root assertion. - Fix by replacing the unlock/lock pair with a mutex unlocker.
maicki
approved these changes
Jul 12, 2018
Adlai-Holler
approved these changes
Jul 12, 2018
nguyenhuy
added a commit
to nguyenhuy/Texture
that referenced
this pull request
Jul 12, 2018
- Rename `ASDisplayNodeAssertLockUnownedByCurrentThread` to `ASDisplayNodeAssertLockNotHeld`, and `ASDisplayNodeAssertLockOwnedByCurrentThread` to `ASDisplayNodeAssertLockHeld` -> shorter and hopefully easier to distinguish between the two. - Add assertions to `_locked_` and `_u_` (i.e "unlocked") methods. - Turn `CHECK_LOCKING_SAFETY` flag on by default. After TextureGroup#1022 and TextureGroup#1023, we're in a good shape to actually enforce locked/unlocked requirements of internal methods. Our test suite passed, and we'll test more at Pinterest after the sync this week. - Fix ASVideoNode to avoid calling `play` while holding the lock. That method inserts a subnode and must be called lock free. - Other minor changes.
nguyenhuy
added a commit
that referenced
this pull request
Jul 13, 2018
- Rename `ASDisplayNodeAssertLockUnownedByCurrentThread` to `ASAssertUnlocked`, and `ASDisplayNodeAssertLockOwnedByCurrentThread` to `ASAssertLocked` -> shorter and hopefully easier to distinguish between the two. - Add assertions to `_locked_` and `_u_` (i.e "unlocked") methods. - Turn `CHECK_LOCKING_SAFETY` flag on by default. After #1022 and #1023, we're in a good shape to actually enforce locked/unlocked requirements of internal methods. Our test suite passed, and we'll test more at Pinterest after the sync this week. - Fix ASVideoNode to avoid calling `play` while holding the lock. That method inserts a subnode and must be called lock free. - Simplify `_loaded(node)` to only nil-check `_layer` because regardless of whether the node is view or layer backed, the layer should always be set if loaded. Use it throughout. - Other minor changes.
mikezucc
pushed a commit
to mikezucc/Texture
that referenced
this pull request
Oct 2, 2018
…g node measurement (TextureGroup#1022) - Currently, there is a pair of mutex unlock and unlock that wraps around `-_u_measureNodeWithBoundsIfNecessary:` in `__layout`. That is because this method must be called without the lock. - When an assertion occurs within that method, the runtime bails early without reacquire the lock (so the lock is free now). However, the runtime then hits the end of the outmost mutex locker scope and tries to release the lock that it no longer holds, causing another assertion in ASThread to be shown to user (TextureGroup#932). This makes it extremely hard to idenfity the root assertion. - Fix by replacing the unlock/lock pair with a mutex unlocker.
mikezucc
pushed a commit
to mikezucc/Texture
that referenced
this pull request
Oct 2, 2018
- Rename `ASDisplayNodeAssertLockUnownedByCurrentThread` to `ASAssertUnlocked`, and `ASDisplayNodeAssertLockOwnedByCurrentThread` to `ASAssertLocked` -> shorter and hopefully easier to distinguish between the two. - Add assertions to `_locked_` and `_u_` (i.e "unlocked") methods. - Turn `CHECK_LOCKING_SAFETY` flag on by default. After TextureGroup#1022 and TextureGroup#1023, we're in a good shape to actually enforce locked/unlocked requirements of internal methods. Our test suite passed, and we'll test more at Pinterest after the sync this week. - Fix ASVideoNode to avoid calling `play` while holding the lock. That method inserts a subnode and must be called lock free. - Simplify `_loaded(node)` to only nil-check `_layer` because regardless of whether the node is view or layer backed, the layer should always be set if loaded. Use it throughout. - Other minor changes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
-_u_measureNodeWithBoundsIfNecessary:in__layout. That is because this method must be called without the lock.Closes #932