Bug 225344 - localStorage changes aren't reflected between WKWebViews using WKWebViewConfiguration._groupIdentifier
Summary: localStorage changes aren't reflected between WKWebViews using WKWebViewConfi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari 14
Hardware: Unspecified Unspecified
: P2 Blocker
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-04 01:05 PDT by Jake Archibald
Modified: 2021-05-05 12:19 PDT (History)
17 users (show)

See Also:


Attachments
Patch (5.32 KB, patch)
2021-05-04 13:04 PDT, Alex Christensen
beidson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jake Archibald 2021-05-04 01:05:29 PDT
Using Safari 14.1 or later:

1. Tab A: Open https://static-misc-3.glitch.me/localstorage-bug/ in new tab
2. Tab A: Hit 'increment'
3. Tab B: Open https://static-misc-3.glitch.me/localstorage-bug/ in new tab - it displays '1' as expected
4. Tab B: Hit 'increment'

Tab A does not get a 'storage' event, clicking "Re-get value" returns the same value.

It seems like localStorage state is copied at some point, but it doesn't receive any updates after that. However, it's still writing to the underlying data store.

Refreshing the page still returns stale data.

This will result in data loss on websites, as two tabs will be writing to storage, unaware of each others' changes.

Additionally, the 'storage' event is used for cross-tab communication in Safari, since it doesn't support BroadcastChannel, and this bug breaks that. In fact, BroadcastChannel polyfills use the 'storage' event https://github.com/JSmith01/broadcastchannel-polyfill.
Comment 1 Jake Archibald 2021-05-04 02:36:52 PDT
In case anyone needs a workaround, you can switch to indexeddb for storage, and use service workers to communicate the changes between tabs https://static-misc-3.glitch.me/safari-storage-workaround/
Comment 2 Thomas Steiner 2021-05-04 02:52:37 PDT
(Somewhat related: people have been asking for `BroadcastChannel` support in https://bugs.webkit.org/show_bug.cgi?id=161472.)
Comment 3 Radar WebKit Bug Importer 2021-05-04 03:11:22 PDT
<rdar://problem/77496721>
Comment 4 Jake Archibald 2021-05-04 03:37:44 PDT
Here's a failing wpt http://wpt.live/webstorage/storage_local_window_open.window.html
Comment 5 Luke 2021-05-04 05:45:50 PDT
It's worth noting this appears to only be the case on macOS. iOS seems to behave as expected. Tested with iPad OS 14.5.1 and macOS Big Sur 11.3.1.
Comment 6 Chris Dumez 2021-05-04 08:25:46 PDT
Investigating. Thanks for the bug report.
Comment 7 Chris Dumez 2021-05-04 08:52:59 PDT
Looks like this was caused by a Safari change, not a WebKit change. Keeping this WebKit bug report open for now in case we need a WebKit-side fix.
Comment 8 Alex Christensen 2021-05-04 13:04:25 PDT
Created attachment 427693 [details]
Patch
Comment 9 Simon Fraser (smfr) 2021-05-04 14:03:36 PDT
Comment on attachment 427693 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427693&action=review

> Source/WebKit/UIProcess/WebPageGroup.cpp:86
> +        data.pageGroupID = generatePageGroupID();

Why don't we use ObjectIdentifier<> for the page group?
Comment 10 Alex Christensen 2021-05-04 14:06:58 PDT
We should.  This is old code that hasn't been updated to our newer idioms, but I certainly don't want to do that in this patch for the reason stated in the ChangeLog.
Comment 11 Alex Christensen 2021-05-04 14:32:16 PDT
http://trac.webkit.org/r276983
Comment 12 Darin Adler 2021-05-04 18:36:49 PDT
Comment on attachment 427693 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427693&action=review

> Source/WebKit/UIProcess/WebPageGroup.cpp:81
> +    if (HashMap<String, uint64_t>::isValidKey(identifier)) {

Seems like overkill to use isValidKey here. If null is a special value, meaning we don’t have one, we should check for null. The fact that null also can’t be used as a hash table key is a separate thing that doesn’t seem the same to me.
Comment 13 Alex Christensen 2021-05-05 10:05:24 PDT
It is probably overkill, but I wanted this to be as low risk as possible to merge onto a branch.  I intend to remove all this code soon.
Comment 14 Darin Adler 2021-05-05 10:17:26 PDT
(In reply to Alex Christensen from comment #13)
> It is probably overkill, but I wanted this to be as low risk as possible to
> merge onto a branch.  I intend to remove all this code soon.

I understand what you are saying.

I don’t think that using isValidKey makes this lower risk than an isNull() check.
Comment 15 Alex Christensen 2021-05-05 12:19:45 PDT
I think you're right.  It seems impossible to pass a hash-table-deleted string through our API.