Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v7]
On Thu, 26 May 2022 07:26:38 GMT, Jay Bhaskar wrote:
>> This PR is new implementation of JavaEvent listener memory management.
>> Issue
>> [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1)
>>
>> 1. Calling remove event listener does not free jni global references.
>> 2. When WebView goes out of scope (disposed from app) , its Event Listeners
>> are not being garbage collected.
>>
>> Solution:
>> 1. Detached the jni global reference from JavaEventListener.
>> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni
>> global reference.
>> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener.
>> 4. EventListenerManager is a singleton class , which stores the
>> JavaObjectWrapperHandler mapped with JavaEventListener.
>> 5. EventListenerManager also stores the JavaEventListener mapped with
>> DOMWindow.
>> 6. When Event listener explicitly removed , JavaEventListener is being
>> forwarded to EventListenerManager to clear the listener.
>> 7. When WebView goes out of scope, EventListenerManager will de-registered
>> all the event listeners based on the ref counts attached with WebView
>> DOMWindow.
>
> Jay Bhaskar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Adding space for map include
Comments inline.
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
line 106:
> 104: else
> 105: isReferringToOtherListener = true;
> 106: }
I think I now understand what this is trying to do, but it doesn't looks like
it's implemented correctly nor is it optimal.
It seems that the `isReferringToOtherListener` flag is intended to be true iff
there is any `JavaEventListener` with a ref count > 1 associated with the
Window being unregistered. Ignoring any possible bugs in how it is implemented,
there are two problems with this approach. First, you want to remove entries
associated with a particular listener if _that_ listener isn't used by another
window. So a global "is any listener referring to another window" isn't what
you want. Second, since this is multimap, it would seem better to remove
individual `(key,value)` pairs associated with the specific window being
removed rather than calling erase only for those listeners that aren't being
shared at all. If this is feasible, then you wouldn't have to even care if that
listener is used by a node in another window. I'm not familiar enough with C++
multimap calls to know how easy this would be, but presumably, it shouldn't be
too hard.
Not directly related to this, it might be cleaner to move this logic to
`unregisterDOMWindow` instead of having a separate `resetDOMWindow` method,
since this is really part of the same conceptual operation.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 607:
> 605: */
> 606: @Test
> 607: public void TestStrongRefNewContentLoad() throws Exception {
Minor: method names should start with a lower-case letter.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 730:
> 728: });
> 729:
> 730: // Verify that the events are delivered to the listeners (0 and
> 2 are same)
Minor: all three listeners are the same, not just 0 and 2.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
line 959:
> 957: // Verify that the event is delivered to the listener
> 958: assertEquals("Click count", 1, listeners.get(0).getClickCount());
> 959: assertEquals("Click count", 1, listeners.get(0).getClickCount());
The second check should be `listeners.get(1)`
-
PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v7]
> This PR is new implementation of JavaEvent listener memory management. > Issue > [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) > > 1. Calling remove event listener does not free jni global references. > 2. When WebView goes out of scope (disposed from app) , its Event Listeners > are not being garbage collected. > > Solution: > 1. Detached the jni global reference from JavaEventListener. > 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni > global reference. > 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. > 4. EventListenerManager is a singleton class , which stores the > JavaObjectWrapperHandler mapped with JavaEventListener. > 5. EventListenerManager also stores the JavaEventListener mapped with > DOMWindow. > 6. When Event listener explicitly removed , JavaEventListener is being > forwarded to EventListenerManager to clear the listener. > 7. When WebView goes out of scope, EventListenerManager will de-registered > all the event listeners based on the ref counts attached with WebView > DOMWindow. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Adding space for map include - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/4c8e4a5b..d6fd438b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=06 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/799.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/799/head:pull/799 PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v6]
On Thu, 26 May 2022 07:08:59 GMT, Jay Bhaskar wrote: >> This PR is new implementation of JavaEvent listener memory management. >> Issue >> [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) >> >> 1. Calling remove event listener does not free jni global references. >> 2. When WebView goes out of scope (disposed from app) , its Event Listeners >> are not being garbage collected. >> >> Solution: >> 1. Detached the jni global reference from JavaEventListener. >> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni >> global reference. >> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. >> 4. EventListenerManager is a singleton class , which stores the >> JavaObjectWrapperHandler mapped with JavaEventListener. >> 5. EventListenerManager also stores the JavaEventListener mapped with >> DOMWindow. >> 6. When Event listener explicitly removed , JavaEventListener is being >> forwarded to EventListenerManager to clear the listener. >> 7. When WebView goes out of scope, EventListenerManager will de-registered >> all the event listeners based on the ref counts attached with WebView >> DOMWindow. > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > Adding new review change modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h line 34: > 32: #include "config.h" > 33: > 34: #include nit: space - PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v6]
> This PR is new implementation of JavaEvent listener memory management. > Issue > [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) > > 1. Calling remove event listener does not free jni global references. > 2. When WebView goes out of scope (disposed from app) , its Event Listeners > are not being garbage collected. > > Solution: > 1. Detached the jni global reference from JavaEventListener. > 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni > global reference. > 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. > 4. EventListenerManager is a singleton class , which stores the > JavaObjectWrapperHandler mapped with JavaEventListener. > 5. EventListenerManager also stores the JavaEventListener mapped with > DOMWindow. > 6. When Event listener explicitly removed , JavaEventListener is being > forwarded to EventListenerManager to clear the listener. > 7. When WebView goes out of scope, EventListenerManager will de-registered > all the event listeners based on the ref counts attached with WebView > DOMWindow. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Adding new review change - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/2a09a847..4c8e4a5b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=04-05 Stats: 34 lines in 5 files changed: 18 ins; 9 del; 7 mod Patch: https://git.openjdk.java.net/jfx/pull/799.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/799/head:pull/799 PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v5]
On Wed, 25 May 2022 14:57:13 GMT, Ambarish Rapte wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Review comments applied > > modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp > line 109: > >> 107: isReferringToOtherListener = false; >> 108: else >> 109: isReferringToOtherListener = true; > > I have not looked very clearly, but here is a question: Should the loop break > when `if` condition passes ? No, it should not , if condition pass means there is ref count of 1 for the particular ref object. but we have to look for all other objects too , to check whether a ref counted object might be referred by other Dom Window. - PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v5]
On Wed, 25 May 2022 12:45:59 GMT, Ambarish Rapte wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review comments applied
>
> modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
> line 34:
>
>> 32: EventListenerManager& EventListenerManager::get_instance()
>> 33: {
>> 34: static NeverDestroyed sharedManager;
>
> This does behave well as a singleton class. But I think the instance should
> be class member variable or a pointer.
Most of the singleton class implementation is as above in Webkit.
-
PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8286867: Update #getGlyphCode return a negative number
On Fri, 13 May 2022 05:34:08 GMT, Tomator wrote:
> When I used BlueJ, I found a problem with Chinese display.
> /javafx.graphics/com/sun/javafx/font/CompositeGlyphMapper.java#getGlyphCode
> may return a negative number when no font library is specified in Linux,and
> this could cause java.lang.ArrayIndexOutOfBoundsException error.So
> javafx.graphics/com/sun/prism/impl/GlyphCache.java#getCachedGlyph shou check
> the glyphCode.
> The crash demo like this:
> `import javafx.application.Application;
> import javafx.scene.Scene;
> import javafx.scene.control.Menu;
> import javafx.scene.control.MenuBar;
> import javafx.scene.control.MenuItem;
> import javafx.scene.layout.BorderPane;
> import javafx.stage.Stage;
>
> public class MenuExample extends Application {
> public static void main(String[] args) {
> launch(args);
> }
>
> @Override
> public void start(Stage primaryStage) throws Exception {
> BorderPane root = new BorderPane();
> Scene scene = new Scene(root,200,300);
> MenuBar menubar = new MenuBar();
> Menu FileMenu = new Menu("文本");
> MenuItem filemenu1=new MenuItem("新建");
> MenuItem filemenu2=new MenuItem("Save");
> MenuItem filemenu3=new MenuItem("Exit");
> Menu EditMenu=new Menu("Edit");
> MenuItem EditMenu1=new MenuItem("Cut");
> MenuItem EditMenu2=new MenuItem("Copy");
> MenuItem EditMenu3=new MenuItem("Paste");
> EditMenu.getItems().addAll(EditMenu1,EditMenu2,EditMenu3);
> root.setTop(menubar);
> FileMenu.getItems().addAll(filemenu1,filemenu2,filemenu3);
> menubar.getMenus().addAll(FileMenu,EditMenu);
> primaryStage.setScene(scene);
> primaryStage.setTitle("TEST");
> primaryStage.show();
>
> }
> } `
>
> the error:
> `java.lang.ArrayIndexOutOfBoundsException: Index -25 out of bounds for length
> 32
> at com.sun.prism.impl.GlyphCache.getCachedGlyph(GlyphCache.java:332)
> at com.sun.prism.impl.GlyphCache.render(GlyphCache.java:148)
> at
> com.sun.prism.impl.ps.BaseShaderGraphics.drawString(BaseShaderGraphics.java:2101)
> at com.sun.javafx.sg.prism.NGText.renderText(NGText.java:312)
> at com.sun.javafx.sg.prism.NGText.renderContent2D(NGText.java:270)
> at com.sun.javafx.sg.prism.NGShape.renderContent(NGShape.java:261)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
> at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
> at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
> at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
> at com.sun.javafx.tk.quantum.ViewPainter.doPaint(ViewPainter.java:479)
> at
> com.sun.javafx.tk.quantum.ViewPainter.paintImpl(ViewPainter.java:328)
> at
> com.sun.javafx.tk.quantum.PresentingPainter.run(PresentingPainter.java:91)
> `
I can repeated this issue on uos when the linux doesn't have
/usr/share/fonts/wps-office/FZSongS_20100603.TTF file ,openjfx version
11,method getGlyphCode will return a negative number
-
PR: https://git.openjdk.java.net/jfx/pull/795
