[ASPrimitiveTraitCollection] Always treat preferredContentSize as a potential nil #trivial #757

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

Source/Details/ASTraitCollection.h

@@ -175,7 +181,7 @@ AS_SUBCLASSING_RESTRICTED
 @property (nonatomic, assign, readonly) UIUserInterfaceStyle userInterfaceStyle;
 #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

Source/Details/ASTraitCollection.m

+  } 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

Source/Details/ASTraitCollection.m

@@ -92,7 +100,7 @@ ASPrimitiveTraitCollection ASPrimitiveTraitCollectionMakeDefault() {
    .displayGamut = UIDisplayGamutUnspecified,
    .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

Source/Details/ASTraitCollection.m

@@ -32,6 +32,14 @@ ASDISPLAYNODE_INLINE UIContentSizeCategory AS_UIContentSizeCategoryUnspecified()
  }
 }

+ASDISPLAYNODE_INLINE ASPrimitiveContentSizeCategory safePrimitiveContentSizeCategory(ASPrimitiveContentSizeCategory sizeCategory) {


I think we still should prefix this functions with AS

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

Source/Details/ASTraitCollection.m

@@ -92,7 +100,7 @@ ASPrimitiveTraitCollection ASPrimitiveTraitCollectionMakeDefault() {
    .displayGamut = UIDisplayGamutUnspecified,
    .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

Source/Details/ASTraitCollection.m

@@ -32,6 +32,14 @@ ASDISPLAYNODE_INLINE UIContentSizeCategory AS_UIContentSizeCategoryUnspecified()
  }
 }

+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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ASPrimitiveTraitCollection] Always treat preferredContentSize as a potential nil #trivial #757

[ASPrimitiveTraitCollection] Always treat preferredContentSize as a potential nil #trivial #757

ypogribnyi commented Jan 19, 2018

ay8s commented Jan 19, 2018

ypogribnyi commented Jan 22, 2018

nguyenhuy commented Jan 22, 2018

nguyenhuy left a comment

nguyenhuy Jan 22, 2018

nguyenhuy Jan 22, 2018

nguyenhuy Jan 22, 2018

maicki Jan 31, 2018

ay8s commented Jan 23, 2018

ypogribnyi commented Jan 26, 2018

ay8s commented Jan 26, 2018

maicki Jan 31, 2018

maicki Jan 31, 2018

maicki Jan 31, 2018

nguyenhuy left a comment

[ASPrimitiveTraitCollection] Always treat preferredContentSize as a potential nil #trivial #757

[ASPrimitiveTraitCollection] Always treat preferredContentSize as a potential nil #trivial #757

Conversation

ypogribnyi commented Jan 19, 2018

ay8s commented Jan 19, 2018

ypogribnyi commented Jan 22, 2018

nguyenhuy commented Jan 22, 2018

nguyenhuy left a comment

Choose a reason for hiding this comment

nguyenhuy Jan 22, 2018

Choose a reason for hiding this comment

nguyenhuy Jan 22, 2018

Choose a reason for hiding this comment

nguyenhuy Jan 22, 2018

Choose a reason for hiding this comment

maicki Jan 31, 2018

Choose a reason for hiding this comment

ay8s commented Jan 23, 2018

ypogribnyi commented Jan 26, 2018

ay8s commented Jan 26, 2018

maicki Jan 31, 2018

Choose a reason for hiding this comment

maicki Jan 31, 2018

Choose a reason for hiding this comment

maicki Jan 31, 2018

Choose a reason for hiding this comment

nguyenhuy left a comment

Choose a reason for hiding this comment