RFR: 8255940: localStorage is null after window.close()

[Kevin Rushforth]

On Mon, 27 Dec 2021 09:31:08 GMT, Jay Bhaskar <duke at 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