Move the ResourceLoadObserver logic to WebKit2 since it is not used by WebKit1. This allows us to simplify code. In a follow-up patch, I will simplify the code even further by leveraging the fact that a WebContent process is always associated with a single WebsiteDataStore / sessionID: - No need for a HashMap of sessionIDs - No need to even allocate the ResourceLoadObserver if the WebProcess is associated with an ephemeral session.
Created attachment 378122 [details] Patch
Created attachment 378123 [details] Patch
Kate Cheney
(In reply to Chris Dumez from comment #3) > Kate Cheney LoL, I was trying to cc.
(In reply to Chris Dumez from comment #0) > Move the ResourceLoadObserver logic to WebKit2 since it is not used by > WebKit1. This allows us to simplify code. > > In a follow-up patch, I will simplify the code even further by leveraging > the fact that a WebContent process is always associated with a single > WebsiteDataStore / sessionID: This is tempting! But this is not (yet) the case. I believe service workers are all being run in the same web process as long as they belong to the same domain, no matter which sessionID they are associated with. We could also think of reusing a web process (which no longer runs a web page but is in the process cache) to run a web page with the same domain but a different session ID. Or even run different pages with different session IDs in the same process. Not sure whether we want to do that but that is something we could easily do with the current infrastructure if we want to. I agree the sessionID infrastructure is adding some complexity. Since it is there and working though, I am a bit hesitant to remove it. There is a lot of related complexity in NetworkProcess as well, the benefits would be even greater there (simplifications, limiting sandbox extensions...). The worry is again the added cost in spinning more processes. Not sure whether this simplification is doable in WebProcessPool.
(In reply to youenn fablet from comment #5) > (In reply to Chris Dumez from comment #0) > > Move the ResourceLoadObserver logic to WebKit2 since it is not used by > > WebKit1. This allows us to simplify code. > > > > In a follow-up patch, I will simplify the code even further by leveraging > > the fact that a WebContent process is always associated with a single > > WebsiteDataStore / sessionID: > > This is tempting! > > But this is not (yet) the case. > I believe service workers are all being run in the same web process as long > as they belong to the same domain, no matter which sessionID they are > associated with. > > We could also think of reusing a web process (which no longer runs a web > page but is in the process cache) to run a web page with the same domain but > a different session ID. Or even run different pages with different session > IDs in the same process. > Not sure whether we want to do that but that is something we could easily do > with the current infrastructure if we want to. > > I agree the sessionID infrastructure is adding some complexity. > Since it is there and working though, I am a bit hesitant to remove it. > > There is a lot of related complexity in NetworkProcess as well, the benefits > would be even greater there (simplifications, limiting sandbox > extensions...). > The worry is again the added cost in spinning more processes. > > Not sure whether this simplification is doable in WebProcessPool. We have assertions in place to make sure that a WebProcess is only associated with a single data store so I’d be surprised if it were not true for service workers. That said, I will take a look. The fact is that web processes can only support one session at the moment and of code tries to do differently, then it is buggy code.
(In reply to Chris Dumez from comment #6) > (In reply to youenn fablet from comment #5) > > (In reply to Chris Dumez from comment #0) > > > Move the ResourceLoadObserver logic to WebKit2 since it is not used by > > > WebKit1. This allows us to simplify code. > > > > > > In a follow-up patch, I will simplify the code even further by leveraging > > > the fact that a WebContent process is always associated with a single > > > WebsiteDataStore / sessionID: > > > > This is tempting! > > > > But this is not (yet) the case. > > I believe service workers are all being run in the same web process as long > > as they belong to the same domain, no matter which sessionID they are > > associated with. > > > > We could also think of reusing a web process (which no longer runs a web > > page but is in the process cache) to run a web page with the same domain but > > a different session ID. Or even run different pages with different session > > IDs in the same process. > > Not sure whether we want to do that but that is something we could easily do > > with the current infrastructure if we want to. > > > > I agree the sessionID infrastructure is adding some complexity. > > Since it is there and working though, I am a bit hesitant to remove it. > > > > There is a lot of related complexity in NetworkProcess as well, the benefits > > would be even greater there (simplifications, limiting sandbox > > extensions...). > > The worry is again the added cost in spinning more processes. > > > > Not sure whether this simplification is doable in WebProcessPool. > > We have assertions in place to make sure that a WebProcess is only > associated with a single data store so I’d be surprised if it were not true > for service workers. That said, I will take a look. The fact is that web > processes can only support one session at the moment and of code tries to do > differently, then it is buggy code. Looks like you're right that if I open the same page in private and non-private browsing, I get a single Service Worker Process. This is a very bad bug that we need to fix ASAP. Sharing WebContent processes in such way is not allowed, as we found while working on PSON. That said, this is not strictly related to the patch at hand here, this will merely impact the next patch to simplify the code.
ping review?
Comment on attachment 378123 [details] Patch This move looks correct to me, but it's troubling that both wincairo and iOS-wk2 seem to be having problems with service workers. Can you confirm these are unrelated to your patch before landing?
(In reply to Brent Fulgham from comment #9) > Comment on attachment 378123 [details] > Patch > > This move looks correct to me, but it's troubling that both wincairo and > iOS-wk2 seem to be having problems with service workers. Can you confirm > these are unrelated to your patch before landing? The iOS-WK2 failures are unrelated (flaky tests, one of them being https://bugs.webkit.org/show_bug.cgi?id=201550). The wincairo issue seems to be a related build failure. I am working on fixing this one before landing.
Created attachment 378248 [details] Patch
Created attachment 378253 [details] Patch
Comment on attachment 378253 [details] Patch Clearing flags on attachment: 378253 Committed r249603: <https://trac.webkit.org/changeset/249603>
All reviewed patches have been landed. Closing bug.
<rdar://problem/55137069>
http://trac.webkit.org/r249617
(In reply to Alex Christensen from comment #16) > http://trac.webkit.org/r249617 Thanks.