[ASPrimitiveTraitCollection] Always treat preferredContentSize as a potential nil #trivial by ypogribnyi · Pull Request #757 · TextureGroup/Texture

ypogribnyi · 2018-01-19T12:02:04Z

As ASPrimitiveTraitCollection is a struct, the application can always initialize it in its own way, without using Texture-provided initialization functions. This may lead to preferredContentSize field being nil, which, in turn, may lead to frustrating crashes when the struct is given to Texture.

While simply advising the user to always use the provided initialization functions should solve the problem, it's better to always treat this structure as possibly improperly initialized and guard against nil value in this field.

Additionally this PR explicitly marks all UIContentSizeCategory usage in ASTraitCollection class as non-null.

Add nil-value checks for preferredContentSizeCategory.

ay8s · 2018-01-19T17:23:39Z

It seems better now when changing device orientation @ypogribnyi. Is there documentation around initializing ASPrimitiveTraitCollection? Might be missing something.

We were seeing this on iOS 10 & 11.

ypogribnyi · 2018-01-22T11:07:25Z

The preferred way to create ASPrimitiveTraitCollection is to use ASPrimitiveTraitCollectionMakeDefault() or ASPrimitiveTraitCollectionFromUITraitCollection(UITraitCollection*). There wasn't any documentation about this, but I added a comment to ASPrimitiveTraitCollection definition in this PR (not sure if that's enough).

By the way, why do we expose two trait collections interfaces (ASPrimitiveTraitCollection and ASTraitCollection) to the user? Might it be better to hide one of them?

nguyenhuy · 2018-01-22T12:18:49Z

By the way, why do we expose two trait collections interfaces (ASPrimitiveTraitCollection and ASTraitCollection) to the user? Might it be better to hide one of them?

Good questions! Based on facebookarchive/AsyncDisplayKit#1592, I think the original reason is to have ASTraitCollection as a friendly object-based API and ASPrimitiveTraitCollection as the internal actual struct that is used throughout the framework. We should at least document this reasoning, and/or move ASPrimitiveTraitCollection to a framework-private interface.

nguyenhuy

@ypogribnyi Great reasoning on the possibility that ASPrimitiveTraitCollection may be initialized manually, and defend against such case.

@ay8s This PR should mitigate your crash, although I'd suggest to dig a bit deeper into your codebase and fix the root cause (initialize ASPrimitiveTraitCollection using the provided functions or better yet, switch to ASTraitCollection). Let us know what you find!

nguyenhuy · 2018-01-22T12:21:12Z

 #endif

-@property (nonatomic, assign, readonly) UIContentSizeCategory preferredContentSizeCategory;
+@property (nonatomic, assign, readonly) UIContentSizeCategory _Nonnull preferredContentSizeCategory;


I don't think we need _Nonnull annotations since we already use NS_ASSUME_NONNULL_BEGIN at the beginning of this file (here). It's always good to be defensive and guard against unexpected values in the implementation though.

nguyenhuy · 2018-01-22T12:23:16Z

+  } else {
+    return AS_UIContentSizeCategoryUnspecified();
+  }
+}


Nit, feel free to ignore this: It's a bit shorter to use ternary operator here, i.e:

return sizeCategory ? sizeCategory : AS_UIContentSizeCategoryUnspecified();

nguyenhuy · 2018-01-22T12:23:44Z

    .userInterfaceIdiom = UIUserInterfaceIdiomUnspecified,
    .layoutDirection = UITraitEnvironmentLayoutDirectionUnspecified,
-    .preferredContentSizeCategory = ASPrimitiveContentSizeCategoryMake(AS_UIContentSizeCategoryUnspecified()),
+    .preferredContentSizeCategory = AS_UIContentSizeCategoryUnspecified(),


Why do we remove the ASPrimitiveContentSizeCategoryMake in here. If you read the comment above ASPrimitiveContentSizeCategory we have to use ASPrimitiveContentSizeCategoryMake

* Mark [ASViewController overrideDisplayTraitsWithWindowSize] as deprecated. Code review changes: * Remove unneeded nonnull annotations * Add null check in ASTraitCollection constructor implementation * Codestyle * Add some documentation about ASPrimitiveTraitCollection vs ASTraitCollection usage

ay8s · 2018-01-23T19:49:42Z

Thanks @ypogribnyi. We don't actually initialize either ASPrimitiveCollectionTraits or ASTraitCollection ourselves.

I wonder if this might help with digging into the cause http://crashes.to/s/058c9ea3e54

ypogribnyi · 2018-01-26T18:30:04Z

@ay8s, thank you for your input! I finally managed to reproduce the crash. Looks like it occurs in ASViewControllers that are used without a node (i.e. not using -[ASViewController initWithNode:]). Could you please confirm if that is your case?

I'm reproducing it on master in ASDKgrams UIKit tab. It crashes after device rotation when trying to log the trait collection fetched with

ASPrimitiveTraitCollection traitCollection = _node.primitiveTraitCollection;

while having nil in _node.

Although having a nil node in a view controller is perfectly valid and fully documented behaviour, I didn't expect that at all.

Anyways, this PR should fix the problem.

ay8s · 2018-01-26T19:58:09Z

Yup that'd be it! Thanks @ypogribnyi!

maicki · 2018-01-31T18:10:22Z

  }
 }

+ASDISPLAYNODE_INLINE ASPrimitiveContentSizeCategory safePrimitiveContentSizeCategory(ASPrimitiveContentSizeCategory sizeCategory) {


I think we still should prefix this functions with AS

maicki · 2018-01-31T18:31:05Z

    .userInterfaceIdiom = UIUserInterfaceIdiomUnspecified,
    .layoutDirection = UITraitEnvironmentLayoutDirectionUnspecified,
-    .preferredContentSizeCategory = ASPrimitiveContentSizeCategoryMake(AS_UIContentSizeCategoryUnspecified()),
+    .preferredContentSizeCategory = AS_UIContentSizeCategoryUnspecified(),


Why do we remove the ASPrimitiveContentSizeCategoryMake in here. If you read the comment above ASPrimitiveContentSizeCategory we have to use ASPrimitiveContentSizeCategoryMake

maicki · 2018-01-31T18:32:26Z


+ASDISPLAYNODE_INLINE ASPrimitiveContentSizeCategory safePrimitiveContentSizeCategory(ASPrimitiveContentSizeCategory sizeCategory) {
+  return sizeCategory ? sizeCategory : AS_UIContentSizeCategoryUnspecified();
+}


I think in both cases we have to use ASPrimitiveContentSizeCategoryMake(AS_UIContentSizeCategoryUnspecified())

Remove safePrimitiveContentSizeCategory in favour of AS_safeContentSizeCategory.

nguyenhuy

Sorry for dropping the ball here, @ypogribnyi. LGTM now. Thanks!

…otential nil #trivial (TextureGroup#757) * Fix ASPrimitiveTraitCollection initialization on iOS 9. Add nil-value checks for preferredContentSizeCategory. * * Mark -[ASTraitCollection init] as deprecated. * Mark [ASViewController overrideDisplayTraitsWithWindowSize] as deprecated. Code review changes: * Remove unneeded nonnull annotations * Add null check in ASTraitCollection constructor implementation * Codestyle * Add some documentation about ASPrimitiveTraitCollection vs ASTraitCollection usage * Rename safeContentSizeCategory to AS_safeContentSizeCategory. Remove safePrimitiveContentSizeCategory in favour of AS_safeContentSizeCategory.

Fix ASPrimitiveTraitCollection initialization on iOS 9.

8ebd277

Add nil-value checks for preferredContentSizeCategory.

ypogribnyi mentioned this pull request Jan 19, 2018

ASTraitCollection crash Inserting nil into dictionary #754

Open

nguyenhuy requested changes Jan 22, 2018

View reviewed changes

maicki reviewed Jan 31, 2018

View reviewed changes

Rename safeContentSizeCategory to AS_safeContentSizeCategory.

88bb48e

Remove safePrimitiveContentSizeCategory in favour of AS_safeContentSizeCategory.

nguyenhuy approved these changes Mar 21, 2018

View reviewed changes

nguyenhuy merged commit e954b10 into TextureGroup:master Mar 21, 2018

Conversation

ypogribnyi commented Jan 19, 2018

Uh oh!

ay8s commented Jan 19, 2018

Uh oh!

ypogribnyi commented Jan 22, 2018

Uh oh!

nguyenhuy commented Jan 22, 2018

Uh oh!

nguyenhuy left a comment

Choose a reason for hiding this comment

Uh oh!

nguyenhuy Jan 22, 2018

Choose a reason for hiding this comment

Uh oh!

nguyenhuy Jan 22, 2018

Choose a reason for hiding this comment

Uh oh!

nguyenhuy Jan 22, 2018

Choose a reason for hiding this comment

Uh oh!

maicki Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

ay8s commented Jan 23, 2018

Uh oh!

ypogribnyi commented Jan 26, 2018

Uh oh!

ay8s commented Jan 26, 2018

Uh oh!

maicki Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

maicki Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

maicki Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

nguyenhuy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants