Integrated: 8088420: JavaFX WebView memory leak via EventListener
On Thu, 19 May 2022 13:13:01 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. This pull request has now been integrated. Changeset: f5348503 Author:Jay Bhaskar Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/f5348503143e8d08f09b4cd48b6a3864bd09c336 Stats: 1509 lines in 11 files changed: 1503 ins; 3 del; 3 mod 8088420: JavaFX WebView memory leak via EventListener Reviewed-by: kcr, arapte - PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v9]
> 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: Adingef else block to unregisterListener - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/79f30067..c609ad9c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=08 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=07-08 Stats: 2 lines in 1 file changed: 0 ins; 1 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 [v8]
On Fri, 27 May 2022 15:42:09 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Change for unregisterDomWindow function and code cleanup > > modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp > line 57: > >> 55: } >> 56: >> 57: if (it->second && it->second->use_count() > 1) > > Shouldn't this be `else if`? The previous block calls `erase(it)`, so it > isn't valid to dereference `it` after that call. Applied and tested - PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v7]
On Thu, 26 May 2022 17:00:46 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adding space for map include > > 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. I have used the suggested method , and tested it works expected. Thanks - PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v8]
> 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: Change for unregisterDomWindow function and code cleanup - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/d6fd438b..79f30067 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=07 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=06-07 Stats: 74 lines in 5 files changed: 11 ins; 23 del; 40 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 [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]
> 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: 8088420: JavaFX WebView memory leak via EventListener [v5]
> 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: Review comments applied - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/43f8e549..2a09a847 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=03-04 Stats: 80 lines in 3 files changed: 6 ins; 59 del; 15 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 [v4]
On Tue, 24 May 2022 14:54:19 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adding JGObject in plave of raw jni object > > modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp > line 99: > >> 97: } >> 98: >> 99: void EventListenerManager::resetDOMWindow(DOMWindow* window) > > Have you validated this logic? If I understand correctly, the > `isReferringToOtherListener` var will be true if any listener in the list of > listeners for this DOM window has a ref count > 1. Is my understanding > correct? This doesn't seem quite right to me, but I may be missing something. yes , this is the case where we would remove pairs from map. - PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v4]
> 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 JGObject in plave of raw jni object - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/5dc52378..43f8e549 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=02-03 Stats: 32 lines in 3 files changed: 25 ins; 2 del; 5 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 [v2]
On Thu, 19 May 2022 21:45:00 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Platform.exit() , removing code block, as it is causing other test fail
>
> modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h
> line 48:
>
>> 46:
>> 47: class JavaObjectWrapperHandler {
>> 48: jobject handler_;
>
> Have you considered using `JGObject` here instead of raw `jobject`?
Applied
-
PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v2]
On Thu, 19 May 2022 21:34:54 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Platform.exit() , removing code block, as it is causing other test fail
>
> modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
> line 57:
>
>> 55: if (it->second && it->second->use_count() == 1) {
>> 56: delete it->second;
>> 57: it->second = nullptr;
>
> Do you need to remove the item from the list in this case?
applied new change , for removal of unused pairs
> modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
> line 91:
>
>> 89: for (win_it = windowHasEvent.begin(); win_it !=
>> windowHasEvent.end(); win_it++) {
>> 90: // de register associated event listeners with window
>> 91: if(window == win_it->second) {
>
> Minor: add space after the `if`.
done
> modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp
> line 92:
>
>> 90: // de register associated event listeners with window
>> 91: if(window == win_it->second) {
>> 92: unregisterListener(win_it->first);
>
> Same question as earlier: do you need to also remove the entries matching the
> `window` from the list here?
applied
> modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h
> line 48:
>
>> 46:
>> 47: class JavaObjectWrapperHandler {
>> 48: jobject handler_;
>
> Have you considered using `JGObject` here instead of raw `jobject`?
No
-
PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v3]
> 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: remove data from map and extend unit test - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/2d5604bf..5dc52378 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=01-02 Stats: 419 lines in 6 files changed: 407 ins; 7 del; 5 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 [v2]
> 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: Platform.exit() , removing code block, as it is causing other test fail - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/a62558ea..2d5604bf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=00-01 Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 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 [v2]
On Thu, 19 May 2022 15:22:08 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Platform.exit() , removing code block, as it is causing other test fail
>
> modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java
> line 113:
>
>> 111: public static void cleanupOnce() {
>> 112: Platform.exit();
>> 113: }
>
> I just noticed that this will cause subsequent tests to fail. Unit tests
> should not call `Platform.exit()` (as opposed to system tests, which should).
> You can remove the entire method.
applied
-
PR: https://git.openjdk.java.net/jfx/pull/799
RFR: 8088420: JavaFX WebView memory leak via EventListener
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. - Commit messages: - 8088420: JavaFX WebView memory leak via EventListener, code clean up - 8088420: JavaFX WebView memory leak via EventListener Changes: https://git.openjdk.java.net/jfx/pull/799/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=799=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8088420 Stats: 1148 lines in 11 files changed: 1142 ins; 3 del; 3 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
Integrated: 8284184: Crash in GraphicsContextJava::drawLinesForText on https://us.yahoo.com/
On Sat, 2 Apr 2022 08:29:43 GMT, Jay Bhaskar wrote: > Issue: Floating point overflow , when making end point for line drawing as > FloatPoint endPoint = startPoint + FloatPoint(widths.last(), 0); > Solution: traverse widths to calculate end point and increment start point. This pull request has now been integrated. Changeset: 64da125f Author:Jay Bhaskar Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/64da125f05f2a25038ce3370c8fe7c0baf0a354b Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod 8284184: Crash in GraphicsContextJava::drawLinesForText on https://us.yahoo.com/ Reviewed-by: kcr, arapte - PR: https://git.openjdk.java.net/jfx/pull/765
Re: RFR: 8284184: Crash in GraphicsContextJava::drawLinesForText on https://us.yahoo.com/ [v2]
On Thu, 7 Apr 2022 09:55:36 GMT, Jay Bhaskar wrote: >> Issue: Floating point overflow , when making end point for line drawing as >> FloatPoint endPoint = startPoint + FloatPoint(widths.last(), 0); >> Solution: traverse widths to calculate end point and increment start point. > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > check length of DashArray Yes, i will file a new follow up bug - PR: https://git.openjdk.java.net/jfx/pull/765
Re: RFR: 8284184: Crash in GraphicsContextJava::drawLinesForText on https://us.yahoo.com/ [v2]
> Issue: Floating point overflow , when making end point for line drawing as > FloatPoint endPoint = startPoint + FloatPoint(widths.last(), 0); > Solution: traverse widths to calculate end point and increment start point. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: check length of DashArray - Changes: - all: https://git.openjdk.java.net/jfx/pull/765/files - new: https://git.openjdk.java.net/jfx/pull/765/files/feda90ec..5c503f5e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=765=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=765=00-01 Stats: 10 lines in 1 file changed: 3 ins; 3 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/765.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/765/head:pull/765 PR: https://git.openjdk.java.net/jfx/pull/765
Re: RFR: 8284184: Crash in GraphicsContextJava::drawLinesForText on https://us.yahoo.com/
On Sat, 2 Apr 2022 08:29:43 GMT, Jay Bhaskar wrote:
> Issue: Floating point overflow , when making end point for line drawing as
> FloatPoint endPoint = startPoint + FloatPoint(widths.last(), 0);
> Solution: traverse widths to calculate end point and increment start point.
The TextDecorationPainter class is responsible to call
GraphicsContextJava::drawLinesForText. Before that
GraphicsContextJava::drawLinesForText the TextDecorationPainter class compute
the rect based on the text decoration style, text origin.
Before calling GraphicsContextJava::drawLinesForText, The
TextDecorationPainter class creates arrays of intersection points from rect.
// using DashArray = Vector;
DashArray boundaries =
translateIntersectionPointsToSkipInkBoundaries(intersections,
underlineBoundingBox.height(), rect.width());
m_context.drawLinesForText(rect.location(), rect.height(), boundaries,
m_isPrinting, style == TextDecorationStyle::Double, strokeStyle);
So following code as fix
for (const auto& width : widths) {
FloatPoint endPoint = startPoint + FloatPoint(width, 0);
drawLine(
IntPoint(startPoint.x(), startPoint.y()),
IntPoint(endPoint.x(), endPoint.y()));
startPoint = endPoint;
}
avoid arithmetic overflow as in previous fix we were calculating end point
FloatPoint endPoint = startPoint + FloatPoint(widths.last(), 0);
I have tested all style of line drawing , continuous , dashes , overflow and
underline , it is working well with the recent fix.
-
PR: https://git.openjdk.java.net/jfx/pull/765
RFR: 8284184: Crash in GraphicsContextJava::drawLinesForText on https://us.yahoo.com/
Issue: Floating point overflow , when making end point for line drawing as FloatPoint endPoint = startPoint + FloatPoint(widths.last(), 0); Solution: traverse widths to calculate end point and increment start point. - Commit messages: - Merge remote-tracking branch 'upstream/master' - 8284184: Crash in GraphicsContextJava::drawLinesForText on https://us.yahoo.com Changes: https://git.openjdk.java.net/jfx/pull/765/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=765=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284184 Stats: 7 lines in 1 file changed: 3 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/765.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/765/head:pull/765 PR: https://git.openjdk.java.net/jfx/pull/765
Re: RFR: 8270867: Debug build of WebKit 613.1 fails on Linux
On Wed, 16 Mar 2022 07:54:57 GMT, Peter Zhelezniakov wrote: > The file `wtf/linux/MemoryPressureLinux.cpp` was renamed > `wtf/unix/MemoryPressureUnix.cpp` in https://trac.webkit.org/changeset/261428 > . In the openjfx workspace, both files are present. I'm removing the old file. Since there are two equivalent files for same purpose , it looks good to remove older one. - Marked as reviewed by jbhaskar (Author). PR: https://git.openjdk.java.net/jfx/pull/756
Re: RFR: 8270867: Debug build of WebKit 613.1 fails on Linux
On Wed, 16 Mar 2022 07:54:57 GMT, Peter Zhelezniakov wrote: > The file `wtf/linux/MemoryPressureLinux.cpp` was renamed > `wtf/unix/MemoryPressureUnix.cpp` in https://trac.webkit.org/changeset/261428 > . In the openjfx workspace, both files are present. I'm removing the old file. Yes , The PR looks good. We can file new bug for ruby version related issue. - PR: https://git.openjdk.java.net/jfx/pull/756
Re: RFR: 8270867: Debug build of WebKit 613.1 fails on Linux
On Wed, 16 Mar 2022 07:54:57 GMT, Peter Zhelezniakov wrote: > The file `wtf/linux/MemoryPressureLinux.cpp` was renamed > `wtf/unix/MemoryPressureUnix.cpp` in https://trac.webkit.org/changeset/261428 > . In the openjfx workspace, both files are present. I'm removing the old file. The patch https://trac.webkit.org/changeset/261428 committed in May 8, 2020. it is older than a recent patch https://bugs.webkit.org/attachment.cgi?id=443979=prettypatch But i remember , the error for this file [MemoryPressureHandlerLinux.cpp] was at if (ReliefLogger::loggingEnabled() && isUnderMemoryPressure()) LOG(MemoryPressure, "System is no longer under memory pressure."); The variable isUnderMemoryPressure was giving error. saying right variable m_underMemoryPressure instead of isUnderMemoryPressure Error: [jfx/modules/javafx.web/src/main/native/Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:39:28: error: 'LogMemoryPressure' was not declared in this scope; did you mean 'isUnderMemoryPressure'? ] There was anothe issue with the build, as below [JavaScriptCore/Scripts/resolve-asm-file-conflicts.rb](http://rejfx.us.oracle.com:8080/job/jfx-submit/label=lin-ol7-x64/ws/jfx/rt/modules/javafx.web/build/linux/Debug/JavaScriptCore/Scripts/resolve-asm-file-conflicts.rb)>:99:in `parse': undefined method `/' for This was removed by updating rub version. - PR: https://git.openjdk.java.net/jfx/pull/756
Integrated: 8280020: Underline and line-through not straight in WebView
On Thu, 10 Feb 2022 11:36:38 GMT, Jay Bhaskar wrote: > Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) This pull request has now been integrated. Changeset: 263db3df Author:Jay Bhaskar Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/263db3df5fdf9e0f6955be6ae58173aa589e55fe Stats: 192 lines in 2 files changed: 182 ins; 2 del; 8 mod 8280020: Underline and line-through not straight in WebView Reviewed-by: kcr, arapte - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v8]
> Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: formating for drawline - Changes: - all: https://git.openjdk.java.net/jfx/pull/731/files - new: https://git.openjdk.java.net/jfx/pull/731/files/355c798b..e7d39427 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=731=07 - incr: https://webrevs.openjdk.java.net/?repo=jfx=731=06-07 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/731.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/731/head:pull/731 PR: https://git.openjdk.java.net/jfx/pull/731
Integrated: 8269115: WebView paste event contains old data
On Tue, 8 Feb 2022 11:13:20 GMT, Jay Bhaskar wrote: > Issue: The DataObject uses m_availMimeTypes as Vector of strings, and > appending mime types in pasteboard operation like setPlainText, Hence it will > append duplicate mime types in m_availMimeTypes , in this case the length > clipboardData.types would be wrong, and duplicates data would be copied to > clipboard target. > Solution: Use ListHashSet data Structure instead of Vector for > m_availMimeTypes. This pull request has now been integrated. Changeset: 2338821b Author:Jay Bhaskar Committer: Ambarish Rapte URL: https://git.openjdk.java.net/jfx/commit/2338821bdbb7db929a89aa89903271dcff333a5c Stats: 120 lines in 3 files changed: 108 ins; 3 del; 9 mod 8269115: WebView paste event contains old data Reviewed-by: arapte, kcr - PR: https://git.openjdk.java.net/jfx/pull/729
Integrated: 8255940: localStorage is null after window.close()
On Mon, 27 Dec 2021 09:31:08 GMT, Jay Bhaskar wrote: > Issue: The current implementation of DOMWindow ::localStorage(..) returns > null pointer in case of page is being closed. > Fix: It should not return nullptr , as per the [w3c web storage > spec](https://www.w3.org/TR/2016/REC-webstorage-20160419/) > "User agents should expire data from the local storage areas only for > security reasons or when requested to do so by the user. User agents should > always avoid deleting data while a script that could access that data is > running." This pull request has now been integrated. Changeset: 5112be95 Author:Jay Bhaskar Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/5112be957be70dd6521e6fb6ee64e669c148729c Stats: 168 lines in 4 files changed: 167 ins; 0 del; 1 mod 8255940: localStorage is null after window.close() Reviewed-by: kcr, arapte - PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v9]
> Issue: The current implementation of DOMWindow ::localStorage(..) returns > null pointer in case of page is being closed. > Fix: It should not return nullptr , as per the [w3c web storage > spec](https://www.w3.org/TR/2016/REC-webstorage-20160419/) > "User agents should expire data from the local storage areas only for > security reasons or when requested to do so by the user. User agents should > always avoid deleting data while a script that could access that data is > running." Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Added review change JAVA - Changes: - all: https://git.openjdk.java.net/jfx/pull/703/files - new: https://git.openjdk.java.net/jfx/pull/703/files/1b4d7751..613cc00f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=703=08 - incr: https://webrevs.openjdk.java.net/?repo=jfx=703=07-08 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/703.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/703/head:pull/703 PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v8]
On Thu, 3 Mar 2022 17:08:49 GMT, Jay Bhaskar wrote:
>> Issue: The current implementation of DOMWindow ::localStorage(..) returns
>> null pointer in case of page is being closed.
>> Fix: It should not return nullptr , as per the [w3c web storage
>> spec](https://www.w3.org/TR/2016/REC-webstorage-20160419/)
>> "User agents should expire data from the local storage areas only for
>> security reasons or when requested to do so by the user. User agents should
>> always avoid deleting data while a script that could access that data is
>> running."
>
> Jay Bhaskar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Restore original , and new changes wrapped with define JAVA
Added
#if !PLATFORM(JAVA)
// FIXME: We should consider supporting access/modification to local storage
// after calling window.close(). See
<https://bugs.webkit.org/show_bug.cgi?id=135330>.
if (!page || !page->isClosing()) {
if (m_localStorage)
return m_localStorage.get();
}
#endif
-
PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8280020: Underline and line-through not straight in WebView [v7]
> Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) Jay Bhaskar has updated the pull request incrementally with two additional commits since the last revision: - test needs width and height , during creattion of webview - test needs width and height , during creattion of webview - Changes: - all: https://git.openjdk.java.net/jfx/pull/731/files - new: https://git.openjdk.java.net/jfx/pull/731/files/5ab81018..355c798b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=731=06 - incr: https://webrevs.openjdk.java.net/?repo=jfx=731=05-06 Stats: 5 lines in 1 file changed: 0 ins; 4 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/731.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/731/head:pull/731 PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8269115: WebView paste event contains old data [v7]
> Issue: The DataObject uses m_availMimeTypes as Vector of strings, and > appending mime types in pasteboard operation like setPlainText, Hence it will > append duplicate mime types in m_availMimeTypes , in this case the length > clipboardData.types would be wrong, and duplicates data would be copied to > clipboard target. > Solution: Use ListHashSet data Structure instead of Vector for > m_availMimeTypes. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: removed automated test HTMLClipBoardTest.java, as manual test added - Changes: - all: https://git.openjdk.java.net/jfx/pull/729/files - new: https://git.openjdk.java.net/jfx/pull/729/files/2b1c9eef..382decf6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=729=06 - incr: https://webrevs.openjdk.java.net/?repo=jfx=729=05-06 Stats: 97 lines in 1 file changed: 0 ins; 97 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/729.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/729/head:pull/729 PR: https://git.openjdk.java.net/jfx/pull/729
Re: RFR: 8269115: WebView paste event contains old data [v6]
> Issue: The DataObject uses m_availMimeTypes as Vector of strings, and > appending mime types in pasteboard operation like setPlainText, Hence it will > append duplicate mime types in m_availMimeTypes , in this case the length > clipboardData.types would be wrong, and duplicates data would be copied to > clipboard target. > Solution: Use ListHashSet data Structure instead of Vector for > m_availMimeTypes. Jay Bhaskar has updated the pull request incrementally with five additional commits since the last revision: - added manual test - Add new Test case for copy and paste clipboard - Add new Test case for copy and paste clipboard - new test case - new test case - Changes: - all: https://git.openjdk.java.net/jfx/pull/729/files - new: https://git.openjdk.java.net/jfx/pull/729/files/aa75ea4e..2b1c9eef Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=729=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx=729=04-05 Stats: 230 lines in 3 files changed: 204 ins; 16 del; 10 mod Patch: https://git.openjdk.java.net/jfx/pull/729.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/729/head:pull/729 PR: https://git.openjdk.java.net/jfx/pull/729
Re: RFR: 8255940: localStorage is null after window.close() [v7]
On Tue, 1 Mar 2022 10:00:58 GMT, Ambarish Rapte wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Clean up test and fix
>
> modules/javafx.web/src/main/native/Source/WebCore/page/DOMWindow.cpp line 850:
>
>> 848: return Exception { SecurityError };
>> 849:
>> 850: if (m_localStorage)
>
> Let's enclose this change in `#if PLATFORM(JAVA)` macro. It makes easy to
> identify Java platform specific changes and handle merging when updating
> WebKit.
done
> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
> line 49:
>
>> 47: import javafx.concurrent.Worker;
>> 48: import javafx.scene.web.WebView;
>> 49: import javafx.scene.web.WebEngine;
>
> Several import statements can be removed. Only below imports are sufficient
> to run the test, others can be removed.
>
> import static org.junit.Assert.assertEquals;
> import static org.junit.Assert.assertTrue;
> import static org.junit.Assert.assertNotNull;
> import org.junit.AfterClass;
> import org.junit.Test;
>
> import java.io.File;
> import java.io.IOException;
>
> import javafx.scene.web.WebView;
> import javafx.scene.web.WebEngine;
done
-
PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v8]
> Issue: The current implementation of DOMWindow ::localStorage(..) returns > null pointer in case of page is being closed. > Fix: It should not return nullptr , as per the [w3c web storage > spec](https://www.w3.org/TR/2016/REC-webstorage-20160419/) > "User agents should expire data from the local storage areas only for > security reasons or when requested to do so by the user. User agents should > always avoid deleting data while a script that could access that data is > running." Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Restore original , and new changes wrapped with define JAVA - Changes: - all: https://git.openjdk.java.net/jfx/pull/703/files - new: https://git.openjdk.java.net/jfx/pull/703/files/8ba34a5d..1b4d7751 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=703=07 - incr: https://webrevs.openjdk.java.net/?repo=jfx=703=06-07 Stats: 27 lines in 2 files changed: 13 ins; 11 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/703.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/703/head:pull/703 PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8280020: Underline and line-through not straight in WebView [v6]
> Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: PR review changes applied - Changes: - all: https://git.openjdk.java.net/jfx/pull/731/files - new: https://git.openjdk.java.net/jfx/pull/731/files/29916450..5ab81018 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=731=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx=731=04-05 Stats: 5 lines in 1 file changed: 1 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/731.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/731/head:pull/731 PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v5]
On Mon, 28 Feb 2022 15:12:37 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> New chnages for line test
>
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 168:
>
>> 166: int width =
>> (int)webView.getEngine().executeScript("document.getElementsByTagName('div')[0].getBoundingClientRect().width");
>> 167:
>> 168: int line_start_x = start_x;
>
> You might want to add a small amount (`DELTA`?) to avoid sampling at the edge.
In test html , the margin and padding is 0 px. it would be ok to add DELTA, done
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 173:
>
>> 171: String line_color = "rgba(0,0,0,255)"; // color of line
>> 172: System.out.println(line_end_x);
>> 173: System.out.println(line_start_y);
>
> Once you are done debugging the test, you can remove these print statements.
ok,
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 177:
>
>> 175: for (int x = line_start_x; x < line_end_x; x++) {
>> 176: String color = colorToString(pr.getColor(x,
>> line_start_y));
>> 177: assertEquals(color, line_color);
>
> The arguments are backwards (the expected value goes first).
done
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v5]
On Mon, 28 Feb 2022 15:10:27 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> New chnages for line test > > tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line > 57: > >> 55: private static final CountDownLatch launchLatch = new >> CountDownLatch(1); >> 56: private static final int LINE_THICKNESS = 20; >> 57: private static final int SKIP_TEXT_BOUNDARY = 32; > > What does this constant represent? LINE_THICKNESS is thickness that is also in html test code. SKIP_TEXT_BOUNDARY is used to skip boundary at the end of line. - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8255940: localStorage is null after window.close() [v7]
> Issue: The current implementation of DOMWindow ::localStorage(..) returns > null pointer in case of page is being closed. > Fix: It should not return nullptr , as per the [w3c web storage > spec](https://www.w3.org/TR/2016/REC-webstorage-20160419/) > "User agents should expire data from the local storage areas only for > security reasons or when requested to do so by the user. User agents should > always avoid deleting data while a script that could access that data is > running." Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Clean up test and fix - Changes: - all: https://git.openjdk.java.net/jfx/pull/703/files - new: https://git.openjdk.java.net/jfx/pull/703/files/f320a013..8ba34a5d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=703=06 - incr: https://webrevs.openjdk.java.net/?repo=jfx=703=05-06 Stats: 3 lines in 2 files changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/703.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/703/head:pull/703 PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v5]
On Mon, 28 Feb 2022 14:53:39 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add Review Changes
>
> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
> line 55:
>
>> 53:
>> 54: private static final File LOCAL_STORAGE_DIR = new
>> File("LocalStorageDir");
>> 55: private static final File PRE_LOCKED = new File("zoo_local_storage");
>
>> LOCAL_STORAGE_DIR is used by web engine , to store local data in this case
>
> Yes, `LOCAL_STORAGE_DIR` is needed. I only meant that `PRE_LOCKED` was
> unused. GitHub decided to highlight both lines.
done
-
PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v6]
On Mon, 28 Feb 2022 13:58:03 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add Review Changes > > modules/javafx.web/src/main/native/Source/WebCore/page/DOMWindow.cpp line 851: > >> 849: >> 850: // FIXME: We should consider supporting access/modification to >> local storage >> 851: // after calling window.close(). See >> <https://bugs.webkit.org/show_bug.cgi?id=135330>. > > The change of moving this block back here looks good. You can delete the > obsolete comment now, right? agree > modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java > line 142: > >> 140: }); >> 141: } >> 142: } > > Minor: can you restore the newline at the end of this file? done - PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8278759 : PointerEvent: buttons property set to 0 when mouse down [v3]
On Fri, 25 Feb 2022 17:54:48 GMT, Hima Bindu Meda wrote: >> Basically, buttons property is a mask which represents the button/buttons >> clicked on the mouse. >> It is observed that event.buttons property is set to 0 when there is >> mouse press or drag event.This behaviour is observed only with javafx >> webView.Other browsers set the buttons property to 1, when there is mouse >> press or drag. >> The issue happens because the buttons property is not updated in the >> framework. >> Added implementation to update and propagate the buttons property from >> javafx platform to native webkit.Added a robot test case for the same. >>Performed sanity testing with the added implementation and the buttons >> property is compliant with the specification mentioned in >> https://w3c.github.io/pointerevents/#the-buttons-property. > > Hima Bindu Meda has updated the pull request incrementally with one > additional commit since the last revision: > > Use ESC key so that the popup, if any, disappears + looks good - Marked as reviewed by jaybhas...@github.com (no known OpenJDK username). PR: https://git.openjdk.java.net/jfx/pull/742
Re: RFR: 8280020: Underline and line-through not straight in WebView [v5]
> Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: New chnages for line test - Changes: - all: https://git.openjdk.java.net/jfx/pull/731/files - new: https://git.openjdk.java.net/jfx/pull/731/files/3246375d..29916450 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=731=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx=731=03-04 Stats: 30 lines in 1 file changed: 9 ins; 7 del; 14 mod Patch: https://git.openjdk.java.net/jfx/pull/731.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/731/head:pull/731 PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8255940: localStorage is null after window.close() [v6]
> Issue: The current implementation of DOMWindow ::localStorage(..) returns > null pointer in case of page is being closed. > Fix: It should not return nullptr , as per the [w3c web storage > spec](https://www.w3.org/TR/2016/REC-webstorage-20160419/) > "User agents should expire data from the local storage areas only for > security reasons or when requested to do so by the user. User agents should > always avoid deleting data while a script that could access that data is > running." Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Add Review Changes - Changes: - all: https://git.openjdk.java.net/jfx/pull/703/files - new: https://git.openjdk.java.net/jfx/pull/703/files/ef7587c2..f320a013 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=703=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx=703=04-05 Stats: 7 lines in 1 file changed: 0 ins; 7 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/703.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/703/head:pull/703 PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v6]
On Sat, 5 Feb 2022 14:57:50 GMT, Kevin Rushforth wrote:
>> modules/javafx.web/src/main/native/Source/WebCore/page/DOMWindow.cpp line
>> 857:
>>
>>> 855: return m_localStorage.get();
>>> 856: }
>>> 857:
>>
>> This will change the behavior for the case where page is null or where the
>> page is valid, but not closing. I think you should partially revert this
>> part of the fix, restoring it as follows:
>>
>>
>> if (m_localStorage)
>> return m_localStorage.get();
>
> I still think you need to restore this block, but without the check for
> `isClosing`.
Kindly look code
ExceptionOr DOMWindow::localStorage()
{
if (!isCurrentlyDisplayedInFrame())
return nullptr;
RefPtr document = this->document();
if (!document)
return nullptr;
if (!document->securityOrigin().canAccessLocalStorage(nullptr))
return Exception { SecurityError };
// FIXME: We should consider supporting access/modification to local storage
// after calling window.close(). See
.
if (m_localStorage)
return m_localStorage.get();
auto* page = document->page();
if (!page)
return nullptr;
// Check if localstorage setting is disabled, then return nullptr
if (!page->settings().localStorageEnabled())
return nullptr;
auto storageArea = page->storageNamespaceProvider().localStorageArea(*document);
m_localStorage = Storage::create(*this, WTFMove(storageArea));
return m_localStorage.get();
}
is it now ok?
-
PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v3]
On Sun, 27 Feb 2022 05:25:25 GMT, Jay Bhaskar wrote:
>> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
>> line 55:
>>
>>> 53:
>>> 54: private static final File LOCAL_STORAGE_DIR = new
>>> File("LocalStorageDir");
>>> 55: private static final File PRE_LOCKED = new
>>> File("zoo_local_storage");
>>
>> This looks like a copy / paste from `UserDataDirectoryTest`, and is unused
>> (and not needed) in this test.
>
> LOCAL_STORAGE_DIR is used by web engine , to store local data in this case
private static final File LOCAL_STORAGE_DIR = new File("LocalStorageDir"); is
required, and i agree
not require for private static final File PRE_LOCKED = new
File("zoo_local_storage");
-
PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v5]
> Issue: The current implementation of DOMWindow ::localStorage(..) returns > null pointer in case of page is being closed. > Fix: It should not return nullptr , as per the [w3c web storage > spec](https://www.w3.org/TR/2016/REC-webstorage-20160419/) > "User agents should expire data from the local storage areas only for > security reasons or when requested to do so by the user. User agents should > always avoid deleting data while a script that could access that data is > running." Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Add Review Changes - Changes: - all: https://git.openjdk.java.net/jfx/pull/703/files - new: https://git.openjdk.java.net/jfx/pull/703/files/3f6e815e..ef7587c2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=703=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx=703=03-04 Stats: 14 lines in 1 file changed: 0 ins; 13 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/703.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/703/head:pull/703 PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v3]
On Fri, 25 Feb 2022 23:15:54 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - Change Dir Path and use local Dir and set data before clearing
>> localstorage test
>> - Merge branch 'PRLocalstorage' of https://github.com/jaybhaskar/jfx into
>> PRLocalstorage
>> - Merge branch 'master' into PRLocalstorage
>
> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
> line 59:
>
>> 57: private static RandomAccessFile preLockedRaf;
>> 58: private static FileLock preLockedLock;
>> 59: private static final Random random = new Random();
>
> These are not needed.
Agree
> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
> line 112:
>
>> 110: final WebEngine webEngine = getEngine();
>> 111: webEngine.setJavaScriptEnabled(true);
>> 112: webEngine.setUserDataDirectory(LOCAL_STORAGE_DIR);
>
> This should be done for all tests, not just this one. Remember that you
> shouldn't assume any particular order for the tests (tests are intended to be
> stateless).
Agree, done
> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
> line 126:
>
>> 124: //get data
>> 125: String s = (String)
>> view.getEngine().executeScript("document.getElementById('key').innerText;");
>> 126: assertEquals(s, "1001");
>
> The arguments should be switched (expected comes first).
Agree , done
> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
> line 137:
>
>> 135: view.getEngine().executeScript("test_local_storage_set();");
>> 136: String s = (String)
>> view.getEngine().executeScript("document.getElementById('key').innerText;");
>> 137: assertEquals(s, "1001");
>
> Switch arguments.
Agree, done
-
PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v3]
On Sun, 27 Feb 2022 05:30:44 GMT, Jay Bhaskar wrote: >> Agree > > Agree, done Agree - PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v3]
On Sun, 27 Feb 2022 05:30:34 GMT, Jay Bhaskar wrote: >> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java >> line 59: >> >>> 57: private static RandomAccessFile preLockedRaf; >>> 58: private static FileLock preLockedLock; >>> 59: private static final Random random = new Random(); >> >> These are not needed. > > Agree Agree, done - PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v3]
On Fri, 25 Feb 2022 23:09:00 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - Change Dir Path and use local Dir and set data before clearing
>> localstorage test
>> - Merge branch 'PRLocalstorage' of https://github.com/jaybhaskar/jfx into
>> PRLocalstorage
>> - Merge branch 'master' into PRLocalstorage
>
> modules/javafx.web/src/main/native/Source/WebCore/page/DOMWindow.cpp line 861:
>
>> 859: // after calling window.close(). See
>> <https://bugs.webkit.org/show_bug.cgi?id=135330>.
>> 860: if (m_localStorage)
>> 861: return m_localStorage.get();
>
> This is not needed here if you take my suggestion of doing it above.
Kindly look code
ExceptionOr DOMWindow::localStorage()
{
if (!isCurrentlyDisplayedInFrame())
return nullptr;
RefPtr document = this->document();
if (!document)
return nullptr;
if (!document->securityOrigin().canAccessLocalStorage(nullptr))
return Exception { SecurityError };
// FIXME: We should consider supporting access/modification to local storage
// after calling window.close(). See
<https://bugs.webkit.org/show_bug.cgi?id=135330>.
if (m_localStorage)
return m_localStorage.get();
auto* page = document->page();
if (!page)
return nullptr;
// Check if localstorage setting is disabled, then return nullptr
if (!page->settings().localStorageEnabled())
return nullptr;
auto storageArea =
page->storageNamespaceProvider().localStorageArea(*document);
m_localStorage = Storage::create(*this, WTFMove(storageArea));
return m_localStorage.get();
}
is it now ok?
> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
> line 55:
>
>> 53:
>> 54: private static final File LOCAL_STORAGE_DIR = new
>> File("LocalStorageDir");
>> 55: private static final File PRE_LOCKED = new File("zoo_local_storage");
>
> This looks like a copy / paste from `UserDataDirectoryTest`, and is unused
> (and not needed) in this test.
LOCAL_STORAGE_DIR is used by web engine , to store local data in this case
-
PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v4]
> Issue: The current implementation of DOMWindow ::localStorage(..) returns > null pointer in case of page is being closed. > Fix: It should not return nullptr , as per the [w3c web storage > spec](https://www.w3.org/TR/2016/REC-webstorage-20160419/) > "User agents should expire data from the local storage areas only for > security reasons or when requested to do so by the user. User agents should > always avoid deleting data while a script that could access that data is > running." Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Add Review Changes - Changes: - all: https://git.openjdk.java.net/jfx/pull/703/files - new: https://git.openjdk.java.net/jfx/pull/703/files/a8647839..3f6e815e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=703=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=703=02-03 Stats: 21 lines in 2 files changed: 14 ins; 5 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/703.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/703/head:pull/703 PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8269115: WebView paste event contains old data [v4]
On Fri, 25 Feb 2022 07:11:49 GMT, Jay Bhaskar wrote: >> Issue: The DataObject uses m_availMimeTypes as Vector of strings, and >> appending mime types in pasteboard operation like setPlainText, Hence it >> will append duplicate mime types in m_availMimeTypes , in this case the >> length clipboardData.types would be wrong, and duplicates data would be >> copied to clipboard target. >> Solution: Use ListHashSet data Structure instead of Vector for >> m_availMimeTypes. > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > Test regression according to bug All , requested changes applied. - PR: https://git.openjdk.java.net/jfx/pull/729
Re: RFR: 8269115: WebView paste event contains old data [v5]
> Issue: The DataObject uses m_availMimeTypes as Vector of strings, and > appending mime types in pasteboard operation like setPlainText, Hence it will > append duplicate mime types in m_availMimeTypes , in this case the length > clipboardData.types would be wrong, and duplicates data would be copied to > clipboard target. > Solution: Use ListHashSet data Structure instead of Vector for > m_availMimeTypes. Jay Bhaskar has updated the pull request incrementally with two additional commits since the last revision: - formating change - formating change - Changes: - all: https://git.openjdk.java.net/jfx/pull/729/files - new: https://git.openjdk.java.net/jfx/pull/729/files/dded44a5..aa75ea4e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=729=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx=729=03-04 Stats: 7 lines in 1 file changed: 0 ins; 2 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/729.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/729/head:pull/729 PR: https://git.openjdk.java.net/jfx/pull/729
Re: RFR: 8269115: WebView paste event contains old data [v4]
> Issue: The DataObject uses m_availMimeTypes as Vector of strings, and > appending mime types in pasteboard operation like setPlainText, Hence it will > append duplicate mime types in m_availMimeTypes , in this case the length > clipboardData.types would be wrong, and duplicates data would be copied to > clipboard target. > Solution: Use ListHashSet data Structure instead of Vector for > m_availMimeTypes. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Test regression according to bug - Changes: - all: https://git.openjdk.java.net/jfx/pull/729/files - new: https://git.openjdk.java.net/jfx/pull/729/files/d4da4ee5..dded44a5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=729=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=729=02-03 Stats: 31 lines in 1 file changed: 13 ins; 4 del; 14 mod Patch: https://git.openjdk.java.net/jfx/pull/729.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/729/head:pull/729 PR: https://git.openjdk.java.net/jfx/pull/729
Re: RFR: 8269115: WebView paste event contains old data [v3]
> Issue: The DataObject uses m_availMimeTypes as Vector of strings, and > appending mime types in pasteboard operation like setPlainText, Hence it will > append duplicate mime types in m_availMimeTypes , in this case the length > clipboardData.types would be wrong, and duplicates data would be copied to > clipboard target. > Solution: Use ListHashSet data Structure instead of Vector for > m_availMimeTypes. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Test case chnaged - Changes: - all: https://git.openjdk.java.net/jfx/pull/729/files - new: https://git.openjdk.java.net/jfx/pull/729/files/129f437b..d4da4ee5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=729=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=729=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/729.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/729/head:pull/729 PR: https://git.openjdk.java.net/jfx/pull/729
Re: RFR: 8269115: WebView paste event contains old data [v2]
On Thu, 24 Feb 2022 16:05:41 GMT, Jay Bhaskar wrote: >> Issue: The DataObject uses m_availMimeTypes as Vector of strings, and >> appending mime types in pasteboard operation like setPlainText, Hence it >> will append duplicate mime types in m_availMimeTypes , in this case the >> length clipboardData.types would be wrong, and duplicates data would be >> copied to clipboard target. >> Solution: Use ListHashSet data Structure instead of Vector for >> m_availMimeTypes. > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > PR commnets applied The commit was before the latest webkit update , yes the test pass without fix, i will investigate and apply the required change - PR: https://git.openjdk.java.net/jfx/pull/729
Re: RFR: 8269115: WebView paste event contains old data [v2]
> Issue: The DataObject uses m_availMimeTypes as Vector of strings, and > appending mime types in pasteboard operation like setPlainText, Hence it will > append duplicate mime types in m_availMimeTypes , in this case the length > clipboardData.types would be wrong, and duplicates data would be copied to > clipboard target. > Solution: Use ListHashSet data Structure instead of Vector for > m_availMimeTypes. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: PR commnets applied - Changes: - all: https://git.openjdk.java.net/jfx/pull/729/files - new: https://git.openjdk.java.net/jfx/pull/729/files/62d16a39..129f437b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=729=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=729=00-01 Stats: 4 lines in 3 files changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/729.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/729/head:pull/729 PR: https://git.openjdk.java.net/jfx/pull/729
Re: RFR: 8269115: WebView paste event contains old data
On Thu, 24 Feb 2022 12:42:49 GMT, Ambarish Rapte wrote: >> Issue: The DataObject uses m_availMimeTypes as Vector of strings, and >> appending mime types in pasteboard operation like setPlainText, Hence it >> will append duplicate mime types in m_availMimeTypes , in this case the >> length clipboardData.types would be wrong, and duplicates data would be >> copied to clipboard target. >> Solution: Use ListHashSet data Structure instead of Vector for >> m_availMimeTypes. > > modules/javafx.web/src/main/native/Source/WebCore/platform/java/DataObjectJava.h > line 133: > >> 131: Vector types; >> 132: types.appendRange(m_availMimeTypes.begin(), >> m_availMimeTypes.end()); >> 133: //returns MIME Types available in clipboard.z > > Minor: The comment should be the first line in the method, as it was before > this. > The character z at the end must be unintended. done > modules/javafx.web/src/main/native/Source/WebCore/platform/java/PasteboardJava.cpp > line 240: > >> 238: // TODO: setURL, setFiles, setData, setHtml (needs URL) >> 239: data->setPlainText(jGetPlainText()); >> 240: data->setData(DataObjectJava::mimeHTML(),jGetPlainText()); > > Minor: Please add a space after `,` done > modules/javafx.web/src/test/java/test/javafx/scene/web/HTMLEditingTest.java > line 73: > >> 71: Clipboard.getSystemClipboard().setContent(content); >> 72: >> 73: > > Minor: The extra line is not required and would recommend to remove. done - PR: https://git.openjdk.java.net/jfx/pull/729
Integrated: 8282134: Certain regex can cause a JS trap in WebView
On Sat, 19 Feb 2022 11:43:00 GMT, Jay Bhaskar wrote: > Fix YarrJIT backtrackCharacterClassNonGreedy breakpoint() Certain regex can > cause a JS trap in WebView This pull request has now been integrated. Changeset: 73963960 Author: Jay Bhaskar Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/73963960dc6e56fe34d7aa5fb4ce6f6d2f07acc5 Stats: 22 lines in 3 files changed: 21 ins; 0 del; 1 mod 8282134: Certain regex can cause a JS trap in WebView Reviewed-by: kcr, arapte - PR: https://git.openjdk.java.net/jfx/pull/739
Re: RFR: 8282134: Certain regex can cause a JS trap in WebView [v3]
On Tue, 22 Feb 2022 12:43:37 GMT, Jay Bhaskar wrote: >> Fix YarrJIT backtrackCharacterClassNonGreedy breakpoint() Certain regex can >> cause a JS trap in WebView > > Jay Bhaskar has refreshed the contents of this pull request, and previous > commits have been removed. Incremental views are not available. The pull > request now contains one commit: > > 8282134: Certain regex can cause a JS trap in WebView Author name Change , due to bot comment ⚠️ @jaybhaskar the full name on your profile does not match the author name in this pull requests' [HEAD](https://git.openjdk.java.net/jfx/pull/739/commits/6319c639e244f20e9ff6846c9bdcd04fb189d89d) commit. If this pull request gets integrated then the author name from this pull requests' [HEAD](https://git.openjdk.java.net/jfx/pull/739/commits/6319c639e244f20e9ff6846c9bdcd04fb189d89d) commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork: $ git checkout public-sec-bug $ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name' $ git push - PR: https://git.openjdk.java.net/jfx/pull/739
Re: RFR: 8282134: Certain regex can cause a JS trap in WebView [v3]
> Fix YarrJIT backtrackCharacterClassNonGreedy breakpoint() Certain regex can > cause a JS trap in WebView Jay Bhaskar has refreshed the contents of this pull request, and previous commits have been removed. Incremental views are not available. The pull request now contains one commit: 8282134: Certain regex can cause a JS trap in WebView - Changes: - all: https://git.openjdk.java.net/jfx/pull/739/files - new: https://git.openjdk.java.net/jfx/pull/739/files/a041e77e..3fad1fc9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=739=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=739=01-02 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/739.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/739/head:pull/739 PR: https://git.openjdk.java.net/jfx/pull/739
Re: RFR: 8282134: Certain regex can cause a JS trap in WebView [v2]
On Tue, 22 Feb 2022 10:03:30 GMT, Jay Bhaskar wrote: >> Fix YarrJIT backtrackCharacterClassNonGreedy breakpoint() Certain regex can >> cause a JS trap in WebView > > Jay Bhaskar has updated the pull request incrementally with two additional > commits since the last revision: > > - Merge branch 'public-sec-bug' of https://github.com/jaybhaskar/jfx into > public-sec-bug > - 8282134: Certain regex can cause a JS trap in WebView git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name' command error out as below fatal: Option -m cannot be combined with -c/-C/-F. - PR: https://git.openjdk.java.net/jfx/pull/739
Re: RFR: 8282134: Certain regex can cause a JS trap in WebView [v2]
> Fix YarrJIT backtrackCharacterClassNonGreedy breakpoint() Certain regex can > cause a JS trap in WebView Jay Bhaskar has updated the pull request incrementally with two additional commits since the last revision: - Merge branch 'public-sec-bug' of https://github.com/jaybhaskar/jfx into public-sec-bug - 8282134: Certain regex can cause a JS trap in WebView - Changes: - all: https://git.openjdk.java.net/jfx/pull/739/files - new: https://git.openjdk.java.net/jfx/pull/739/files/6319c639..a041e77e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=739=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=739=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/739.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/739/head:pull/739 PR: https://git.openjdk.java.net/jfx/pull/739
Re: RFR: 8280020: Underline and line-through not straight in WebView [v4]
> Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Improve style and width iterartion logic - Changes: - all: https://git.openjdk.java.net/jfx/pull/731/files - new: https://git.openjdk.java.net/jfx/pull/731/files/135a073b..3246375d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=731=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=731=02-03 Stats: 13 lines in 1 file changed: 4 ins; 1 del; 8 mod Patch: https://git.openjdk.java.net/jfx/pull/731.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/731/head:pull/731 PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Sat, 19 Feb 2022 15:03:38 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update test case for line straightness check
>
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 167:
>
>> 165: int line_start_y = start_y + height;
>> 166: String line_color = "rgba(0,0,0,255)"; // color of line
>> 167: for (int i = line_start_y; i < snapshot.getHeight(); i++) {
>
> The snapshot height is irrelevant. You should use thickness minus 6 (3 pixels
> on each of the top and bottom):
>
>
> i < line_start_y + thickness - 6;
I think it should be likefor (int i = line_start_y; i <= (width -
line_start_y -6); i++) , as we need to iterate in x direction
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Mon, 21 Feb 2022 04:19:29 GMT, Jay Bhaskar wrote: >> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line >> 163: >> >>> 161: >>> 162: // buttom start x position of underline ( startx + font >>> size + thickness -1) >>> 163: int line_start_x = start_x + height + 20 - 1; >> >> The x value shouldn't be affected by thickness or the height. This should be >> something like `start_x + 3` (the `+3` is so you don't sample anywhere near >> the edge, to avoid boundary problems). >> >> I also recommend sampling near the right edge to catch the problem of a >> slanted line, so: `int line_end_x = start_x + width - 3;` > > The bounding rect.x rect.y is top left corner, and line is being drawn below > the bottom, so height and thickness need to be considered. int i = line_start_y; i < (line_start_y - 6); i++ , this meets the condition of sampling near the right edge - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Sat, 19 Feb 2022 14:55:30 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update test case for line straightness check > > tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line > 163: > >> 161: >> 162: // buttom start x position of underline ( startx + font >> size + thickness -1) >> 163: int line_start_x = start_x + height + 20 - 1; > > The x value shouldn't be affected by thickness or the height. This should be > something like `start_x + 3` (the `+3` is so you don't sample anywhere near > the edge, to avoid boundary problems). > > I also recommend sampling near the right edge to catch the problem of a > slanted line, so: `int line_end_x = start_x + width - 3;` The bounding rect.x rect.y is top left corner, and line is being drawn below the bottom, so height and thickness need to be considered. - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Sat, 19 Feb 2022 14:45:53 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update test case for line straightness check > > tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line > 75: > >> 73: this.primaryStage = primaryStage; >> 74: this.primaryStage.setWidth(80); >> 75: this.primaryStage.setHeight(60); > > Minor: Since you set the size of the Scene later on, you don't need to set it > here. This would be remain same , i would not set size later - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Sat, 19 Feb 2022 16:51:52 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update test case for line straightness check
>
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 172:
>
>> 170: continue;
>> 171: else
>> 172: fail("Each pixel color of line should be" +
>> line_color + " but was:" + expected);
>
> The name `expected` is misleading. It isn't the expected value, but rather
> the "actual" value that you just read.
>
> Also, there are two formatting issues:
> 1. There should be a space after the `if`
> 2. The target statements of the `if` and `else` must be surrounded by curly
> braces (unless on the same line as the `if` which would look odd in this case)
done
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Sat, 19 Feb 2022 14:32:46 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update test case for line straightness check
>
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 78:
>
>> 76: launchLatch.countDown();
>> 77: }
>> 78: }
>
> Add a blank after this.
done
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 87:
>
>> 85: }
>> 86:
>> 87:
>
> Minor: the extra blank line is unnecessary.
done
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 105:
>
>> 103: Platform.runLater(() -> {
>> 104: webView = new WebView();
>> 105: Scene scene = new Scene(webView,80,60);
>
> This test passes on my Windows system even without the fix. Based on what I
> see on the screen, I think its because the scene size is too small. I would
> make it larger (at least 150x100).
>
> Minor: add a space after the commas to separate the args
space added, i will look this observation on windows
-
PR: https://git.openjdk.java.net/jfx/pull/731
RFR: 8282134: Certain regex can cause a JS trap in WebView
Fix YarrJIT backtrackCharacterClassNonGreedy breakpoint() Certain regex can cause a JS trap in WebView - Commit messages: - 8282134: Certain regex can cause a JS trap in WebView Changes: https://git.openjdk.java.net/jfx/pull/739/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=739=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282134 Stats: 22 lines in 3 files changed: 21 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/739.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/739/head:pull/739 PR: https://git.openjdk.java.net/jfx/pull/739
Re: RFR: 8280020: Underline and line-through not straight in WebView [v2]
On Sun, 13 Feb 2022 07:35:32 GMT, Jay Bhaskar wrote: >> Issue: The end point of line in drawLinesForText , add thickness to the >> endPoint.y(). In this case origin which is start point and the end point >> would not be same, and line would be drawn not straight. >> Solution: Do not add thickness to the y position of end point of line. >> Start Point(x,y) --End Point(x + width, 0) > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > adjust thickness A new system test case for checking line straightness added - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
> Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Update test case for line straightness check - Changes: - all: https://git.openjdk.java.net/jfx/pull/731/files - new: https://git.openjdk.java.net/jfx/pull/731/files/c5b6ff76..135a073b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=731=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=731=01-02 Stats: 238 lines in 2 files changed: 176 ins; 62 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/731.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/731/head:pull/731 PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v2]
On Sun, 13 Feb 2022 07:35:32 GMT, Jay Bhaskar wrote: >> Issue: The end point of line in drawLinesForText , add thickness to the >> endPoint.y(). In this case origin which is start point and the end point >> would not be same, and line would be drawn not straight. >> Solution: Do not add thickness to the y position of end point of line. >> Start Point(x,y) --End Point(x + width, 0) > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > adjust thickness The dash case (--) also works without looping https://user-images.githubusercontent.com/6468850/153743964-647e00cd-1dab-4ffb-9686-8a04c5844b98.png;> - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v2]
> Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: adjust thickness - Changes: - all: https://git.openjdk.java.net/jfx/pull/731/files - new: https://git.openjdk.java.net/jfx/pull/731/files/94a493ae..c5b6ff76 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=731=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=731=00-01 Stats: 15 lines in 1 file changed: 2 ins; 0 del; 13 mod Patch: https://git.openjdk.java.net/jfx/pull/731.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/731/head:pull/731 PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView
On Thu, 10 Feb 2022 11:36:38 GMT, Jay Bhaskar wrote: > Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) StrokeStyle savedStrokeStyle = strokeStyle(); setStrokeStyle(stroke); float savedStrokeThickness = strokeThickness(); setStrokeThickness(thickness); FloatPoint startPoint = origin + FloatPoint(0, thickness / 2); FloatPoint endPoint = startPoint + FloatPoint(width, 0); drawLine( IntPoint(startPoint.x(), startPoint.y()), IntPoint(endPoint.x(), endPoint.y())); setStrokeStyle(savedStrokeStyle); setStrokeThickness(savedStrokeThickness); It works , and stroke thickness is effective. - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView
On Thu, 10 Feb 2022 11:36:38 GMT, Jay Bhaskar wrote:
> Issue: The end point of line in drawLinesForText , add thickness to the
> endPoint.y(). In this case origin which is start point and the end point
> would not be same, and line would be drawn not straight.
> Solution: Do not add thickness to the y position of end point of line.
> Start Point(x,y) --End Point(x + width, 0)
void GraphicsContextCG::drawLinesForText(const FloatPoint& point, float
thickness, const DashArray& widths, bool printing, bool doubleLines,
StrokeStyle strokeStyle)
{
if (!widths.size())
return;
Color localStrokeColor(strokeColor());
FloatRect bounds =
computeLineBoundsAndAntialiasingModeForText(FloatRect(point,
FloatSize(widths.last(), thickness)), printing, localStrokeColor);
if (bounds.isEmpty())
--
computeLineBoundsAndAntialiasingModeForText this will return rect after
considering thickness.
The TextDecorationPainter::paintTextDecoration..
FloatRect underlineBoundingBox =
m_context.computeUnderlineBoundsForText(rect, m_isPrinting);
DashArray intersections =
m_font.dashesForIntersectionsWithRect(textRun, textOrigin,
underlineBoundingBox);
DashArray boundaries =
translateIntersectionPointsToSkipInkBoundaries(intersections,
underlineBoundingBox.height(), rect.width());
ASSERT(!(boundaries.size() % 2));
// We don't use underlineBoundingBox here because
drawLinesForText() will run computeUnderlineBoundsForText() internally.
m_context.drawLinesForText(rect.location(), rect.height(),
boundaries, m_isPrinting, style == TextDecorationStyle::Double, strokeStyle);
This is already calculating underlineBoundingBox and generates boundaries.
m_context.drawLinesForText(rect.location(), rect.height(), boundaries,
m_isPrinting, style == TextDecorationStyle::Double, strokeStyle);
rect.height() is passed as thickness.
So, void GraphicsContextJava::drawLinesForText(const FloatPoint& origin, float
thickness, const DashArray& widths, bool, bool, StrokeStyle stroke) {
if (paintingDisabled())
return;
for (const auto& width : widths) {
// This is a workaround for http://bugs.webkit.org/show_bug.cgi?id=15659
StrokeStyle savedStrokeStyle = strokeStyle();
setStrokeStyle(stroke);
// do not add thickness to y position of end point , as line should be
straight to the origin
FloatPoint endPoint = origin + FloatPoint(width, 0);
drawLine(
IntPoint(origin.x(), origin.y()),
IntPoint(endPoint.x(), endPoint.y()));
setStrokeStyle(savedStrokeStyle);
}
}
To calculate end point , we should not add thickess to the end point. Let it
draw with default thickness.
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView
On Thu, 10 Feb 2022 11:36:38 GMT, Jay Bhaskar wrote: > Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) [PageFillTest.java](https://github.com/openjdk/jfx/tree/master/tests/system/src/test/java/test/javafx/scene/web/PageFillTest.java) this example only for taking snapshot and read color property. It will not help to test line drawing. - PR: https://git.openjdk.java.net/jfx/pull/731
RFR: 8280020: Underline and line-through not straight in WebView
Issue: The end point of line in drawLinesForText , add thickness to the endPoint.y(). In this case origin which is start point and the end point would not be same, and line would be drawn not straight. Solution: Do not add thickness to the y position of end point of line. Start Point(x,y) --End Point(x + width, 0) - Commit messages: - 8280020: Underline and line-through not straight in WebView Changes: https://git.openjdk.java.net/jfx/pull/731/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=731=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8280020 Stats: 64 lines in 2 files changed: 63 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/731.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/731/head:pull/731 PR: https://git.openjdk.java.net/jfx/pull/731
RFR: 8269115: WebView paste event contains old data
Issue: The DataObject uses m_availMimeTypes as Vector of strings, and appending mime types in pasteboard operation like setPlainText, Hence it will append duplicate mime types in m_availMimeTypes , in this case the length clipboardData.types would be wrong, and duplicates data would be copied to clipboard target. Solution: Use ListHashSet data Structure instead of Vector for m_availMimeTypes. - Commit messages: - 8269115:WebView paste event contains old data Changes: https://git.openjdk.java.net/jfx/pull/729/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=729=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269115 Stats: 26 lines in 3 files changed: 12 ins; 3 del; 11 mod Patch: https://git.openjdk.java.net/jfx/pull/729.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/729/head:pull/729 PR: https://git.openjdk.java.net/jfx/pull/729
Re: RFR: 8255940: localStorage is null after window.close() [v3]
On Sun, 6 Feb 2022 11:54:14 GMT, Jay Bhaskar wrote:
>> No, it's not OK for two reasons:
>>
>> 1. It prevents concurrent execution of tests from two different directories
>> 2. The Java temp directory might be something other than "/tmp" on some
>> systems.
>>
>> Best is to use a local subdir under the build directory.
>
> i am unable to find other test case which uses local sub-directory in the
> build dir so let me know i can use this
> String defaultBaseDir = System.getProperty("java.io.tmpdir");
> webEngine.setUserDataDirectory(new File(defaultBaseDir +
> "localstorage")
I have updated your suggestions and used local dir for setUserDataDirectory
-
PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v3]
> Issue: The current implementation of DOMWindow ::localStorage(..) returns > null pointer in case of page is being closed. > Fix: It should not return nullptr , as per the [w3c web storage > spec](https://www.w3.org/TR/2016/REC-webstorage-20160419/) > "User agents should expire data from the local storage areas only for > security reasons or when requested to do so by the user. User agents should > always avoid deleting data while a script that could access that data is > running." Jay Bhaskar has updated the pull request incrementally with three additional commits since the last revision: - Change Dir Path and use local Dir and set data before clearing localstorage test - Merge branch 'PRLocalstorage' of https://github.com/jaybhaskar/jfx into PRLocalstorage - Merge branch 'master' into PRLocalstorage - Changes: - all: https://git.openjdk.java.net/jfx/pull/703/files - new: https://git.openjdk.java.net/jfx/pull/703/files/73299577..a8647839 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=703=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=703=01-02 Stats: 78 lines in 3 files changed: 68 ins; 1 del; 9 mod Patch: https://git.openjdk.java.net/jfx/pull/703.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/703/head:pull/703 PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v2]
On Sat, 5 Feb 2022 15:01:37 GMT, Kevin Rushforth wrote:
>> i think /tmp/java-store is ok, as it would not require cleanup
>
> No, it's not OK for two reasons:
>
> 1. It prevents concurrent execution of tests from two different directories
> 2. The Java temp directory might be something other than "/tmp" on some
> systems.
>
> Best is to use a local subdir under the build directory.
i am unable to find other test case which uses local sub-directory in the
build dir so let me know i can use this
String defaultBaseDir = System.getProperty("java.io.tmpdir");
webEngine.setUserDataDirectory(new File(defaultBaseDir + "localstorage")
-
PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v2]
On Sat, 5 Feb 2022 15:17:46 GMT, Jay Bhaskar wrote: >> Can you comment on this? > > yes, your are right , i would change it i will test unit case again after your suggested chnage - PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v2]
On Sat, 5 Feb 2022 15:02:09 GMT, Kevin Rushforth wrote:
>> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
>> line 60:
>>
>>> 58: assertNotNull(webEngine.executeScript("localStorage;"));
>>> 59: getEngine().executeScript("window.close();");
>>> 60: assertNotNull(webEngine.executeScript("localStorage;"));
>>
>> It seems useful to verify the contents by writing something before the
>> window is closed, and then verifying that the same value can be read.
>
> Can you comment on this?
yes, your are right , i would change it
-
PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v2]
On Sat, 5 Feb 2022 15:04:37 GMT, Kevin Rushforth wrote: >> The test runs after testLocalStorageSet , so there would be items in >> localstorage > > No, this is a common misconception when writing JUnit tests. The test > execution order is _not_ guaranteed and will change. Each test method needs > to run as if it were the first (or only) test. i would change as suggested , test and then pushed the changes - PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v2]
On Sat, 5 Feb 2022 14:58:25 GMT, Kevin Rushforth wrote: >> in recent code base (webkit upgrade)the localstorage is false , >> @@ -1579,7 +1579,7 @@ NeedsSiteSpecificQuirks: >> WebKit: >>default: true >> WebCore: >> - default: false >> + default: true >> >> So , it needs to be enable also. > > 1. My point was that if you initially check for `m_localStorage` being > non-null, without the check for `isClosing` (see my earlier comment), then > you don't need to check it here. > > 2. Regarding the enabling of local storage in WebCore, are you seeing any > problems as a result of not having it enabled? yes, in case of not enable , in any case localstorage would be null - PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v2]
On Sat, 5 Feb 2022 12:51:58 GMT, Jay Bhaskar wrote: >> modules/javafx.web/src/main/native/Source/WebCore/page/DOMWindow.cpp line >> 859: >> >>> 857: if (page->isClosing() && m_localStorage) >>> 858: return m_localStorage.get(); >>> 859: >> >> If you make the earlier modification I suggested, then you don't need this >> block. > > We must check if localstorage setting is disabled, then return nullptr > first.,as below > if (!page->settings().localStorageEnabled()) > return nullptr; > > and there after the section > if (page->isClosing() && m_localStorage) > return m_localStorage.get(); > would become as below > if (m_localStorage) > return m_localStorage.get(); in recent code base (webkit upgrade)the localstorage is false , @@ -1579,7 +1579,7 @@ NeedsSiteSpecificQuirks: WebKit: default: true WebCore: - default: false + default: true So , it needs to be enable also. - PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v2]
On Fri, 28 Jan 2022 00:10:31 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains two additional >> commits since the last revision: >> >> - Merge branch 'openjdk:master' into PRLocalstorage >> - Window.close(), Fix for localstoarge > > modules/javafx.web/src/main/native/Source/WebCore/page/DOMWindow.cpp line 859: > >> 857: if (page->isClosing() && m_localStorage) >> 858: return m_localStorage.get(); >> 859: > > If you make the earlier modification I suggested, then you don't need this > block. We must check if localstorage setting is disabled, then return nullptr first.,as below if (!page->settings().localStorageEnabled()) return nullptr; and there after the section if (page->isClosing() && m_localStorage) return m_localStorage.get(); would become as below if (m_localStorage) return m_localStorage.get(); - PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v2]
On Fri, 28 Jan 2022 00:11:40 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains two additional
>> commits since the last revision:
>>
>> - Merge branch 'openjdk:master' into PRLocalstorage
>> - Window.close(), Fix for localstoarge
>
> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
> line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights
>> reserved.
>
> Copyright should be a single year (2022)
done
> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
> line 48:
>
>> 46: final WebEngine webEngine = getEngine();
>> 47: webEngine.setJavaScriptEnabled(true);
>> 48: webEngine.setUserDataDirectory(new File("/tmp/java-store"));
>
> You should not hard-code /tmp/. You can either use a local subdirectory in
> the build dir (which some other tests do), or else you will need to use
> something like `Files.createTempDirectory("webstorage")`. If you use the
> latter, then you will need to worry about how to clean up after the test, so
> the former seems better.
i think /tmp/java-store is ok, as it would not require cleanup
> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
> line 70:
>
>> 68: WebView view = getView();
>> 69: view.getEngine().executeScript("test_local_storage_set();");
>> 70: String s = (String)
>> view.getEngine().executeScript("document.getElementById('key').innerText;");
>
> This will work, but it might be cleaner to add a JavaScript `getLocalStorage`
> method so you don't have to get it from a DOM element.
The method webEngine.executeScript("localStorage;") returns JSObject and in to
get data members we need to call js.getMember(...) , getMember is not available
in our code base, so i used tring s = (String)
view.getEngine().executeScript("document.getElementById('key').innerText;");
> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
> line 80:
>
>> 78: submit(() -> {
>> 79: WebView view = getView();
>> 80: view.getEngine().executeScript("delete_items();");
>
> You need to set some items first before deleting them if you want it to be an
> effective test of this case.
The test runs after testLocalStorageSet , so there would be items in
localstorage
-
PR: https://git.openjdk.java.net/jfx/pull/703
Re: RFR: 8255940: localStorage is null after window.close() [v2]
> Issue: The current implementation of DOMWindow ::localStorage(..) returns > null pointer in case of page is being closed. > Fix: It should not return nullptr , as per the [w3c web storage > spec](https://www.w3.org/TR/2016/REC-webstorage-20160419/) > "User agents should expire data from the local storage areas only for > security reasons or when requested to do so by the user. User agents should > always avoid deleting data while a script that could access that data is > running." Jay Bhaskar has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'openjdk:master' into PRLocalstorage - Window.close(), Fix for localstoarge - Changes: - all: https://git.openjdk.java.net/jfx/pull/703/files - new: https://git.openjdk.java.net/jfx/pull/703/files/12f97feb..73299577 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=703=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=703=00-01 Stats: 416909 lines in 6905 files changed: 234954 ins; 135768 del; 46187 mod Patch: https://git.openjdk.java.net/jfx/pull/703.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/703/head:pull/703 PR: https://git.openjdk.java.net/jfx/pull/703
RFR: 8255940: localStorage is null after window.close()
Issue: The current implementation of DOMWindow ::localStorage(..) returns null pointer in case of page is being closed. Fix: It should not return nullptr , as per the [w3c web storage spec](https://www.w3.org/TR/2016/REC-webstorage-20160419/) "User agents should expire data from the local storage areas only for security reasons or when requested to do so by the user. User agents should always avoid deleting data while a script that could access that data is running." - Commit messages: - Window.close(), Fix for localstoarge Changes: https://git.openjdk.java.net/jfx/pull/703/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=703=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255940 Stats: 128 lines in 3 files changed: 118 ins; 9 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/703.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/703/head:pull/703 PR: https://git.openjdk.java.net/jfx/pull/703
