Bug ID: JDK-8170610 [TESTBUG] Validation of JSObject Memory leak

JDK 8	JDK 9
8u151Fixed	9Fixed

Changeset: aaec0a9072b5 Author: mbilla Date: 2017-01-12 08:15 +0530 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/aaec0a9072b5
12-01-2017
+1
11-01-2017
+1
11-01-2017
webrev: http://cr.openjdk.java.net/~mbilla/8170610/webrev.04/ Few Minor Changes: 1) Renaming of variable and function names 2) Changing function "isAllElementsNull" as static.
11-01-2017
webrev: http://cr.openjdk.java.net/~mbilla/8170610/webrev.03/
10-01-2017
Thanks [~kcr] [~arajkumar] Regarding 1) , test case failed when disposer is commented out. So I will be incorporating above 1, 2,4 and 5 comments
10-01-2017
#1 - I don't have an opinion on this; if the simplified check does the job (works with current code, fails if disposer is commented out), then it is fine. #2 Regarding the checking of the reference array for all null, Murali is right that a labeled break will not work. it might aid readability to create a method to check whether all entries in the array are null. Something like: private boolean isAllNull(Reference<?>[] array) { for (i = 0; i < array.length; i++) { if (array[i] != null) return false; } return true; } That method would then get called in three places (two in the first test and one in the second test) and it might be clearer what you are testing. Just a suggestion. #3 I agree with Murali that two tests are better even though it duplicates code. When / if there is a failure only the first failure in each test method is reported. #4 - Great suggestion #5 - Good idea. How about test_getPeerCount or getJSProtectCount or similar (its shorter and still gets the idea across)
10-01-2017
Few more comments, 1. I tried to replace the JS test eval code with "new Object()", it seems to be working. So it should simplify the js existing code. 2. boolean allGCed = true; + for (int j = 0; j < count; j++) { + if (willGC[j].get() != null) { + allGCed = false; + break; + } + } + + if (allGCed) { + break; + } Consider using labeled break instead of separate variable to quite the outer loop. 3. These two tests are almost identical, just adding the below statement to "testJSObjectGarbageCollectability" will cover both the scenarios. assertTrue("All JSObjects are disposed", JSObjectShim.test_getCount() == 0); 4. After evaluating the test js code, ensure JSObject is created by asserting the below statement, assertTrue(JSObjectShim.test_getCount() > 0); 5. "test_getCount" looks too generic, Consider renaming "test_getCount()" to "test_getDisposableJSContextObjectCount".
10-01-2017
webrev: http://cr.openjdk.java.net/~mbilla/8170610/webrev.02/
10-01-2017
Regarding 2) Consider using labeled break instead of separate variable to quite the outer loop. -> We need a separate variable to avoid always executing the outer loop for 5 times. Also need to call sleep before we goto outer loop. 3) These two tests are almost identical, just adding the below statement to "testJSObjectGarbageCollectability" will cover both the scenarios. - > we wanted separate test cases (one for normal JSObject life cycle and another exclusively to test the fix for JDK-8170938. I prefer to keep separate test cases in this case. Regarding 1, 4 and 5, We can discuss with kevin further.
10-01-2017
One more issue I just noticed is with the loop that checks whether all are GC'ed. It doesn't short-circuit the outer for loop when all are GC'ed so will always take 5 seconds to execute. Rather than using an array of booleans, you might consider a single boolean inside the outer for loop that you use to track whether any are null. Something like this: for (int i = 0; i < 5; i++) { System.gc(); System.runFinalization(); boolean allGCed = true; for (int j = 0; j < count; j++) { if (willGC[j].get() != null) { allGCed = false; break; } } if (allGCed) { break; } Thread.sleep(SLEEP_TIME); } for (int j = 0; j < count; j++) { assertNull("JSObject not GC'ed", willGC[j].get()); }
09-01-2017
The following in testJSObjectGarbageCollectability is not quite right: + if (alreadyGCed[j] == false) { + assertFalse("All JSObjects are not GC'ed", false); + break; + } That block of code isn't testing anything and will always pass. Better to replace the if test and assertFalse with: assertTrue("JSObject not GC'ed", alreadyGCed[j]); The rest looks fine to me.
09-01-2017
webrev: http://cr.openjdk.java.net/~mbilla/8170610/webrev.01/ Note: 1. Retained SLEEP_TIME as "private static final int SLEEP_TIME = 1000;" 2. Made count as test case local variable. 3. Maintained the same name "test_getCount" even for Shim method.
09-01-2017
[~mbilla] Thanks for confirming my concern. Unless there is a reason to change it, perhaps it is best to leave the JS code as you have it in your .00 webrev. You do need to address the other concerns, though.
09-01-2017
Tested the below proposed JS call with logs and i get both test cases failed (assertion failed), Looks like below JS call is not exactly testing what we intended. Probably as per below change, peer type might not be "JS_CONTEXT_OBJECT." JSObject tmpJSObject = getEngine().executeScript("{}"); We need to test peer of type " JS_CONTEXT_OBJECT" but the above change does not execute the gcprotect path (tested with logs).
07-01-2017
Having SLEEP_TIME being a static final since matches our pattern in other tests. I guess it would also be OK to make it a local variable (which it was in a preliminary version until I suggested making it a static constant), but it seems fine as it is. I agree with making count a local variable, since that seems test-specific (and the name is too generic anyway for a class variable). I can't speak to the proposed change to the JS call. I am concerned that without the function call it may not be testing the same thing. I have one other comment: In JSObject: Please rename getCount() to "test_getCount()" to highlight that it is only for testing. The shim method can either likewise be renamed or not as you prefer.
07-01-2017
+ private static final int SLEEP_TIME = 1000; + private static final int count = 10000; Having those variables as static field looks bit odd to me. Since it is specific to newly introduced test cases, it is better to declare as local variables to the test case. +Object rawFn = getEngine().executeScript("" + + "(function(add) {\n" + + " window.obj = {};\n" + + " window.obj.count = add;\n" + + " return window.obj;\n" + + "})" + ); + assertTrue(rawFn instanceof JSObject); + JSObject func = (JSObject)rawFn; + + for (int i = 0; i < count; i++) { + JSObject tmpJSObject = (JSObject) func.call("call", null, 1); + getEngine().executeScript("window.obj = null"); + willGC[i] = new WeakReference<>(tmpJSObject); + } All the above cab be simplified to JSObject tmpJSObject = getEngine().executeScript("{}"); Isn't it? + for (int j = 0; j < count; j++) { + if ((willGC[j].get() == null) && (j == count - 1)) { + pass = true; + break; + } + } What if "count - 1"th object alone is garbage collected, but others are not.? + if (!pass) { + assertFalse("All JSObjects are not GC'ed", pass == false); + } Remove the condition and use assertTrue instead.
06-01-2017
[~jtulach] The attached test case 8164792.diff tests the GCbehvior for Multiple JSObjects getting created with same peer ( and as we explored, this behavior is as per the existing design) and this test case does not exactly tests the fix for JDK-8170938. Also this test case passes with/without the fix for JDK-8170938. Hence i created 2 fresh test cases (one of the test case can validate the possible future regressions for the fix JDK-8170938) as per below webrev. webrev: http://cr.openjdk.java.net/~mbilla/8170610/webrev.00/ Test case testJSObjectGarbageCollectability : This test case will check the general behavior of hether JSObject is properly Garbagecollected or not. This test case will pass with/without fix for JDK-8170938 Test case testJSObjectDisposeCount: This test case will test for regression for the fix JDK-8170938.
05-01-2017
The patch 8164792.diff shows how the test shall look like (in my opinion). However I am not able to compile and link the JDK-9 version of JavaFX on my computer properly to verify it. Also I had to make some classes and their methods public - that is unfortunate - I've noticed there some "Shims", so maybe that is a solution, but I am not sure how they work.
09-12-2016