Bug ID: JDK-8151459 Validation of new behaviour for JS callback memory leak

JDK 8	JDK 9
8u102Fixed	9Fixed

Backport webrev looks good. +1
04-04-2016
webrev for 8u (backport) : http://cr.openjdk.java.net/~mbilla/8151459/8u/webrev.00/
04-04-2016
Note that minor changes will be needed for the backport. 1) location of test file (there is no "test/" prefix in the directory tree) 2) Imports for any class in test.* will need to change to drop the "test." 3) Platform.startup doesn't exist as public API; use PlatformImpl.startup instead.
31-03-2016
Changeset: a251a1d65932 Author: mbilla Date: 2016-03-31 13:57 -0700 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/a251a1d65932
31-03-2016
+1
31-03-2016
webrev: http://cr.openjdk.java.net/~mbilla/8151459/webrev.03/ @kevin, incorporated changes for above comments. Thanks for the above important note.
31-03-2016
Test looks very good now. Two minor comments for cleanup: 1) There are unused import statements that can be removed (NetBeans will flag these as warnings...they are not harmful, but good to clean up) 2) There is an extra space in the following line (I checked and it is the only remaining one). 347 for (int i = 0; i < NUM_STAGES; i++) { ------------------- Additionally, I noticed the following, but there is no problem with it: *) I think that the assertEquals / assertSame checks in the callback methods for strong ref arrays will not actually be reported, since they are called from JavaScript and exceptions will not be propagated back to the calling code. However, it is OK since it will abort the rest of that method and thus skip the setting of callbackStatus[stageIndex] = true;
31-03-2016
I verified the same 4 failures on Mac. Good. I'll run the test with your fixes and review the code while it is running.
31-03-2016
@Kevin: Tested by backing out both fixes (JDK-8089681, JDK-8152737) and observed that 4 test cases failed (which is expected) in windows: test.memoryleak.JSCallbackMemoryTest > testJsCallbackReleaseFunction FAILED java.lang.AssertionError: test.memoryleak.JSCallbackMemoryTest > testJsCallbackLocalPrimitiveArrayFunctionWithGC FAILED java.lang.AssertionError: test.memoryleak.JSCallbackMemoryTest > testJsCallbackLocalObjectArrayFunctionWithGC FAILED java.lang.AssertionError: test.memoryleak.JSCallbackMemoryTest > testJsCallbackLeak FAILED java.lang.AssertionError: expected:<5> but was:<0>
31-03-2016
webrev: http://cr.openjdk.java.net/~mbilla/8151459/webrev.02/ @Kevin, Incorporated the changes regarding the above comments and tested in windows.
31-03-2016
I debugged the problem with the local Object array, and it is a test issue related to the fact that the WebEngine holds onto the web state listener (the lambda) which in turn holds a reference to the array. The simplest way to solve this would be to move the allocation of the array to the setMember call itself, so that no reference is held by the test app. Something like this: window.setMember("localObjectArray", new Object[] { new Object(), new Object(), new Object(), new Object() }); I note that in this case, the callback is still called but it is called with a null object array if the array has been cleaned. So the callback could do something like this: public void jscallback5(Object[] objArray) { if (objArray != null) { unexpectedCallback = true; } } Same thing applies for the local primitive array.
30-03-2016
Overall the tests look good. Just a couple functional issues and a few style issues to fix Questions: ---------- * Only one test failed on Mac (two on Windows) with both fixes backed out; should more have failed? Maybe not, but I was expecting more failures Functional issues: ------------------ * You should either remove all debug print statements or qualify them with "if (VERBOSE)" where VERBOSE is a "private static final boolean" that is set to false (meaning someone could set it to true for debugging). * Line 117: the following should be assertEquals (assertSame is for comparing Object references) assertSame(primitiveArray[i], pArray[i]); * I think the following lines should be removed, since you are hiding the stage later in these tests: 217 stage.hide(); 288 stage.hide(); 347 stage.hide(); 400 stage.hide(); 449 stage.hide(); 503 stage.hide(); 566 stage.hide(); 624 stage.hide(); * You don't actually check that the following callback is not called: 341 webview.getEngine().executeScript("document.getElementById(\"mybtn4\").click()"); If there is a good way to check this (using a boolean instance var) that would be a good addition. This might explain why there aren't more failures with both of your patches removed. * I'm surprised that the following is true for a local array. I would expect it to be false (not called). Can you explain this? I worry that this points to a bug. 520 assertTrue(callbackstatus[i]); Coding style issues: -------------------- * You might want to switch the following two so that all static fields are grouped together: 74 private Stage[] stages = new Stage[NUM_STAGES]; 75 76 private static Stage primarystage; * In the following: 80 final private boolean[] callbackstatus = new boolean[NUM_STAGES]; the order should be "private final" per coding style * Suggestion: you can make all of the fields private if you like; some are default "package scope" (e.g., html and refs). Since this is a test it doesn't matter much...just thinking that it would be more consistent. * MyOjbect does not need to extend Pane (doesn't need to extend anything really) since you don't use it as a Pane * Indentation is wrong for print statement in MyObject::jsobjcallback (although if you eliminate the debug prints it will no longer be a problem) * Remove the extra space after the '[]' in the following: private WebView[] webviewarray = new WebView[NUM_STAGES]; ... private final int[] primitiveArray = {1, 2, 3, 4, 5}; also webviewarray can be final (and maybe capitalize the "a" in "array" for consistency) also there should be a space after '{' and before '}' in primitiveArray init * Other spacing issues: 116 for (int i =0; i < pArray.length; i++) { 132 for (int i =0; i < objArray.length; i++) { 474 int[] localPrimitiveArray = {1, 2, 3, 4, 5};
30-03-2016
webrev: http://cr.openjdk.java.net/~mbilla/8151459/webrev.01/ @Kevin R: I added total 8 test cases including primitive array and Object arrays. Also Tested without fix for JDK-8089681 and JDK-8152737 and executed the 8 test cases and made sure some of the test cases will fail/crash.
30-03-2016
In addition to added a new test case to verify JDK-8152737 it would be a good idea to look for ways to exercise other code paths that might have been missed.
25-03-2016
I verified that testJsCallbackLeak fails on a build without your fix and works with your fix. Good. I did spot a few things during the review that will need to be addressed: 1. The "refs" set should not be static 2. "index" should not be static. As it stands, this is fragile since your tests would fail with an AIOOBE if someone later adds another test that calls the callback function. Also, it might be a better test if the stage number were cached in the TestStage itself. That way you could tell the difference between each of 5 stages hitting the callback (success) once versus one of them getting called back 5 times (as it is, you might as well use a counter as a boolean array). This one may not be important, so if you prefer not to address it, then at least change the index to an instance variable. 3. testJsCallbackLeak: After the check for the stages being cleaned up, you could verify that calling the javascript callback does not crash and does not execute anything -- basically verify that callbackstatus[i] is false. 4. testJsCallbackFunction: After verifying that the JS callbacks are executed, you might want to hide the stages (so move the hide to a second runAndWait block) and then check that the stages can be cleaned up. This will ensure that executing the callback doesn't cause a leak. 5. Since STAGE_OFFSET is not final, I recommend making it camel case -- stageOffset -- for clarity. More to the point, I think it can be a local variable, since it seems fine for separate instances of the test to reuse the position (the important thing is that the N windows for a particular test are at different offsets). 6. "assertEquals(callbackstatus[i], true)" can just be "assertTrue(callbackstatus[i])" (it's backwards as you have it anyway, since expected value should be first). 7. Please fix the following formatting problems (refer to our coding style) 52 public class JSCallbackMemoryTest 53 { curly brace should be on the same line as the class name (with a space between them) 168 for (int j = 0; j < 2 ; j++ ) { you have an extra space after the "2" and also after the "j++" ... 173 for ( WeakReference<Object> ref: refs ) { extra spaces after the "(" and before the ")". Please add a missing space before the ":" operator. 174 if ( ref.get() == null ) 175 collected.add(ref); In addition to the extra spaces, one of our "bug rules" is that the target of an "if" statement must be surrounded by curly braces. ... 179 if ( j == 0) { remove the extra space 201 stage.setTitle("Stage "+ i); add missing space before the '+'
23-03-2016
webrev: http://cr.openjdk.java.net/~mbilla/8151459/webrev.00/
22-03-2016

Duplicate :	JDK-8158783 - [TEST BUG] Need test for JDK-8152737
Relates :	JDK-8152737 - Crash in RuntimeObject::put when object passed to JSObject::setMember is GCed
Relates :	JDK-8089681 - WebView leaks memory when containing object acts as javascript callback handler