Bug 206820

Created attachment 388842 [details] Patch

Comment on attachment 388842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388842&action=review > Source/WebKit/ChangeLog:13 > + But some applications might have service worker enabled while not having set the SW registration storage path. Those applications would then just have no service worker persistence, right?

Comment on attachment 388842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388842&action=review >> Source/WebKit/ChangeLog:13 >> + But some applications might have service worker enabled while not having set the SW registration storage path. > > Those applications would then just have no service worker persistence, right? Right. I think it is the same behavior for other storage data. The ASSERT was specifically about sessionID setup being available.

Ping review.

Ping again.

Comment on attachment 388842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388842&action=review >>> Source/WebKit/ChangeLog:13 >>> + But some applications might have service worker enabled while not having set the SW registration storage path. >> >> Those applications would then just have no service worker persistence, right? > > Right. I think it is the same behavior for other storage data. > The ASSERT was specifically about sessionID setup being available. Seems wrong for me to have a non-ephemeral session with a SW registration path set. This sounds like a bug. I think the SW registration path should always be set for persistent session, no? Otherwise, we are not *persisting* service workers.

(In reply to Chris Dumez from comment #6) > Comment on attachment 388842 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388842&action=review > > >>> Source/WebKit/ChangeLog:13 > >>> + But some applications might have service worker enabled while not having set the SW registration storage path. > >> > >> Those applications would then just have no service worker persistence, right? > > > > Right. I think it is the same behavior for other storage data. > > The ASSERT was specifically about sessionID setup being available. > > Seems wrong for me to have a non-ephemeral session with a SW registration > path set. This sounds like a bug. I think the SW registration path should > always be set for persistent session, no? Otherwise, we are not *persisting* > service workers. We do not crash the network process for other data types (IDB, DOM cache, network cache) in case of empty path. I do not think we should do it either for Service Worker. There is nothing precluding an application to provide a wrong path and we do not crash either. I agree the API surface should limit (or better, forbid) this kind of setup. Ideally, you would need to provide a correct root path for all data for a given session and we would then organise all stored data in that root path appropriately, at least for persistent data like IDB, SW and DOM cache. This seems like another issue though.

Comment on attachment 388842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388842&action=review >>>>> Source/WebKit/ChangeLog:13 >>>>> + But some applications might have service worker enabled while not having set the SW registration storage path. >>>> >>>> Those applications would then just have no service worker persistence, right? >>> >>> Right. I think it is the same behavior for other storage data. >>> The ASSERT was specifically about sessionID setup being available. >> >> Seems wrong for me to have a non-ephemeral session with a SW registration path set. This sounds like a bug. I think the SW registration path should always be set for persistent session, no? Otherwise, we are not *persisting* service workers. > > We do not crash the network process for other data types (IDB, DOM cache, network cache) in case of empty path. I do not think we should do it either for Service Worker. > There is nothing precluding an application to provide a wrong path and we do not crash either. > > I agree the API surface should limit (or better, forbid) this kind of setup. > Ideally, you would need to provide a correct root path for all data for a given session and we would then organise all stored data in that root path appropriately, at least for persistent data like IDB, SW and DOM cache. > This seems like another issue though. There is no API to set the service worker directory, only SPI.

Comment on attachment 388842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388842&action=review >>>>>> Source/WebKit/ChangeLog:13 >>>>>> + But some applications might have service worker enabled while not having set the SW registration storage path. >>>>> >>>>> Those applications would then just have no service worker persistence, right? >>>> >>>> Right. I think it is the same behavior for other storage data. >>>> The ASSERT was specifically about sessionID setup being available. >>> >>> Seems wrong for me to have a non-ephemeral session with a SW registration path set. This sounds like a bug. I think the SW registration path should always be set for persistent session, no? Otherwise, we are not *persisting* service workers. >> >> We do not crash the network process for other data types (IDB, DOM cache, network cache) in case of empty path. I do not think we should do it either for Service Worker. >> There is nothing precluding an application to provide a wrong path and we do not crash either. >> >> I agree the API surface should limit (or better, forbid) this kind of setup. >> Ideally, you would need to provide a correct root path for all data for a given session and we would then organise all stored data in that root path appropriately, at least for persistent data like IDB, SW and DOM cache. >> This seems like another issue though. > > There is no API to set the service worker directory, only SPI. Also, if I read our code properly: 1. WebsiteDataStore has both a WebsiteDataStoreConfiguration (initialized by the client, so will call client configuration) and a *resolved* WebsiteDataStoreConfiguration 2. The WebsiteDataStoreConfiguration gets a default / valid path when constructed if it is persistent 3. When WebsiteDataStore::resolveDirectoriesIfNecessary() called, it only overrides the *resolved* WebsiteDataStoreConfiguration's path if the client configuration is not empty. Therefore, I would not expected the resolved configuration path to ever be empty if it is for a persistent session. This may be a bug where we got the path from the client configuration instead of the resolved configuration? Or some other logic bug somewhere. Alex is likely more familiar with this than I am but I am very confused as to why we end up with en empty path here instead of a default path when the session is persistent.

I believe the crash will be fixed by bug 208851. I added ASSERT over there that catches the underlying bug. Without bug 208851, removing this ASSERT will not help us much, we will end up hitting further ASSERTs that are much clearer. I do no think this is worth keeping this one.

Closing based on Youenn's comments.