Bug 201517 – Move the ResourceLoadObserver logic to WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 378122 [details]
Patch

[Chris Dumez]

Created attachment 378123 [details]
Patch

[Chris Dumez]

Kate Cheney

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> Kate Cheney

LoL, I was trying to cc.

[youenn fablet]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

ping review?

[Brent Fulgham]

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?

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 378248 [details]
Patch

[Chris Dumez]

Created attachment 378253 [details]
Patch

[Chris Dumez]

Comment on attachment 378253 [details]
Patch

Clearing flags on attachment: 378253

Committed r249603: <https://trac.webkit.org/changeset/249603>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55137069>

[Alex Christensen]

http://trac.webkit.org/r249617

[Chris Dumez]

(In reply to Alex Christensen from comment #16)
> http://trac.webkit.org/r249617

Thanks.