aballway
commented
7 years ago Seems to be introduced in https://github.com/Microsoft/WinObjC/commit/88975cab8f6346ca3b318e2847b2ba6dae4aa767
Open ashvarma opened 7 years ago
aballway
commented
7 years ago Seems to be introduced in https://github.com/Microsoft/WinObjC/commit/88975cab8f6346ca3b318e2847b2ba6dae4aa767
jaredhms
commented
7 years ago We recently shifted Islandwood over to not using queued CATransactions for its UIElement property and hierarchy management; instead we perform the property/hierarchy changes synchronously on the backing Xaml elements as they’re issued by the app. This change was required to fix several other bugs, and it should also lead to a much more simplified Islandwood implementation layer. It’s worked pretty well so far, but it’s uncovered an interesting issue with ScrollViewer’s ScrollViewerViewChangedEvent.
CTCatalog has a UIPickerView within a UITableView. Selecting an item in the UIPickerView results in reloading the entire parent UITableView’s data, which ends up removing and re-adding all of that UITableView’s contents. We don’t actually delete/recreate the UIPickerView, but it is temporarily removed from the UIElement tree, and then added back as the UITableView rebuilds its content. This may sound odd, but it’s a perfectly valid use of UITableView’s reloadData method.
Just as on iOS, our UIPickerView is built upon our UIScrollView (which is backed by a Xaml ScrollViewer). The UIPickerView’s ‘selection changed’ event is fired as a result of the UIScrollView’s ‘scrollViewDidScroll’ event, which is keyed off of the Xaml ScrollViewer’s ScrollViewerViewChangedEvent. This is where the issue lies. The ScrollViewerViewChangedEvent is fired during Arrange, and removing the ScrollViewer from the UI during this event handler leads to an AV in CFrameworkElement::ArrangeCore because the ScrollViewer’s m_pLayoutStorage was deleted.
Here's the full stack where we remove the ScrollViewer from the UIElement tree.
Windows.UI.Xaml.dll!CUIElement::Shutdown() Line 7434 C++
Windows.UI.Xaml.dll!CUIElementCollection::OnRemoveFromCollection(CDependencyObject * pDO, int iPreviousIndex) Line 207 C++
Windows.UI.Xaml.dll!CDOCollection::RemoveAt(unsigned int nIndex) Line 749 C++
Windows.UI.Xaml.dll!CUIElementCollection::Remove(unsigned int nIndex, CDependencyObject * pObject) Line 1291 C++
Windows.UI.Xaml.dll!CUIElementCollection::RemoveAt(unsigned int nIndex) Line 1344 C++
Windows.UI.Xaml.dll!Collection_RemoveAt(CCollection * pCollection, unsigned int nIndex) Line 2196 C++Windows.UI.Xaml.dll!DirectUI::PresentationFrameworkCollectionTemplateBase<Windows::UI::Xaml::UIElement >::RemoveAt(unsigned int index) Line 872 C++
UIKIT.DLL!LayerProxy::RemoveFromSuperLayer() Line 356 C++
UIKIT.DLL!LayerTransaction::_QueueMovement(char16_t layerMovement, unsigned int) Line 240 Objective-C++
UIKIT.DLL!LayerTransaction::RemoveLayer(const std::shared_ptr
…which results in an AV at Windows.UI.Xaml.dll!CFrameworkElement::ArrangeCore(XRECTF_WH finalRect) Line 1855:
RenderSize = innerInkSize;
...
#define RenderSize GetLayoutStorage()->m_sizeIt seems a bit odd that ScrollViewer raises ScrollViewerViewChangedEvent within Arrange; MSDN doesn’t call this out as a limitation. Ideally we’d be able to do whatever we want within the ScrollViewerViewChangedEvent, including removing the ScrollViewer from the UIElement tree.
jaredhms
commented
7 years ago We have a few options;
1) Update the app to opt into useLegacyBatchedCATransactions 2) Update the app to dispatch_async its call to reload the UITableView data on a subsequent run of the UI thread 3) Update UIScrollViewer to dispatch_async its call to scrollViewDidScroll:
We'll need to discuss our options here; I think 3 sounds the most promising if we can prove that it won't cause other issues.
jaredhms
commented
7 years ago @tadam-msft, what do you want to do with this one?
tadam-msft
commented
7 years ago Option 3
This is on the 1701 payload that's in master - 20170127.1