[rfc][icedtea-web] Rewrite of MethodOverloadResolver

[Adam Domurad]

On 01/15/2013 08:46 AM, Jiri Vanek wrote:
> On 01/09/2013 09:05 PM, Adam Domurad wrote:
>> On 12/14/2012 03:43 PM, Adam Domurad wrote:
>>> Thought I'd do a simple patch to turn the test code embedded in 
>>> MethodOverloadResolver into a a
>>> proper unit test ... unfortunately, my small look into this class 
>>> lead me to find many subtle bugs
>>> -- and coincidentally decide to do it properly. Lots of corner cases 
>>> covered, and it could
>>> probably use unit tests even still, but its a major improvement and 
>>> I'm a bit tired of looking at
>>> it so I'm posting it.
>>>
>>> Some broken things I uncovered included improper overloading order, 
>>> method/argument situations
>>> that would error-out if they ever occurred (but were valid), and the 
>>> fact that the JSObject used
>>> throughout the class was actually a dummy value used at the bottom 
>>> of the class. The code was also
>>> made a lot cleaner in the process, in the interest of 'doing it 
>>> right' while fixing it.
>>> ChangeLog:
>>> 2012-12-14  Adam Domurad  <adomurad at redhat.com>
>>>
>>>     Rewrite of MethodOverloadResolver with detailed unittests.
>>>     * plugin/icedteanp/java/sun/applet/MethodOverloadResolver.java:
>>>     Rewritten to reduce duplicated code, fix very subtle bugs in
>>>     never-tested codepaths, obey spec properly. Introduced new 
>>> helper types
>>>     where Object[] arrays with special-meaning positions were passed
>>>     around.
>>>     * 
>>> plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java:
>>>     Updated to work with newly introduced types / refactored overload
>>>     resolver.
>>>     * tests/netx/unit/sun/applet/MethodOverloadResolverTest.java: 
>>> In-depth
>>>     unit tests of hairy details of method overloading in JS<->Java.
>>>
>>>
>> Ping?
>> -Adam
>
> Although I really like this patch as it is making 
> MethodOverloadResolver much more readable and better, there is no 
> significant *visible* improvement (according to Jana's tests). Also I 
> was unable to review it properly - as it  was de facto replacement.
>
> Unless you+jana will find some improvement in (at least) liveconect 
> test or similar, then I must revoke this patch.
>
> If no "proof" will be found by you and jana, then I would like to get 
> this into head by small (reviewable) steps accompanied by 
> unit/reproducer tests.
>
> Sorry for troubles
>   J.

Just to be clear - I believe many fixed cases are visible in the unit 
tests. However these are difficult to determine because they were 
written against the new MethodOverloadResolver. Especially like I 
pointed out, JSObject was resolving to the completely wrong class. There 
were certain cases where exceptions would be thrown merely if a certain 
overload was present. If this is the issue, we can get more of these 
cases into the reproducers, but I do think there were a number of them. 
I'll work with Jana on this one.

-Adam