Bug 107192

Summary: Rework NetworkProcess resource load identifiers
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: 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 Flags
Patch v1 ap: review+

Description Brady Eidson 2013-01-17 15:47:09 PST
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>
Comment 1 Brady Eidson 2013-01-18 16:08:39 PST
Created attachment 183567 [details]
Patch v1
Comment 2 Alexey Proskuryakov 2013-01-18 16:28:27 PST
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?
Comment 3 Brady Eidson 2013-01-18 16:35:22 PST
(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!
Comment 4 Brady Eidson 2013-01-18 16:43:23 PST
http://trac.webkit.org/changeset/140218
Comment 5 Alexey Proskuryakov 2013-01-18 17:11:59 PST
> 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.
Comment 6 Brady Eidson 2013-01-18 17:17:38 PST
(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.