Bug 225344

Summary: localStorage changes aren't reflected between WKWebViews using WKWebViewConfiguration._groupIdentifier
Product: WebKit Reporter: Jake Archibald <jaffathecake>
Component: DOMAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Blocker CC: achristensen, beidson, bugmail, cabanier, cdumez, darin, emilio, eoconnor, graouts, gsnedders, koddsson, lwarlow, m, m.kurz+webkitbugs, rik, shantmarouti, sihui_liu, simon.fraser, tomac, twilco.o, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 14   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch beidson: review+

Jake Archibald
Reported 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.
Attachments
Patch (5.32 KB, patch)
2021-05-04 13:04 PDT, Alex Christensen 		beidson: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Jake Archibald
Comment 1 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/
Thomas Steiner
Comment 2 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.)
Radar WebKit Bug Importer
Comment 3 2021-05-04 03:11:22 PDT
<rdar://problem/77496721>
Jake Archibald
Comment 4 2021-05-04 03:37:44 PDT
Here's a failing wpt http://wpt.live/webstorage/storage_local_window_open.window.html
Luke Warlow
Comment 5 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.
Chris Dumez
Comment 6 2021-05-04 08:25:46 PDT
Investigating. Thanks for the bug report.
Chris Dumez
Comment 7 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.
Alex Christensen
Comment 8 2021-05-04 13:04:25 PDT
Created attachment 427693 [details] Patch
Simon Fraser (smfr)
Comment 9 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?
Alex Christensen
Comment 10 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.
Alex Christensen
Comment 11 2021-05-04 14:32:16 PDT
http://trac.webkit.org/r276983
Darin Adler
Comment 12 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.
Alex Christensen
Comment 13 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.
Darin Adler
Comment 14 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.
Alex Christensen
Comment 15 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.
Thomas Steiner
Comment 16 2021-06-08 13:24:16 PDT
This fix seems to have rolled out with iOS/iPadOS 15 beta 1.
Guido Bouman
Comment 17 2021-07-08 09:56:22 PDT
Will this also land in 14.2, if/when that comes? Seems like a big regression that's worth patching before 15. Even worth a 14.1.1 if you ask me.
Alex Christensen
Comment 18 2021-07-08 09:57:35 PDT
We are aware of how important this bug is. We have no comment on future releases.
Sam Sneddon [:gsnedders]
Comment 19 2021-07-28 14:01:18 PDT
(In reply to Thomas Steiner from comment #16) > This fix seems to have rolled out with iOS/iPadOS 15 beta 1. As mentioned above, this never affected iOS/iPadOS, as it was macOS only. To note, this was fixed in macOS 11.5.
Note You need to log in before you can comment on or make changes to this bug.