Rework NetworkProcess resource load identifiers Currently the WebProcess synchronous waits to hear back from the NetworkProcess when scheduling a load just to get a globally unique identifier for the load. In practice, identifiers do not need to be globally unique. As long as they are unique-per-WebProcess we're fine. So there's no need for the NetworkProcess to vend out identifiers to WebProcesses. By splitting up the notion of "identifier from the WebProcess" and "identifier from the NetworkProcess" - which was started in http://trac.webkit.org/changeset/140038 - we can get rid of a sync message and be more correct with identifiers in WebProcesses. In radar as <rdar://problem/12934449>
Created attachment 183567 [details] Patch v1
Comment on attachment 183567 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=183567&action=review Looks like a good improvement logically. One thing I'm quite unhappy about is making some classes RefCounted. It is always better to have a concrete ownership policy than to hope that RefPtrs will balance out in the end somehow. I'm particularly unhappy about complicating ownership in NetworkProcess code, which is very multithreaded, and RefPtrs to shared objects add unpredictability to it. It's especially bad because we have debug assertions for RefPtr threading errors disabled now. > Source/WebCore/loader/FrameLoader.cpp:2590 > + unsigned long identifier = 0; > + if (m_frame->page()) > + identifier = m_frame->page()->progress()->createUniqueIdentifier(); > + platformStrategies()->loaderStrategy()->loadResourceSynchronously(networkingContext(), identifier, newRequest, storedCredentials, error, response, data); Really, we want to call the strategy with a 0 identifier? > Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:103 > - HostRecord* oldHost = m_identifiers.get(identifier); > + RefPtr<HostRecord> oldHost = loader->hostRecord(); Why can't oldHost be a plain pointer any more?
(In reply to comment #2) > (From update of attachment 183567 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183567&action=review > > Looks like a good improvement logically. > > One thing I'm quite unhappy about is making some classes RefCounted. It is always better to have a concrete ownership policy than to hope that RefPtrs will balance out in the end somehow. I'm particularly unhappy about complicating ownership in NetworkProcess code, which is very multithreaded, and RefPtrs to shared objects add unpredictability to it. It's especially bad because we have debug assertions for RefPtr threading errors disabled now. I agree with your message. This improvement both fixes real bugs and is so much in the right direction that I think it's not difficult to accept the new RefCounting. I hope this improvement makes it easier to spot further improvements that will be more concrete. > > Source/WebCore/loader/FrameLoader.cpp:2590 > > + unsigned long identifier = 0; > > + if (m_frame->page()) > > + identifier = m_frame->page()->progress()->createUniqueIdentifier(); > > + platformStrategies()->loaderStrategy()->loadResourceSynchronously(networkingContext(), identifier, newRequest, storedCredentials, error, response, data); > > Really, we want to call the strategy with a 0 identifier? In practice, this is already how WebCore dealers with "loads for a frame without a page". I agree with your beef, but the sin is much more onerous than this one site! > > Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:103 > > - HostRecord* oldHost = m_identifiers.get(identifier); > > + RefPtr<HostRecord> oldHost = loader->hostRecord(); > > Why can't oldHost be a plain pointer any more? I think that is a remnant from an earlier form of the refactor. Indeed, it can be. Thanks for the review!
http://trac.webkit.org/changeset/140218
> In practice, this is already how WebCore dealers with "loads for a frame without a page". I agree with your beef, but the sin is much more onerous than this one site! Does that happen in practice? I'm worried that everything goes off the rails then.
(In reply to comment #5) > > In practice, this is already how WebCore dealers with "loads for a frame without a page". I agree with your beef, but the sin is much more onerous than this one site! > > Does that happen in practice? I'm worried that everything goes off the rails then. Does it actually *happen*? Hopefully not. Does the code account for the possibility in multiple places (now)? Unfortunately.