Fix crashes with browser capabilities

[echampet]

Correct https://bugzilla.xamarin.com/show_bug.cgi?id=14720 and
probably https://bugzilla.xamarin.com/show_bug.cgi?id=13014

Signed-off-by: Etienne CHAMPETIER etienne.champetier@fiducial.net
Signed-off-by: Frederic Mestayer f.mestayer@fiducial.net

in mcs/class/System.Web
make run-test PROFILE=net_2_0 ok
make run-test PROFILE=net_4_0 ok
make run-test PROFILE=net_4_5 ok

make run-test PROFILE=net_3_5 doesn't run at all on my system

[kumpera]

Please provide unit tests for this change.

[echampet]

Hi, I think I've to load browscap.ini here

mono/mcs/class/System.Web/Test/System.Web/AppBrowsersTest.cs

Line 47 in 0a2f219

public void TestFixtureSetUp ()

but i've no clue how.

[akoeplinger]

You should specify the capacity parameter of the Hashtable constructor to avoid resize operations while adding keys, i.e. new Hashtable (value.Count, StringComparer.OrdinalIgnoreCase);

Before your change, this was done internally by the constructor that takes an IDictionary.

[echampet]

@akoeplinger, i've just updated my commit, thanks
(tests are still ok)

[akoeplinger]

The tests in the class you mentioned seem to be ignored for many years (it says "Pending fix for bug 351878", which likely refers to this item in the old Novell Bugzilla).

I think adding tests there wouldn't make sense now. Can someone from the Mono team comment if the original bug was fixed so the tests can be reenabled?

[knocte]

A lot of time has passed since then, and the runtime has been receiving lots of fixes recently, there is a chance that this bug is fixed, and you can enable those tests now, just remove the [Ignore] attribute and check if they pass (run them many times just to make sure the bug doesn't appear in any case).

[echampet]

After reenabeling

in mcs/class/System.Web
make run-test PROFILE=net_2_0 FIXTURE=System.Web.AppBrowsersTest

Excluded categories: NotWorking,ValueAdd,CAS,InetAccess
..F...
Tests run: 5, Failures: 1, Not run: 0, Time: 7.874 seconds

Test Case Failures:

1. MonoTests.System.Web.AppBrowsersTest.AppBrowsersCapabilities : NUnit.Framework.AssertionException: AppBrowsersCapabilities Mono 2 8 - please merge the ucontext_t* changes from head. #8
   Expected: "0"
   But was: null

at NUnit.Framework.Assert.That (System.Object actual, NUnit.Framework.Constraints.Constraint constraint, System.String message, System.Object[] args) [0x00020] in /opt/echange/mono/mcs/nunit24/NUnitFramework/framework/Assert.cs:2205
at NUnit.Framework.Assert.AreEqual (System.Object expected, System.Object actual, System.String message, System.Object[] args) [0x00000] in /opt/echange/mono/mcs/nunit24/NUnitFramework/framework/Assert.cs:886
at NUnit.Framework.Assert.AreEqual (System.Object expected, System.Object actual, System.String message) [0x00000] in /opt/echange/mono/mcs/nunit24/NUnitFramework/framework/Assert.cs:900
at MonoTests.System.Web.AppBrowsersTest.AppBrowsersCapabilities_OnLoad (System.Web.UI.Page p) [0x000d4] in /opt/echange/mono/mcs/class/System.Web/Test/System.Web/AppBrowsersTest.cs:84
at MonoTests.SystemWeb.Framework.PageInvoker.Invoke (MonoTests.SystemWeb.Framework.PageDelegate callback) [0x00006] in /opt/echange/mono/mcs/class/System.Web/Test/mainsoft/NunitWeb/NunitWeb/PageInvoker.cs:276
at System.Runtime.Remoting.Proxies.RealProxy.PrivateInvoke (System.Runtime.Remoting.Proxies.RealProxy rp, IMessage msg, System.Exception& exc, System.Object[]& out_args) [0x001e9] in /opt/echange/mono/mcs/class/corlib/System.Runtime.Remoting.Proxies/RealProxy.cs:248

With the patch all 5 tests are ok
I've commented CompatBrowserIE7 (it doesn't pass)

[knocte]

Some recommendations (briefly discussed already in IRC):

• Don't comment tests, but add them the NUnit category "NotWorking".
• Add a test with the testcase of the bug you're fixing.
• It's recommended that the first word of the first line of each commit message is the assembly you're working on (if working on the BCL). In this case it would be "[System.Web]".

[echampet]

It's a WIP, make run-test PROFILE=net_2_0 ok
make run-test PROFILE=net_4_0 ko

[echampet]

I got 'Parent not found with id = default'

mono/mcs/class/System.Web/System.Web.Configuration_2.0/nBrowser/Build.cs

Line 206 in 0f2995e

throw new nBrowser.Exception(String.Format("Parent not found with id = {0}", child.ParentId));

I think it's related to the lack of Compat.browser in NET_4_*

[knocte]

Should that be fixed then? Maybe copy the Compat.browser file to all etc profile folders instead of just one, in the install make target?

[echampet]

Tests for 2, 4, 4.5 are ok.
For the test case i simply reenable AppBrowsersTest.cs tests.

[knocte]

Thanks for the update!

I think it is fine that the file Compat.browser ends up triplicated in $prefix/etc, however I think it's not good to have it triplicated in the git repository, can you avoid this?

In regards to "For the test case i simply reenable AppBrowsersTest.cs tests", do any of those tests have duplicate keys in browser capabilities? If not, then you're not really adding a test for this bugfix (as Rodrigo has requested), but restoring old tests unrelated to this issue.

[echampet]

The test CompatBrowserIE7 is failing because of Key duplication

[knocte]

Ah ok!

[echampet]

Tests still ok, rpm creation should be ok (just modified mono-core.spec.in to include Compat.browser, also msvc/win32.xml for win*)
Please merge :)

[knocte]

Instead of

•      if (!capabilities.Contains (key))
  

•        capabilities.Add (key, value [key]);
  

this would be shorter (and would make the last duplicated value the one to use, instead of the first one):

•       capabilities [key] = value [key];
  

Right?