Bug 41035

Summary: DataViewTest2.DataView_ListChangedEventTest occasionally fails with llvm+sgen
Product: iOS Reporter: Alexander Köplinger [MSFT] <alkpli>
Component: BCL Class LibrariesAssignee: Zoltan Varga <vargaz>
Status: VERIFIED FIXED    
Severity: normal CC: gouri.kumari, kumpera, masafa, mono-bugs+monotouch, sebastien
Priority: ---    
Version: XI 9.8 (tvOS / C7)   
Target Milestone: 10.0.0 (C8)   
Hardware: PC   
OS: Mac OS   
Tags: Is this bug a regression?: ---
Last known good build:

Description Alexander Köplinger [MSFT] 2016-05-11 19:28:33 UTC
(this was split from https://bugzilla.xamarin.com/show_bug.cgi?id=32583)

I can repro the DataViewTest2.DataView_ListChangedEventTest failure on my iPad mini 4 (iOS 9.1) with Xamarin.iOS 9.8.0.294/6950f7b in about 1 of 20 runs.

I used the System.Data tests with the Release+llvm+sgen configuration.

The key thing for me was switching to SGen instead of Boehm, with Boehm or without LLVM (thumb didn't matter in my testing) I couldn't get it to repro.

I saw two different failures:

> [FAIL] DataViewTest2.DataView_ListChangedEventTest :   #10
>   Expected: -1
>   But was:  0
> 		  at MonoTests.System.Data.DataViewTest2.DataView_ListChangedEventTest () <0x1fa9e1 + 0x0023a> in <filename unknown>:0 


> [FAIL] DataViewTest2.DataView_ListChangedEventTest : System.NullReferenceException : Object reference not set to an instance of an object
> 	  at MonoTests.System.Data.DataViewTest2.DataView_ListChangedEventTest () <0x1d29e1 + 0x001cb> in <filename unknown>:0 


This seems to be happening for quite some time already, in #32583 it was noted it happens in C6 too.
It's not clear yet where the bug is, switching the GC to SGen might just cause it to manifest now without being an actual GC bug.
Comment 1 GouriKumari 2016-07-01 14:43:16 UTC
Observed the failure on two different devices an iPhone6Plus and iPad2

Error:
[FAIL] DataViewTest2.DataView_ListChangedEventTest :   #10
  Expected: -1
  But was:  0

##Logs:
Test Log:
https://wrench.internalx.com/Wrench/WebServices/Download.aspx?workfile_id=15597838
https://wrench.internalx.com/Wrench/WebServices/Download.aspx?workfile_id=15552391

Build Log:
https://gist.github.com/GouriKumari/cf09f8ca26ab5bb794f125060b6e672c
https://gist.github.com/GouriKumari/db21c76797f8a40ee192c29f063f7d06

##Test Env:
mtouch 9.8.2.10 (cycle7: 843a527)
mtouch 9.8.2.7 (cycle7: 08215a3)
Comment 2 Rodrigo Kumpera 2016-07-01 15:05:09 UTC
Hey Zoltan,

Please take a look at this one.
Comment 3 Zoltan Varga 2016-07-03 00:01:37 UTC
This happens because DataViewListener keeps a weak ref to the DataView, and deletes the listener if the DataView object gets gc-d (in DataViewListener.MaintainDataView). This can happen in the test:

			DataTable table = new DataTable ("test");
			table.Columns.Add ("col1", typeof(int));
			
			DataView view = new DataView (table);
			
			view.ListChanged += new ListChangedEventHandler (dv_ListChanged);
			
			evProp = null;
			table.Rows.Add (new object[] {1});
			Assert.AreEqual (0, evProp.NewIndex, "#1");

By the time the Add method is called, the DataView object might be dead, so the callback is not called, causing evProp to stay null, leading to a nullref.
Comment 4 Rodrigo Kumpera 2016-07-04 19:36:01 UTC
Hey Zoltan,

So can we say that the problem in this case is with the test?

Could we fix it by using a GC.KeepAlive to ensure DataView will remain alive until the end of the test?
Comment 5 Zoltan Varga 2016-07-04 20:32:54 UTC
We can certainly fix the test, whenever the current behaviour is intended or not is another question.
Comment 6 Zoltan Varga 2016-07-05 20:37:56 UTC
Fixed in master/4.5.1 branch.
Comment 7 GouriKumari 2016-07-07 14:38:09 UTC
@vargaz: Is this fix applied to xamarin-macios master repo.  If this is a test only fix , I think we can move it to Cycle8. Could you please confirm.
Comment 8 Zoltan Varga 2016-07-07 19:34:36 UTC
Mono was updated but xamarin-macios was not bumped to use it yet.
Comment 9 GouriKumari 2016-07-14 16:55:58 UTC
@vargaz: Can you update this bug if the fix is in Cycle8 branch? Since cycle8 is  following mono4.6.0, will it be included already?
Comment 10 Zoltan Varga 2016-07-14 17:15:02 UTC
The fix is in cycle8.
Comment 12 GouriKumari 2016-08-10 16:41:28 UTC
Verified with 64 bit device. DataViewTest2.DataView_ListChangedEventTest pass on devices.

##Logs:
https://wrench.internalx.com/Wrench/GetFile.aspx?id=16104398
https://wrench.internalx.com/Wrench/GetFile.aspx?id=16104402
https://wrench.internalx.com/Wrench/GetFile.aspx?id=16104406

## Test Env:
mtouch 9.10.0.52 (cycle8: fc97af6)

## Devices:
iPadAir, iPad2 and iPhone6SPlus