[webkit-reviews] review denied: [Bug 106826] Synchronous XMLHttpRequests need to go to the NetworkProcess : [Attachment 182646] Patch v1

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Brady Eidson
<beidson at apple.com>'s request for review:
Bug 106826: Synchronous XMLHttpRequests need to go to the NetworkProcess
https://bugs.webkit.org/show_bug.cgi?id=106826

Attachment 182646: Patch v1
https://bugs.webkit.org/attachment.cgi?id=182646&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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::Del
ayedReply> 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::Ne
tworkConnectionToWebProcess::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.