On Mon, 27 Dec 2021 09:31:08 GMT, Jay Bhaskar <d...@openjdk.java.net> 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 change will cause the typical case where the page isn't closing to always allocate a new storage area, whereas the current code returns `m_localStorage.get()` if `m_localStorage` is valid. Also, will cause the case where `page` is `null` to return `nullptr`, whereas the current code returns `m_localStorage.get()` if `m_localStorage` is valid. Unless I'm missing something, the more correct fix is is to restore the check for `m_localStorage`, without the check for `page` (not removing the whole block). See inline comments. modules/javafx.web/src/main/native/Source/WebCore/page/DOMWindow.cpp line 854: > 852: return m_localStorage.get(); > 853: } > 854: 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(); 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. 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) 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. 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. 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. 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. ------------- Changes requested by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/703
