On Mon, 7 Feb 2022 08:14:50 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." > > 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 You haven't addressed one of my comments on the fix. I'll try to clarify what I meant so we're on the same page. Currently (meaning without your fix), the following check is done right after getting the page: // 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(); } There are three cases we might consider: 1. The page is null 2. The page is non-null and is _not_ closing 3. The page is non-null and _is_ closing In the first two cases we currently return the local-storage if it exists. Unless I'm missinsg something, this should also be done for the case of non-null and closing (the third case), which simplifies to unconditionally returning the local-storage if it exists, or: if (m_localStorage) return m_localStorage.get(); If you don't do this, then case 1, the case of a `null` page, will return `nullptr`, which is a change in behavior. The other important part of the change, which you already did, was to remove the `if (page->isClosing() return nullptr;` block. I think those are the only two changes to the existing code that are needed, but it's possible I'm missing something. As for the test, it looks better now. I noted just a few things that could be cleaned up below. 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. 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. 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. 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). 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). 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. ------------- PR: https://git.openjdk.java.net/jfx/pull/703
