Summary: | Rework NetworkProcess resource load identifiers | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||
Component: | WebKit2 | Assignee: | Brady Eidson <beidson> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, ap, japhet, webkit.review.bot | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Mac | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Brady Eidson
2013-01-17 15:47:09 PST
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! > 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. |