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

[Kevin Rushforth]

On Mon, 7 Feb 2022 08:14:50 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."
>
> 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