Bug 106826 – Synchronous XMLHttpRequests need to go to the NetworkProcess

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Synchronous XMLHTTPRequests need to go to the NetworkProcess

In radar as <rdar://problem/12951765>

[Brady Eidson]

Created attachment 182646 [details]
Patch v1

[WebKit Review Bot]

Attachment 182646 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:79:  The parameter name "reply" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

(In reply to comment #2)
> Attachment 182646 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:79:  The parameter name "reply" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Total errors found: 1 in 19 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Fixed locally.

[Alexey Proskuryakov]

Comment on attachment 182646 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=182646&action=review

> Source/WebCore/platform/network/NetworkingContext.h:28
> +#include "ResourceHandle.h"

This include will cause much longer rebuilds when changing ResourceHandle.h. It's just for StoredCredentials enum, please find another way to pass this argument, or move the enum to another file.

> Source/WebCore/platform/network/NetworkingContext.h:80
> +    virtual bool handledSynchronousLoad(const ResourceRequest&, StoredCredentials, ResourceError&, ResourceResponse&, Vector<char>&) { return false; }

This makes me quite unhappy. NetworkingContext is a bad beast already, but at least it mostly provides policy functions. A complete load implementation is way out of place here.

There is another patch posted for review where ResourceHandle::loadResourceSynchronously() will consult NetworkingContext for referer policy. Together, these will create a circular dependency mess.

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:370
> +    if (context->handledSynchronousLoad(request, storedCredentials, error, response, data))
> +        return;

I understand that you tried to keep these changes port-spefic, but it's not right to drop down all the way to ResourceHandleMac, which is a wrapper for CFNetwork, and then pass control back to WebKit.

> Source/WebKit2/NetworkProcess/HostRecord.cpp:66
> +void HostRecord::scheduleSync(PassRefPtr<SyncResourceLoader> loader)
> +{
> +    m_syncLoadersPending.append(loader);
> +}

It's unpleasantly asymmetric that adding has a member function, but removing is done via syncLoadersPending().

> Source/WebKit2/NetworkProcess/HostRecord.h:68
> +    HashSet<RefPtr<SyncResourceLoader> > m_syncLoadersLoading;

Why does this need to keep a reference? Can SyncResourceLoader destruction notify relevant HostRecords?

> Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:195
> +    // We serve synchronous requests before any other requests as it doesn't make sense to spend resources performing

Perhaps "improves responsiveness" would be more positive and descriptive than "doesn't make sense".

> Source/WebKit2/NetworkProcess/SyncResourceLoader.cpp:36
> +#if ENABLE(NETWORK_PROCESS)

I think that most files have such checks after the first two includes, slightly improving build times when disabled.

> Source/WebKit2/NetworkProcess/SyncResourceLoader.cpp:42
> +SyncResourceLoader::SyncResourceLoader(const NetworkResourceLoadParameters& parameters, PassRefPtr<Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad::DelayedReply> reply)

Should this class be named SyncNetworkResourceLoader for consistency with NetworkResourceLoader?

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:185
> +    // FIXME (NetworkProcess): Get the right value for private browsing enabled

I don't think that this can be left on a FIXME. Private browsing works now, so breaking it is a fairly big regression.

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:187
> +    if (!WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad(loadParameters), Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad::Reply(error, response, dataReference), 0)) {

Will async response messages be delivered to WebProcess during this call? Both "yes" and "no" answers will leave me very concerned, because 
(1) handling network events during a sync XHR call will cause reentrance in JavaScript code, and
(2) not handling them would cause deadlocks, because there may be already six requests to this host, and they need to finish somehow.

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:192
> +        return true;

This looks like a typo, but is actually correct. That's worth a comment (but I as mentioned above, think that this should be refactored).

> Source/WebKit2/WebProcess/Network/WebResourceLoader.h:35
> +#include <WebCore/ResourceHandle.h>

Again, I hope that we won't need this include.

[Adam Barth]

Comment on attachment 182646 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=182646&action=review

>> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:370
>> +        return;
> 
> I understand that you tried to keep these changes port-spefic, but it's not right to drop down all the way to ResourceHandleMac, which is a wrapper for CFNetwork, and then pass control back to WebKit.

I would much prefer if we picked a consistent design for how the NetworkProcess interfaces with WebCore.  For example, why are we using NetworkingContext here when in other places we're using PlatformStrategies?

[Alexey Proskuryakov]

Thinking about this more - perhaps we have the same deadlock in single process case too?

[Brady Eidson]

(In reply to comment #5)
> (From update of attachment 182646 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182646&action=review
> 
> >> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:370
> >> +        return;
> > 
> > I understand that you tried to keep these changes port-spefic, but it's not right to drop down all the way to ResourceHandleMac, which is a wrapper for CFNetwork, and then pass control back to WebKit.

While I admit it's not super clean, I don't have nearly as hard a time accepting this.

ResourceHandle* is a wrapper for "platform networking implementations".  The fact that one of our platform network implementations is implemented by the WK2 network process is an implementation detail that WebCore doesn't need to care about.

But...

> 
> I would much prefer if we picked a consistent design for how the NetworkProcess interfaces with WebCore.  For example, why are we using NetworkingContext here when in other places we're using PlatformStrategies?

The first implementation of this used LoaderStrategy, consistent with previous work.  This included an #ifdef block inside of FrameLoader.

A brief discussion around the office actually centered around the fact that you personally have objected to some of the cross platform sites where we've had to hook in the NetworkProcess.

Your past objections on this are why I refactored this in to something that affected only our platform and therefore you could have no objection to its impact on cross-platform code.

It seems both Alexey's objections and your comment here means I should revert to the LoaderStrategy form of this and hook back in to FrameLoader.

[Brady Eidson]

(In reply to comment #4)
> (From update of attachment 182646 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182646&action=review
> 
> > Source/WebCore/platform/network/NetworkingContext.h:28
> > +#include "ResourceHandle.h"
> 
> This include will cause much longer rebuilds when changing ResourceHandle.h. It's just for StoredCredentials enum, please find another way to pass this argument, or move the enum to another file.

I felt bad about this, too.  I'll move the enum to another file.

> 
> > Source/WebCore/platform/network/NetworkingContext.h:80
> > +    virtual bool handledSynchronousLoad(const ResourceRequest&, StoredCredentials, ResourceError&, ResourceResponse&, Vector<char>&) { return false; }
> 
> This makes me quite unhappy. NetworkingContext is a bad beast already, but at least it mostly provides policy functions. A complete load implementation is way out of place here.

This was only necessary due to the refactoring to make this affect only our platform.  As mentioned in the comment I just made, it seems like I should go back to the LoaderStrategy version of the patch which makes this unnecessary.


> > Source/WebKit2/NetworkProcess/HostRecord.cpp:66
> > +void HostRecord::scheduleSync(PassRefPtr<SyncResourceLoader> loader)
> > +{
> > +    m_syncLoadersPending.append(loader);
> > +}
> 
> It's unpleasantly asymmetric that adding has a member function, but removing is done via syncLoadersPending().

Bizarrely that asymmetry came about during the LoaderStrategy -> ResourceHandle refactoring.  I will address it when reworking the patch.

> 
> > Source/WebKit2/NetworkProcess/HostRecord.h:68
> > +    HashSet<RefPtr<SyncResourceLoader> > m_syncLoadersLoading;
> 
> Why does this need to keep a reference? Can SyncResourceLoader destruction notify relevant HostRecords?

That is not how the HostRecord mechanism works with regards to normal ResourceLoaders, so I'm not sure it's prudent to break symettry for sync XHRs.  Especially when we've had discussions about making sync XHR's work with normal subresource loaders which would change this implementation.

> 
> > Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:195
> > +    // We serve synchronous requests before any other requests as it doesn't make sense to spend resources performing
> 
> Perhaps "improves responsiveness" would be more positive and descriptive than "doesn't make sense".

Agreed.

> > Source/WebKit2/NetworkProcess/SyncResourceLoader.cpp:36
> > +#if ENABLE(NETWORK_PROCESS)
> 
> I think that most files have such checks after the first two includes, slightly improving build times when disabled.

Yup.

> > Source/WebKit2/NetworkProcess/SyncResourceLoader.cpp:42
> > +SyncResourceLoader::SyncResourceLoader(const NetworkResourceLoadParameters& parameters, PassRefPtr<Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad::DelayedReply> reply)
> 
> Should this class be named SyncNetworkResourceLoader for consistency with NetworkResourceLoader?

Makes sense.

> > Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:185
> > +    // FIXME (NetworkProcess): Get the right value for private browsing enabled
> 
> I don't think that this can be left on a FIXME. Private browsing works now, so breaking it is a fairly big regression.

I believe there is another comment literally identical to this one and that I copied this comment from it.  I will double check that in the morning.

If my memory is correct, that goes at odds with your claim that private browsing works now.

> > Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:187
> > +    if (!WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad(loadParameters), Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad::Reply(error, response, dataReference), 0)) {
> 
> Will async response messages be delivered to WebProcess during this call? Both "yes" and "no" answers will leave me very concerned, because 
> (1) handling network events during a sync XHR call will cause reentrance in JavaScript code, and
> (2) not handling them would cause deadlocks, because there may be already six requests to this host, and they need to finish somehow.

Async response messages from the NetworkProcess will get queued up in the CoreIPC pipeline.  So the async loads will complete and the sync load will be serviced.

Those response messages are *not* delivered to the WebProcess in the meantime as it is waiting for the synchronous reply to this load.  So in the NetworkProcess the async loads actually complete before the sync load starts.  But from the WebProcesses perspective the sync load completes and then the messages for the async loads come through.

Perhaps there is yet another edge case scenario I have not fully thought through or commented on, but both (1) or (2) that you present here are accounted for.

Thanks for your thorough review - Hopefully the LoaderStrategy form of this that I'll get up tomorrow afternoon will address your feedback.

[Adam Barth]

> > I would much prefer if we picked a consistent design for how the NetworkProcess interfaces with WebCore.  For example, why are we using NetworkingContext here when in other places we're using PlatformStrategies?
> 
> The first implementation of this used LoaderStrategy, consistent with previous work.  This included an #ifdef block inside of FrameLoader.
> 
> A brief discussion around the office actually centered around the fact that you personally have objected to some of the cross platform sites where we've had to hook in the NetworkProcess.
> 
> Your past objections on this are why I refactored this in to something that affected only our platform and therefore you could have no objection to its impact on cross-platform code.
> 
> It seems both Alexey's objections and your comment here means I should revert to the LoaderStrategy form of this and hook back in to FrameLoader.

I do think it would be better to intercept requests at the ResourceHandle level, but I think we should do that for all sorts of network requests instead of having a hodge-podge of different approaches.

My guess is that if you looked at a complete design for intercepting network requests at the ResourceHandle, you wouldn't do it by having a bunch of handledFooLoad functions on NetworkContext.  I suspect you'd want to elaborate LoaderStrategy with the ability to create objects to handle each load (e.g., similar to how WebURLLoader works in Chromium).

Maybe you have some clear picture in mind of the design you're aiming for, but that's not at all clear from reading the individual patches.

[Alexey Proskuryakov]

> Those response messages are *not* delivered to the WebProcess in the meantime as it is waiting for the synchronous reply to this load.  

Ok.

> So in the NetworkProcess the async loads actually complete before the sync load starts. 

Unfortunately, this is not true if the async loads get redirected, or get an authentication request. In this case, they will wait for a response from WebProcess forever.

In fact, this might have changed with my recent refactoring, as we now use sync messages in these cases, and sync messages are probably actually delivered to WebProcess. And thankfully, XHR doesn't (yet?!!!) send DOM events for either.

[Alexey Proskuryakov]

> And thankfully, XHR doesn't (yet?!!!) send DOM events for either.

More precisely, nothing in the Web platform involves such DOM events AFAICT, or we'd have to dispatch these events during sync XHR to avoid deadlocks.

[Brady Eidson]

(In reply to comment #9)
> > > I would much prefer if we picked a consistent design for how the NetworkProcess interfaces with WebCore.  For example, why are we using NetworkingContext here when in other places we're using PlatformStrategies?
> > 
> > The first implementation of this used LoaderStrategy, consistent with previous work.  This included an #ifdef block inside of FrameLoader.
> > 
> > A brief discussion around the office actually centered around the fact that you personally have objected to some of the cross platform sites where we've had to hook in the NetworkProcess.
> > 
> > Your past objections on this are why I refactored this in to something that affected only our platform and therefore you could have no objection to its impact on cross-platform code.
> > 
> > It seems both Alexey's objections and your comment here means I should revert to the LoaderStrategy form of this and hook back in to FrameLoader.
> 
> I do think it would be better to intercept requests at the ResourceHandle level, but I think we should do that for all sorts of network requests instead of having a hodge-podge of different approaches.
> 
> My guess is that if you looked at a complete design for intercepting network requests at the ResourceHandle, you wouldn't do it by having a bunch of handledFooLoad functions on NetworkContext. I suspect you'd want to elaborate LoaderStrategy with the ability to create objects to handle each load (e.g., similar to how WebURLLoader works in Chromium).

Of course this is what we'd want to do, and how this patch was developed.

But since you have been resistant to that approaches effect on cross platform code in the general ResourceLoader case, that's why I let concern for WebKit politics to creep in to design in this case.

It seems like you're in agreement that we should continue to expand upon LoaderStrategy, so it seems like the patch I'll get back up here later today won't meet with resistance on that detail.

[Brady Eidson]

Created attachment 182884 [details]
Patch v2 - Use LoaderStrategy

[Sam Weinig]

(In reply to comment #9)
> Maybe you have some clear picture in mind of the design you're aiming for, but that's not at all clear from reading the individual patches.

The design we are aiming for is for all loading to eventually go through the ResourceLoader, and have ResourceHandle retain its original purpose, to be a thin wrapper around OS level URL loading code (e.g. NSURLConnection on Mac).  Since ResourceLoader does not support sync loading yet, this is a stopgap so we can continue working, and we are trying to do it a way that least impacts other ports.  Brady will be looking at adding support for sync loading to ResourceLoader.

[Alexey Proskuryakov]

Comment on attachment 182884 [details]
Patch v2 - Use LoaderStrategy

View in context: https://bugs.webkit.org/attachment.cgi?id=182884&action=review

As discussed above, this is only safe because NetworkResourceLoader uses sync messages for willSendRequest and authentication now. Please add very explicit comments about this to NetworkResourceLoader.cpp (we should either use sync messages, or pass special flags for async messages, so that they get delivered while waiting for synchronous load).

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:1292
> -		49484FC5102CF23C00187DD3 /* CanvasPattern.h in Headers */ = {isa = PBXBuildFile; fileRef = 49484FB7102CF23C00187DD3 /* CanvasPattern.h */; };
>  		49484FC4102CF23C00188DD3 /* CanvasProxy.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 49484FB6102CF23C00188DD3 /* CanvasProxy.cpp */; };
> +		49484FC5102CF23C00187DD3 /* CanvasPattern.h in Headers */ = {isa = PBXBuildFile; fileRef = 49484FB7102CF23C00187DD3 /* CanvasPattern.h */; };

This change looks wrong.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:1864
> -		65DF323C09D1DE65000BE325 /* JSCanvasPattern.h in Headers */ = {isa = PBXBuildFile; fileRef = 65DF323609D1DE65000BE325 /* JSCanvasPattern.h */; };
>  		65DF323B09D1DE65001BE325 /* JSCanvasProxy.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 65DF323509D1DE65001BE325 /* JSCanvasProxy.cpp */; };
> +		65DF323C09D1DE65000BE325 /* JSCanvasPattern.h in Headers */ = {isa = PBXBuildFile; fileRef = 65DF323609D1DE65000BE325 /* JSCanvasPattern.h */; };

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:8582
> -		49484FB7102CF23C00187DD3 /* CanvasPattern.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = CanvasPattern.h; path = canvas/CanvasPattern.h; sourceTree = "<group>"; };
> -		49484FB8102CF23C00187DD3 /* CanvasPattern.idl */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; name = CanvasPattern.idl; path = canvas/CanvasPattern.idl; sourceTree = "<group>"; };
>  		49484FB6102CF23C00188DD3 /* CanvasProxy.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = CanvasProxy.cpp; path = canvas/CanvasProxy.cpp; sourceTree = "<group>"; };
> +		49484FB7102CF23C00187DD3 /* CanvasPattern.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = CanvasPattern.h; path = canvas/CanvasPattern.h; sourceTree = "<group>"; };
>  		49484FB7102CF23C00188DD3 /* CanvasProxy.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = CanvasProxy.h; path = canvas/CanvasProxy.h; sourceTree = "<group>"; };
> +		49484FB8102CF23C00187DD3 /* CanvasPattern.idl */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; name = CanvasPattern.idl; path = canvas/CanvasPattern.idl; sourceTree = "<group>"; };

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:9199
> -		65DF323609D1DE65000BE325 /* JSCanvasPattern.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; path = JSCanvasPattern.h; sourceTree = "<group>"; };
>  		65DF323509D1DE65001BE325 /* JSCanvasProxy.cpp */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.cpp; path = JSCanvasProxy.cpp; sourceTree = "<group>"; };
> +		65DF323609D1DE65000BE325 /* JSCanvasPattern.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; path = JSCanvasPattern.h; sourceTree = "<group>"; };

Ditto.

How did this happen?

> Source/WebCore/loader/FrameLoader.cpp:2588
> +            PrivateBrowsing privateBrowsing = m_frame->settings()->privateBrowsingEnabled() ? PrivateBrowsingEnabled : PrivateBrowsingDisabled;
> +            platformStrategies()->loaderStrategy()->loadResourceSynchronously(networkingContext(), newRequest, storedCredentials, privateBrowsing, error, response, data);

A better way to get private browsing state is via session.isPrivateBrowsingSession(). Please see how it's done in e.g. WebPlatformStrategies::cookiesForDOM().

> Source/WebCore/loader/LoaderStrategy.h:38
>  class ResourceLoadScheduler;
> +class ResourceError;

Alphabetic ordering.

> Source/WebCore/loader/LoaderStrategy.h:45
> +enum PrivateBrowsing {
> +    PrivateBrowsingDisabled,
> +    PrivateBrowsingEnabled
> +};

This won't be needed.

> Source/WebKit2/NetworkProcess/SyncNetworkResourceLoader.h:29
> +#include "NetworkConnectionToWebProcessMessages.h"

This won't be needed if you forward declare Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad::DelayedReply, and add an explicit destructor implemented in .cpp file.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:221
> +    if (!WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad(loadParameters), Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad::Reply(error, response, dataReference), 0)) {

I think that a bad thing can happen with IPC here. If there is an outstanding willSendRequest or authentication message from NetworkProcess, PerformSynchronousLoad will be delivered reentrantly, and block that outstanding load until synchronous loading finishes.

Is there a way to say that PerformSynchronousLoad should not be dispatched while waiting for sync message reply? I think that it would be a solution for that.

[Alexey Proskuryakov]

> I think that a bad thing can happen with IPC here.

Actually, unsure - the messages are sent from a secondary thread in NetworkProcess, so there is not reply stack at play. Maybe it's safe. I can't think about this deeply right now.

[Brady Eidson]

(In reply to comment #15)
> (From update of attachment 182884 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182884&action=review
> 
> As discussed above, this is only safe because NetworkResourceLoader uses sync messages for willSendRequest and authentication now. Please add very explicit comments about this to NetworkResourceLoader.cpp (we should either use sync messages, or pass special flags for async messages, so that they get delivered while waiting for synchronous load).
> 

> This change looks wrong.
> 
...
> Ditto.
> 
...
> Ditto.
> 
...
> Ditto.
> 
> How did this happen?

Xcode does this periodically.  It's happened to me a few times before, and has happened to others.  You've never seen it?

We've normally checked it in with a "let Xcode have its way with us" mindset.

> 
> > Source/WebCore/loader/FrameLoader.cpp:2588
> > +            PrivateBrowsing privateBrowsing = m_frame->settings()->privateBrowsingEnabled() ? PrivateBrowsingEnabled : PrivateBrowsingDisabled;
> > +            platformStrategies()->loaderStrategy()->loadResourceSynchronously(networkingContext(), newRequest, storedCredentials, privateBrowsing, error, response, data);
> 
> A better way to get private browsing state is via session.isPrivateBrowsingSession(). Please see how it's done in e.g. WebPlatformStrategies::cookiesForDOM().

Will do this.
> 
> > Source/WebCore/loader/LoaderStrategy.h:38
> >  class ResourceLoadScheduler;
> > +class ResourceError;
> 
> Alphabetic ordering.

Done.

> 
> > Source/WebCore/loader/LoaderStrategy.h:45
> > +enum PrivateBrowsing {
> > +    PrivateBrowsingDisabled,
> > +    PrivateBrowsingEnabled
> > +};
> 
> This won't be needed.

Good.

> 
> > Source/WebKit2/NetworkProcess/SyncNetworkResourceLoader.h:29
> > +#include "NetworkConnectionToWebProcessMessages.h"
> 
> This won't be needed if you forward declare Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad::DelayedReply, and add an explicit destructor implemented in .cpp file.
> 

Ugh, but, okay.

> > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:221
> > +    if (!WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad(loadParameters), Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad::Reply(error, response, dataReference), 0)) {
> 
> I think that a bad thing can happen with IPC here. If there is an outstanding willSendRequest or authentication message from NetworkProcess, PerformSynchronousLoad will be delivered reentrantly, and block that outstanding load until synchronous loading finishes.

As you established yourself in the next comment, I also don't think this will be an issue.  We've made CoreIPC pretty resilient against this type of issue as its come up before.

[Alexey Proskuryakov]

> Xcode does this periodically.  It's happened to me a few times before, and has happened to others.  You've never seen it?

Not like this - it looks like visual order may be broken.

> Please add very explicit comments about this to NetworkResourceLoader.cpp

You didn't say "yes" to this global comment.

[Brady Eidson]

(In reply to comment #17)
> (In reply to comment #15)
> > (From update of attachment 182884 [details] [details])

> > > Source/WebKit2/NetworkProcess/SyncNetworkResourceLoader.h:29
> > > +#include "NetworkConnectionToWebProcessMessages.h"
> > 
> > This won't be needed if you forward declare Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad::DelayedReply, and add an explicit destructor implemented in .cpp file.
> > 
> 
> Ugh, but, okay.

Actually, PerformSynchronousLoad is a class and PerformSynchronousLoad::DelayedReply is a nested class, and afaik you can't forward declare nested classes without having the definition of the outer class.  Doing that would require the #include in this case.

Googling around seems to confirm this...  am I wrong?

[Brady Eidson]

(In reply to comment #18)
> > Xcode does this periodically.  It's happened to me a few times before, and has happened to others.  You've never seen it?
> 
> Not like this - it looks like visual order may be broken.

This precise thing has happened to me before.  And while the order is broken in the project in this diff, the visual order is definitely not broken.

> 
> > Please add very explicit comments about this to NetworkResourceLoader.cpp
> 
> You didn't say "yes" to this global comment.

Yes.

[Brady Eidson]

I'll try removing my new file, reverting the project, and re-adding it.  In my experience that re-causes the nastiness

[Alexey Proskuryakov]

I tried this just today, and it worked for me when using clang. I don't know if this is something new or non-standard.

class PerformSynchronousLoad;
class PerformSynchronousLoad::DelayedReply;

[Brady Eidson]

(In reply to comment #21)
> I'll try removing my new file, reverting the project, and re-adding it.  In my experience that re-causes the nastiness

Yup.  Re-adding it caused the exact same breakage.  Note that the project file itself - in these sections - does not have alphabetical order.

[Brady Eidson]

(In reply to comment #22)
> I tried this just today, and it worked for me when using clang. I don't know if this is something new or non-standard.
> 
> class PerformSynchronousLoad;
> class PerformSynchronousLoad::DelayedReply;

I was using struct, since they're structs in the header, but same concept.  Also fails with class.


Replacing the #include with:
namespace Messages {
namespace NetworkConnectionToWebProcess {

struct PerformSynchronousLoad;
struct PerformSynchronousLoad::DelayedReply;

}
}

Results in:
/Volumes/SSD-Data/git/OpenSource/Source/WebKit2/NetworkProcess/SyncNetworkResourceLoader.h:38:8: Incomplete type 'Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad' named in nested name specifier

[Brady Eidson]

So if it's working for you, perhaps you have a newer clang that supports it.  But it does appear to be nonstandard, and the clang I'm using is a supported config for WebKit2, so it seems like we can't do this particular optimization.

[Brady Eidson]

Will hash out the rest later, need to leave now.

[Alexey Proskuryakov]

> So if it's working for you, perhaps you have a newer clang that supports it.

I think that what happened was that I wrote such code, quickly refactored it away even before building, and decided that it built successfully when there were no errors later.

[Adam Barth]

@Sam: Thanks.

[Brady Eidson]

Created attachment 183033 [details]
Final patch for landing - Give it an EWS run

[Brady Eidson]

Created attachment 183035 [details]
Super-final (updated to ToT and the patch *TOTALLY* applies locally...)

[WebKit Review Bot]

Attachment 183035 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

(In reply to comment #31)

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]

Fixed locally.

[Brady Eidson]

Super final patch is much smaller because the xcodeproj nonsense went away!

I wish someone knew what that's all about in the first place.

[Brady Eidson]

It's been hours - gtk ews is a slacker.  Landing this without waiting on it to finish.

[Brady Eidson]

http://trac.webkit.org/changeset/139935