StorageManager::removeAllowedSessionStorageNamespaceConnection should make sure its storageNamespaceID is valid
<rdar://problem/51352080>
Created attachment 372341 [details] Patch
Comment on attachment 372341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372341&action=review > Source/WebKit/ChangeLog:10 > + The namespace ID is coming straight from IPC so should not be trusted. I agree with this, but I don't think this crash is caused by untrusted message sent to web process. Likely there is a bug in the code (e.g. web page was removed before it's getting added because of some race), and using the If condition here will conceal the real bug. But as we also don't like the idea of notifying network process when page is added/removed in web process for sessionStorage, this may be a good enough fix before we drop those code. > Source/WebKit/ChangeLog:13 > + Also, namespace IDs are added/removed based on web pages being created/deleted. > + Namespace IDs are supposed to be scoped by session IDs. > + Using page IDs for namespace IDs works as long as the page does not change of session ID during its lifetime, which is not guaranteed. If my understanding is correct, if page is changing its session, it should add itself to new storage session(StorageManager) first, which will create a new sessionStorageNameSpace that have the corresponding namespace/page ID.
> I agree with this, but I don't think this crash is caused by untrusted > message sent to web process. Likely there is a bug in the code (e.g. web > page was removed before it's getting added because of some race), and using > the If condition here will conceal the real bug. > > But as we also don't like the idea of notifying network process when page is > added/removed in web process for sessionStorage, this may be a good enough > fix before we drop those code. Yes, the patch is only about fixing the crash. It should be completed by either retiring the use of page IDs, or making it fully working. I think the end goal should be to retire the use of page IDs here. > > Source/WebKit/ChangeLog:13 > > + Also, namespace IDs are added/removed based on web pages being created/deleted. > > + Namespace IDs are supposed to be scoped by session IDs. > > + Using page IDs for namespace IDs works as long as the page does not change of session ID during its lifetime, which is not guaranteed. > > If my understanding is correct, if page is changing its session, it should > add itself to new storage session(StorageManager) first, which will create a > new sessionStorageNameSpace that have the corresponding namespace/page ID. I am not sure what we want in the short term. Maybe it is ok to fix this one before trying to remove the page ID usage. I fear that we may see other bugs in that area until we remove page ID usage.
> I agree with this, but I don't think this crash is caused by untrusted > message sent to web process. Likely there is a bug in the code (e.g. web > page was removed before it's getting added because of some race), and using > the If condition here will conceal the real bug. There is a debug assertion to catch the real bug. The crash might come from session ID change.
Comment on attachment 372341 [details] Patch Clearing flags on attachment: 372341 Committed r246552: <https://trac.webkit.org/changeset/246552>
All reviewed patches have been landed. Closing bug.
*** Bug 199208 has been marked as a duplicate of this bug. ***