Hi Sergey, Thanks for the comments. Please review the updated webrev. http://cr.openjdk.java.net/~mhalder/8165555/webrev.04/ <http://cr.openjdk.java.net/~mhalder/8165555/webrev.04/>
The “frame” can’t be a local variable as it is used in two different threads inside the function. Thanks, Manajit > On 04-Oct-2016, at 6:26 pm, Sergey Bylokhov <[email protected]> > wrote: > > Hi, Manajit. > It looksbetter, bet it can be improved a little bit. > - you should not skip exceptions in the run(); > - It will be good to select the frame by mouse click before pressing the > key, otherwise you can run somthing else. > - I suggest to iterate the code in main a few times in the loop(I guess 10 > iterations is ok) > -The "frame" can be a local var inside the createRobot(), also the method > can be renamed. > > ----- [email protected] <mailto:[email protected]> wrote: > > > > Hi Sergey, > > > > Thanks for the review. Changes in CRobot.m are removed and added a jtreg > > test case. > > Please review the webrev. > > > > http://cr.openjdk.java.net/~mhalder/8165555/webrev.03/ > > <http://cr.openjdk.java.net/~mhalder/8165555/webrev.03/> > > > > Thanks, > > Manajit > > > > On 27-Sep-2016, at 4:15 pm, Sergey Bylokhov <[email protected] > > <mailto:[email protected]>> wrote: > > > On 27.09.16 11:51, Manajit Halder wrote: > Hi Sergey, > > Thanks for the comment. “self.javaToMacKeyMap is retaining the reference. > Please review the modified code: > > http://cr.openjdk.java.net/~mhalder/8165555/webrev.02/ > <http://cr.openjdk.java.net/~mhalder/8165555/webrev.02/> > > The changes in CRobot.m is unnecessary? I also suggest to write the test. > Recently I have run all jck tests on my osx system and no of them were > crashed, so it seems that the sequence of tests in the bug report are not > stable and the new reg test will be welcome to catch such bugs always. > > On 23-Sep-2016, at 10:24 pm, Sergey Bylokhov > <[email protected] <mailto:[email protected]> > <mailto:[email protected] <mailto:[email protected]>>> > wrote: > > On 21.09.16 16:01, Manajit Halder wrote: > Hi Sergey, > > Thanks for the comment. > > Access to "instance" is not broken. The problem is with the dictionary > variable "javaToMacKeyMap" within the "instance" reference. > The dictionary is not getting initialised for the second time when > singleton method is called for the second time. The dictionary is > getting initialised during the first time singleton method is called and > hence crash was observed. Tried adding self.javaToMacKeyMap > but doesn’t solve the problem. > > I still suggest you to check "self.javaToMacKeyMap" at line 48 this > should call generated setter which will retain the reference. > > > I propose two solutions to this problem: > > Solution 1: > Reinitialise the dictionary every-time the singleton method is called. > Webrev: > http://cr.openjdk.java.net/~mhalder/8165555/webrev.01/ > <http://cr.openjdk.java.net/~mhalder/8165555/webrev.01/> > > Drawback: > dictionary is initialised multiple times (every time singleton method is > called). > > Solution 2: > Make the dictionary static. > Drawback: > Still dictionary need to be initialised multiple times. > No singleton method, just two static methods, one method to initialise > the dictionary and other to get the key form the dictionary. > Webrev: > Not prepared. Will be prepared if second solution accepted. > > Test case: > There are no test cases. Problem can be reproduced by > executing following JCK test: > java -jar JCK-runtime-9/lib/jtjck.jar -mode:single -k:interactive > "api/java_awt/interactive/event/EventTests.html#EventTest0019 > api/java_awt/interactive/event/EventTests.html#EventTest0013” > > Thanks, > Manajit > > On 20-Sep-2016, at 4:42 am, Sergey Bylokhov > <[email protected] <mailto:[email protected]> > <mailto:[email protected] <mailto:[email protected]>> > <mailto:[email protected] <mailto:[email protected]>>> > wrote: > > Hi, Manajit. > It seems that after the fix "(CRobotKeyCodeMapping *) sharedInstance" > returns the new object per invocation, so it is not really > sharedInstance. I am not sure I understand what is wrong in the > current code, from the my point of view this is a correct singleton. > It it true that the problem is in access to broken "instance" and not > to "javaToMacKeyMap" inside the "instance"? If not then > "javaToMacKeyMap" should be changed to "self.javaToMacKeyMap". > Do you have a test case to reproduce the bug? > > On 19.09.16 15:26, Manajit Halder wrote: > Hi All, > > Kindly review the fix for JDK9. > > Bug: > https://bugs.openjdk.java.net/browse/JDK-8165555 > <https://bugs.openjdk.java.net/browse/JDK-8165555> > > Webrev: > http://cr.openjdk.java.net/~mhalder/8165555/webrev.00/ > > Issue: > [macosx] VM crashes on second attempt to execute JCK interactive tests > that use Robot (single JVM, agent) > > Cause: > While executing the JCK test for the second time the robot was getting > initialised once again and old instance of CRobotKeyCodeMapping was not > available. > Crash was observed while trying to access invalid instance of > CRobotKeyCodeMapping. > > Fix: > A new instance of CRobotKeyCodeMapping is created when robot is > initialised. > > Regards, > Manajit > > > -- > Best regards, Sergey. > > > > -- > Best regards, Sergey. > > > > -- > Best regards, Sergey.
