Don’t call NSAttributedString with HTML from a background thread #26153

kubaflo · 2024-11-27T12:20:07Z

According to Apple's documentation: https://developer.apple.com/documentation/foundation/nsattributedstring/1524613-initwithdata

Don’t call this method from a background thread if the options dictionary includes the NSDocumentTypeDocumentAttribute attribute with a value of NSHTMLTextDocumentType. If you do, the method tries to synchronize with the main thread, fails, and times out. Calling it from the main thread works, but can still time out if the HTML contains references to external resources. The HTML import mechanism is meant for implementing something like markdown (that is, text styles, colors, and so on), not for general HTML import.

Also, it seems to be a common problem among Apple developers:
https://forums.developer.apple.com/forums/thread/115405

Issues Fixed

Fixes #25946

Before	After
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-11-27.at.12.54.42.mp4	Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-11-27.at.12.52.55.mp4

kubaflo · 2024-11-27T19:58:27Z

I now noticed that the crash happens with CarouselView1 only when swiping between more than 2 items, but in the case of CarouselView2 the app crashes instantly when CarouselView2 has more than 1 item

<CarouselView Grid.Row="1">
    <CarouselView.ItemsSource>
        <x:Array Type="{x:Type x:String}">
            <x:String>Item1</x:String>
            <x:String>Item2</x:String>
        </x:Array>
    </CarouselView.ItemsSource>
    <CarouselView.ItemTemplate>
        <DataTemplate>
            <VerticalStackLayout Background="Red">
                <Label Text="Hello"/>
                <Label Text="{Binding .}"
                        TextType="Html"/>
            </VerticalStackLayout>
        </DataTemplate>
    </CarouselView.ItemTemplate>
</CarouselView>

kubaflo · 2024-11-27T20:04:04Z

Ohh I just bumped into a scaling issue with ListView/CollectionView1 and CollectionView1 after changes in this PR:

For the following code:

<Grid RowDefinitions="*,*">
    <CollectionView>
        <CollectionView.ItemsSource>
            <x:Array Type="{x:Type x:String}">
                <x:String>Item1</x:String>
                <x:String>Item1</x:String>
                <x:String>Item1</x:String>
            </x:Array>
        </CollectionView.ItemsSource>
        <CollectionView.ItemTemplate>
            <DataTemplate>
                <VerticalStackLayout>
                    <Label Text="Hello"/>
                    <Label Text="{Binding .}"
                            TextType="Html"/>
                </VerticalStackLayout>
            </DataTemplate>
        </CollectionView.ItemTemplate>
    </CollectionView>

    <StackLayout Grid.Row="1">
        <BindableLayout.ItemsSource>
            <x:Array Type="{x:Type x:String}">
                <x:String>Item1</x:String>
                <x:String>Item1</x:String>
                <x:String>Item1</x:String>
            </x:Array>
        </BindableLayout.ItemsSource>
        <BindableLayout.ItemTemplate>
            <DataTemplate>
                <VerticalStackLayout>
                    <Label Text="Hello"/>
                    <Label Text="{Binding .}"
                            TextType="Html"/>
                </VerticalStackLayout>
            </DataTemplate>
        </BindableLayout.ItemTemplate>
    </StackLayout>
</Grid>

I have a cell scaling issue

It gets automatically fixed after updating any of the Label's properties
@rmarinho do you maybe have an idea why it happens?

rmarinho

Nop, not from the top of my mind, can we rebase, see if it's still a issue and also add a test case for this ?

kubaflo · 2024-12-10T13:09:03Z

Nop, not from the top of my mind, can we rebase, see if it's still a issue and also add a test case for this ?

I've rebased, but the issue is still there without using MainQueue.DispatchAsync
However, I'm not sure if it is a correct fix, because it still regresses the height of HTML text in listView and cv

rmarinho

Failing tests on iOS not related

PureWeen · 2024-12-13T20:55:13Z

src/Core/src/Platform/iOS/LabelExtensions.cs

-#pragma warning disable CS8601
-			platformLabel.AttributedText = new NSAttributedString(text, attr, ref nsError);
-#pragma warning restore CS8601
+			CoreFoundation.DispatchQueue.MainQueue.DispatchAsync(() =>


I might not be following your description quite right but should we check if we're on a background thread before dispatching?

With something like this:

NSError nsError = new(); if (NSThread.IsMain) { platformLabel.AttributedText = new NSAttributedString(text, attr, ref nsError); if (label.Handler is ILabelHandler labelHandler) { labelHandler.UpdateValue(nameof(ILabel.TextColor)); labelHandler.UpdateValue(nameof(ILabel.Font)); } label.InvalidateMeasure(); } else { CoreFoundation.DispatchQueue.MainQueue.DispatchAsync(() => { platformLabel.AttributedText = new NSAttributedString(text, attr, ref nsError); if (label.Handler is ILabelHandler labelHandler) { labelHandler.UpdateValue(nameof(ILabel.TextColor)); labelHandler.UpdateValue(nameof(ILabel.Font)); } label.InvalidateMeasure(); }); }

The list view/collection view with the HTML label looks right, but the app crashes when using the collection view

Maybe use a local function to avoid code duplication.

Will the async cause issues because that would be unexpected? Will DispatchSync work/have issues/locking?

src/Core/src/Platform/iOS/LabelExtensions.cs

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/Core/src/Platform/iOS/LabelExtensions.cs:93

The NSError variable nsError is not checked after the NSAttributedString initialization. Add a check for nsError and handle any errors appropriately.

platformLabel.AttributedText = new NSAttributedString(text, attr, ref nsError);

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/Core/src/Platform/iOS/LabelExtensions.cs:93

Check and handle nsError after assigning NSAttributedString to ensure any errors are properly managed.

platformLabel.AttributedText = new NSAttributedString(text, attr, ref nsError);

kubaflo · 2024-12-17T19:21:04Z

Item1 Item1 Item1

<StackLayout Grid.Row="1">
    <BindableLayout.ItemsSource>
        <x:Array Type="{x:Type x:String}">
            <x:String>Item1</x:String>
            <x:String>Item1</x:String>
            <x:String>Item1</x:String>
        </x:Array>
    </BindableLayout.ItemsSource>
    <BindableLayout.ItemTemplate>
        <DataTemplate>
            <VerticalStackLayout>
                <Label Text="Hello"/>
                <Label Text="{Binding .}"
                        TextType="Html"/>
            </VerticalStackLayout>
        </DataTemplate>
    </BindableLayout.ItemTemplate>
</StackLayout>

I tested and this is fixed with this PR: #25664 But, I believe @albyrock87 divided it into smaller ones

jfversluis · 2024-12-18T08:40:53Z

/azp run

jfversluis · 2025-01-09T10:59:28Z

/rebase

azure-pipelines · 2025-01-09T11:02:27Z

Azure Pipelines successfully started running 3 pipeline(s).

mattleibow

Added some comment, but I just want to check. How do we know this is on a background thread? All UIKit operations are on a main thread - in fact, all of our handlers are on a main thread.

Something has gone horribly wrong if a handler mapper is updating the UI from a background thread. Where did it switch and why?

I assume this only happens in CV, so this may be some weird case where the CV is moving to a background thread... to update the UI? Feels very sus.

mattleibow · 2025-01-10T09:33:58Z

src/Core/src/Platform/iOS/LabelExtensions.cs

-#pragma warning disable CS8601
-			platformLabel.AttributedText = new NSAttributedString(text, attr, ref nsError);
-#pragma warning restore CS8601
+			CoreFoundation.DispatchQueue.MainQueue.DispatchAsync(() =>


Maybe use a local function to avoid code duplication.

mattleibow · 2025-01-10T09:36:05Z

src/Controls/src/Core/Label/Label.iOS.cs

@@ -38,13 +38,6 @@ public static void MapMaxLines(ILabelHandler handler, Label label)

 		static void MapFormatting(ILabelHandler handler, Label label)


I feel like I wrote this code and overengineered this. It appears there is just 1 usage of the MapFormatting method, and it was already weird. Maybe this should just be merged with the MapText or UpdateText?

mattleibow · 2025-01-10T09:37:05Z

src/Core/src/Platform/iOS/LabelExtensions.cs

-#pragma warning disable CS8601
-			platformLabel.AttributedText = new NSAttributedString(text, attr, ref nsError);
-#pragma warning restore CS8601
+			CoreFoundation.DispatchQueue.MainQueue.DispatchAsync(() =>


Will the async cause issues because that would be unexpected? Will DispatchSync work/have issues/locking?

mattleibow · 2025-01-10T09:37:09Z

src/Core/src/Platform/iOS/LabelExtensions.cs

+				if (label.Handler is ILabelHandler labelHandler)
+				{
+					labelHandler.UpdateValue(nameof(ILabel.TextColor));
+					labelHandler.UpdateValue(nameof(ILabel.Font));
+				}


Not sure if using handlers here is expected... Not sure if that is bad or good, but in my mind all the handler things happen in the handler or close to it. This is a few methods away now so may be surprising.

jfversluis · 2025-01-14T14:51:59Z

@kubaflo some comments!

kubaflo · 2025-01-14T16:13:08Z

@mattleibow I've refactored a bit

rmarinho · 2025-01-14T16:14:22Z

/azp run

azure-pipelines · 2025-01-14T16:14:45Z

Azure Pipelines successfully started running 3 pipeline(s).

kubaflo requested a review from a team as a code owner November 27, 2024 12:20

kubaflo requested review from Eilon and StephaneDelcroix November 27, 2024 12:20

dotnet-policy-service bot added the community ✨ Community Contribution label Nov 27, 2024

kubaflo added area-controls-collectionview CollectionView, CarouselView, IndicatorView area-controls-label Label, Span community ✨ Community Contribution and removed community ✨ Community Contribution labels Nov 27, 2024

This comment was marked as outdated.

Sign in to view

rmarinho requested changes Dec 10, 2024

View reviewed changes

kubaflo force-pushed the fix-25946 branch from 7993d42 to b515625 Compare December 10, 2024 13:03

This comment was marked as outdated.

Sign in to view

rmarinho previously approved these changes Dec 13, 2024

View reviewed changes

rmarinho added this to the .NET 9 SR3 milestone Dec 13, 2024

PureWeen requested changes Dec 13, 2024

View reviewed changes

samhouts requested a review from Copilot December 13, 2024 21:41

Copilot AI reviewed Dec 13, 2024

View reviewed changes

samhouts requested a review from Copilot December 13, 2024 21:50

Copilot AI reviewed Dec 13, 2024

View reviewed changes

samhouts requested a review from Copilot December 13, 2024 21:51

Copilot AI reviewed Dec 13, 2024

View reviewed changes

kubaflo dismissed rmarinho’s stale review via 0b99f74 December 18, 2024 00:31

This comment was marked as off-topic.

Sign in to view

PureWeen modified the milestones: .NET 9 SR3, .NET 9 SR4 Jan 5, 2025

github-actions bot force-pushed the fix-25946 branch from 0b99f74 to c102604 Compare January 9, 2025 11:00

mattleibow requested changes Jan 10, 2025

View reviewed changes

kubaflo added 3 commits January 14, 2025 16:43

Don’t call NSAttributedString with HTML from a background thread

91a584c

Added a UITest

24950f4

Refactor

d7097fd

kubaflo force-pushed the fix-25946 branch from c102604 to d7097fd Compare January 14, 2025 16:11

PureWeen added the do-not-merge Don't merge this PR label Jan 14, 2025

PureWeen modified the milestones: .NET 9 SR4, .NET 9 SR5 Jan 29, 2025

PureWeen added the partner/syncfusion/review label Feb 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don’t call NSAttributedString with HTML from a background thread #26153

Don’t call NSAttributedString with HTML from a background thread #26153

kubaflo commented Nov 27, 2024 •

edited by mattleibow

Loading

This comment was marked as outdated.

This comment was marked as outdated.

kubaflo commented Nov 27, 2024

kubaflo commented Nov 27, 2024 •

edited

Loading

rmarinho left a comment

kubaflo commented Dec 10, 2024

This comment was marked as outdated.

This comment was marked as outdated.

rmarinho left a comment

PureWeen Dec 13, 2024

kubaflo Dec 14, 2024

mattleibow Jan 10, 2025

mattleibow Jan 10, 2025

kubaflo commented Dec 17, 2024

jfversluis commented Dec 18, 2024 •

edited

Loading

This comment was marked as off-topic.

jfversluis commented Jan 9, 2025

azure-pipelines bot commented Jan 9, 2025

mattleibow left a comment •

edited

Loading

mattleibow Jan 10, 2025

mattleibow Jan 10, 2025

mattleibow Jan 10, 2025

mattleibow Jan 10, 2025

jfversluis commented Jan 14, 2025

kubaflo commented Jan 14, 2025

rmarinho commented Jan 14, 2025

azure-pipelines bot commented Jan 14, 2025

		@@ -38,13 +38,6 @@ public static void MapMaxLines(ILabelHandler handler, Label label)

		static void MapFormatting(ILabelHandler handler, Label label)

Don’t call NSAttributedString with HTML from a background thread #26153

Are you sure you want to change the base?

Don’t call NSAttributedString with HTML from a background thread #26153

Conversation

kubaflo commented Nov 27, 2024 • edited by mattleibow Loading

Issues Fixed

This comment was marked as outdated.

This comment was marked as outdated.

kubaflo commented Nov 27, 2024

kubaflo commented Nov 27, 2024 • edited Loading

rmarinho left a comment

Choose a reason for hiding this comment

kubaflo commented Dec 10, 2024

This comment was marked as outdated.

This comment was marked as outdated.

rmarinho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubaflo commented Dec 17, 2024

jfversluis commented Dec 18, 2024 • edited Loading

This comment was marked as off-topic.

jfversluis commented Jan 9, 2025

azure-pipelines bot commented Jan 9, 2025

mattleibow left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfversluis commented Jan 14, 2025

kubaflo commented Jan 14, 2025

rmarinho commented Jan 14, 2025

azure-pipelines bot commented Jan 14, 2025

kubaflo commented Nov 27, 2024 •

edited by mattleibow

Loading

kubaflo commented Nov 27, 2024 •

edited

Loading

jfversluis commented Dec 18, 2024 •

edited

Loading

mattleibow left a comment •

edited

Loading