Bug 226814 – localStorage "locks" items when they're updated too frequently

• Component: WebCore Misc.
• Status: RESOLVED DUPLICATE

[Pascal Cremer]

Created attachment 430961 [details]
Demo

Hi everyone,

I've originally come across this issue when checking my apps in iOS 15b1, but I could replicate it in the latest Safari Technology Preview (125) for macOS 11 as well.

It seems like items in the Local Storage are "locked", when they're updated too frequently (i.e. when calling `window.localStorage.setItem()`). What happens is that, at some point, `window.localStorage.setItem()` just stops working without any errors. Here's the test script I've used to replicate the issue from the Web Inspector:

```
const setItemMulti = (times, delay) => {
    if (times == 0) {
        return;
    }
    
    window.localStorage.setItem('foo', times);
    setTimeout(() => setItemMulti(times-1, delay), delay);
}

setItemMulti(10, 50)
```

It works for `delay` >= 100ms, but breaks for values below, such as 50ms. After running the script, calling `window.localStorage.setItem('foo', 123)` (or similar) won't change the value of `foo` any longer. Please see the attached GIF for a demo. I can also confirm that the same script behaves as expected in Safari 14.1 (16611.1.21.161.6) on macOS 11.

This issue currently breaks one app I'm maintaining, where we use the localStorage to store JWT tokens and other user information.

Any help or feedback is much appreciated.

[Radar WebKit Bug Importer]

<rdar://problem/79228594>

[Chris Dumez]

Just tried the provided code on Safari 15 a couple of times and each time I ended up with foo=1, which I think is the expected result.

[Chris Dumez]

(In reply to Chris Dumez from comment #2)
> Just tried the provided code on Safari 15 a couple of times and each time I
> ended up with foo=1, which I think is the expected result.

I even tried a more aggressive `setItemMulti(100, 10)` and it seems to work as expected too.

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> (In reply to Chris Dumez from comment #2)
> > Just tried the provided code on Safari 15 a couple of times and each time I
> > ended up with foo=1, which I think is the expected result.
> 
> I even tried a more aggressive `setItemMulti(100, 10)` and it seems to work
> as expected too.

I was on bugs.webkit.org earlier. I have just tried running an Apache server on localhost (since the GIF attached seems to indicate the host was "localhost"). However, still no luck reproducing.

[Chris Dumez]

I have just tried WebKit r277448 (which matches the WebKit in STP 125) since I cannot install STP 125 on MacOS Monterey. Sadly, it still didn't reproduce with the code sample over. I am doing the exact same thing as in the animated GIF AFAICT.

[Chris Dumez]

(In reply to Chris Dumez from comment #5)
> I have just tried WebKit r277448 (which matches the WebKit in STP 125) since
> I cannot install STP 125 on MacOS Monterey. Sadly, it still didn't reproduce
> with the code sample over. I am doing the exact same thing as in the
> animated GIF AFAICT.

Since I have a fast machine (iMac Pro), I tried to reduce the interval like so:
setItemMulti(1000, 1)

I still see the value of foo going down quickly from 1000 to 1. Not sure what's going on..

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> (In reply to Chris Dumez from comment #5)
> > I have just tried WebKit r277448 (which matches the WebKit in STP 125) since
> > I cannot install STP 125 on MacOS Monterey. Sadly, it still didn't reproduce
> > with the code sample over. I am doing the exact same thing as in the
> > animated GIF AFAICT.
> 
> Since I have a fast machine (iMac Pro), I tried to reduce the interval like
> so:
> setItemMulti(1000, 1)
> 
> I still see the value of foo going down quickly from 1000 to 1. Not sure
> what's going on..

To be safe, I also tried on an iPhone 12 Pro running iOS 15 beta but still no issue.

[Pascal Cremer]

Hey Chris,

That's weird indeed, I also cannot longer reproduce it with my snippet.

I could show you the web app I was talking about, where I can reproduce this condition every time. I however cannot put any information about it on a public bug tracker. Can I instead email you directly or anything else?

[Chris Dumez]

(In reply to Pascal Cremer from comment #8)
> Hey Chris,
> 
> That's weird indeed, I also cannot longer reproduce it with my snippet.
> 
> I could show you the web app I was talking about, where I can reproduce this
> condition every time. I however cannot put any information about it on a
> public bug tracker. Can I instead email you directly or anything else?

The ideal would really be a reduced test case that reproduces the issue. However, if that's not possible you can email me directly and I'll see what I can do.

[Hui Tang]

I can reproduce the problem. Can I upload a screenshot here? . I am a newcomer here. Thank you.

[Hui Tang]

(In reply to Chris Dumez from comment #9)
> (In reply to Pascal Cremer from comment #8)
> > Hey Chris,
> > 
> > That's weird indeed, I also cannot longer reproduce it with my snippet.
> > 
> > I could show you the web app I was talking about, where I can reproduce this
> > condition every time. I however cannot put any information about it on a
> > public bug tracker. Can I instead email you directly or anything else?
> 
> The ideal would really be a reduced test case that reproduces the issue.
> However, if that's not possible you can email me directly and I'll see what
> I can do.

I also encountered this problem on the iOS 15 beta. If allowed, I can also send an email to provide more information.

[Hui Tang]

It broke my jwt tokens authentication also.

[Pascal Cremer]

(In reply to Hui Tang from comment #10)
> I can reproduce the problem. Can I upload a screenshot here? . I am a
> newcomer here. Thank you.

Could you isolate the problem? I can reproduce it with one of my web apps, but it seems like I cannot isolate it enough to produce a failing PoC.

[Hui Tang]

(In reply to Pascal Cremer from comment #13)
> (In reply to Hui Tang from comment #10)
> > I can reproduce the problem. Can I upload a screenshot here? . I am a
> > newcomer here. Thank you.
> 
> Could you isolate the problem? I can reproduce it with one of my web apps,
> but it seems like I cannot isolate it enough to produce a failing PoC.

I can't, I guess my experience is almost exactly the same as yours.

But, I may have found a way to fix it.

When you put your new token to storage, try remove it first. just like this:

```
+localStorage.removeItem('token'); // add this line
 localStorage.setItem('token', 'xxx');
```

Have a good try, it make my code works again.

[Chris Dumez]

(In reply to Pascal Cremer from comment #13)
> (In reply to Hui Tang from comment #10)
> > I can reproduce the problem. Can I upload a screenshot here? . I am a
> > newcomer here. Thank you.
> 
> Could you isolate the problem? I can reproduce it with one of my web apps,
> but it seems like I cannot isolate it enough to produce a failing PoC.

As long as it is reproducible, it would really help me to get access to a reproduction case (even if not small). My email is open if this is not something that can be shared publicly.

[Chris Dumez]

(In reply to Chris Dumez from comment #15)
> (In reply to Pascal Cremer from comment #13)
> > (In reply to Hui Tang from comment #10)
> > > I can reproduce the problem. Can I upload a screenshot here? . I am a
> > > newcomer here. Thank you.
> > 
> > Could you isolate the problem? I can reproduce it with one of my web apps,
> > but it seems like I cannot isolate it enough to produce a failing PoC.
> 
> As long as it is reproducible, it would really help me to get access to a
> reproduction case (even if not small). My email is open if this is not
> something that can be shared publicly.

Given the symptoms, it is entirely possible this is not a local storage bug but a JavaScript bug (maybe JIT issue). Those are usually tough to reduce because they require running the code a number of times before triggering.

[Alexey Shvayka]

(In reply to Pascal Cremer from comment #0)
> It works for `delay` >= 100ms, but breaks for values below, such as 50ms.

I'm not sure this is likely to be a symptom of JIT-related issue. JIT bugs that are not GC-related normally reproduce after a function called X times.

With that said, to completely eliminate IC-related issues, one can replace all occurrences, if any, of:
* `localStorage.x` with `localStorage.getItem("x")`;
* `localStorage.x = val` with `localStorage.setItem("x", val)`;
* `delete localStorage.x` with `localStorage.removeItem("x")`.

Type info flags that are set on JSStorage seems to be correct.

> I've originally come across this issue when checking my apps in iOS 15b1, but I could replicate it in the latest Safari Technology Preview (125) for macOS 11 as well.

MBA 8.1, macOS 11.4, TP 125, followed strictly the GIF (with Web Inspector open), with different `times` / `delay` arguments. No luck to reproduce.

Maybe some site-related Storage quirks are to blame?

[Yusuke Suzuki]

LocalStorage implementation is fully in C++ in WebCore side. Even if anything happens in optimized JS code side, it should not make LocalStorage locked (since this state management should be done in C++). So I don't think this is JS related.

[Chris Dumez]

Good news. Pascal was able to provide me with a test case and I determined that this was fixed recently on trunk by https://trac.webkit.org/changeset/278651.

Given the patch, I suspect we had a quota accounting issue and we were refusing the do writes thinking that the quota was reached. I'll look into this more tomorrow to see if I can spot the logic issue.

In any case, it appears to be fixed.

[Pascal Cremer]

(In reply to Chris Dumez from comment #19)
> Good news. Pascal was able to provide me with a test case and I determined
> that this was fixed recently on trunk by
> https://trac.webkit.org/changeset/278651.
> 
> Given the patch, I suspect we had a quota accounting issue and we were
> refusing the do writes thinking that the quota was reached. I'll look into
> this more tomorrow to see if I can spot the logic issue.
> 
> In any case, it appears to be fixed.

Thanks Chris, really appreciate you've dedicated some time to investigate.

Would be great if you could share your any findings, since I'm still trying to wrap my head around on how I could reach the exception state with the snippet from my start post.

[Chris Dumez]

(In reply to Pascal Cremer from comment #20)
> (In reply to Chris Dumez from comment #19)
> > Good news. Pascal was able to provide me with a test case and I determined
> > that this was fixed recently on trunk by
> > https://trac.webkit.org/changeset/278651.
> > 
> > Given the patch, I suspect we had a quota accounting issue and we were
> > refusing the do writes thinking that the quota was reached. I'll look into
> > this more tomorrow to see if I can spot the logic issue.
> > 
> > In any case, it appears to be fixed.
> 
> Thanks Chris, really appreciate you've dedicated some time to investigate.
> 
> Would be great if you could share your any findings, since I'm still trying
> to wrap my head around on how I could reach the exception state with the
> snippet from my start post.

It was an accounting issue on our side. In particular, this code in setItem():
```
        if (!m_databaseSize) {
            m_databaseSize = SQLiteFileSystem::databaseFileSize(m_databasePath);
        }
        CheckedUint64 newDatabaseSize = *m_databaseSize;
        newDatabaseSize -= oldValue.sizeInBytes();
        newDatabaseSize += value.sizeInBytes();
```

I see that oldValue.sizeInBytes() can be larger than newDatabaseSize. As a result, `newDatabaseSize -= oldValue.sizeInBytes()` would underflow and we'd end up with a very large newDatabaseSize (over quota). Mixing string sizes in bytes and database size on disk was not a good idea.

[Chris Dumez]

*** This bug has been marked as a duplicate of bug 226788 ***

[Pascal Cremer]

Hey everyone,

should the fix be available in the new Safari Technology Preview 126? I've just downloaded it and I'm running into the same issue again.

[Hui Tang]

126 does not include that fix. Maybe 127,128...

[Chris Dumez]

(In reply to Pascal Cremer from comment #23)
> Hey everyone,
> 
> should the fix be available in the new Safari Technology Preview 126? I've
> just downloaded it and I'm running into the same issue again.

No, the fix is not in STP 126 yet, hopefully the next one.