Bug 99864

Created attachment 169686 [details] patch

Comment on attachment 169686 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=169686&action=review > Source/WebCore/loader/SubresourceLoader.cpp:154 > + if (newRequest.isNull()) { > + cancel(frameLoader()->client()->interruptedForPolicyChangeError(newRequest)); > + return; > + } > + m_resource->updateResourceRequest(newRequest); This part puzzles me. Why is this needed? > Source/WebCore/loader/cache/CachedRawResource.cpp:122 > + if (type() == MainResource) > + return; Should the callers do this instead? It seems odd for the CachedRawResource to filter out these headers. > Source/WebCore/loader/cache/CachedRawResource.h:69 > + virtual void addAdditionalRequestHeaders(CachedResourceLoader*); Could you be willing to add the OVERRIDE keyword here?

(In reply to comment #2) > (From update of attachment 169686 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169686&action=review > > > Source/WebCore/loader/SubresourceLoader.cpp:154 > > + if (newRequest.isNull()) { > > + cancel(frameLoader()->client()->interruptedForPolicyChangeError(newRequest)); > > + return; > > + } > > + m_resource->updateResourceRequest(newRequest); > > This part puzzles me. Why is this needed? The if statement is because, for reasons I haven't quite nailed down, if we don't manually cancel there on a null request, we end up getting valid didReceiveReponse() calls. I should look more into that. The updateResourceRequest() call is necessary because DocumentLoader expects to be kept fully update date on the ResourceRequest being used for a main resource load. Most notably, without some way of plumbing this out here, document.referrer breaks in the case where willSendRequest() calls out to the WebKit layer and changes the referrer. This is because document.referrer just reads the referrer header off of DocumentLoader::m_request. This manifests in the test http/tests/security/no-referrer.html, which calls testRunner.setWillSendRequestClearHeader("Referer"). There may be a better way of plumbing that data, let me play with it. > > > Source/WebCore/loader/cache/CachedRawResource.cpp:122 > > + if (type() == MainResource) > > + return; > > Should the callers do this instead? It seems odd for the CachedRawResource to filter out these headers. > > > Source/WebCore/loader/cache/CachedRawResource.h:69 > > + virtual void addAdditionalRequestHeaders(CachedResourceLoader*); > > Could you be willing to add the OVERRIDE keyword here? I figured it was a tossup between doing it this way or just having a similar check before the only callsite in CachedResource::load(). Given that it's only one callsite, I think your way may be better. In that case, the OVERRIDE keyword shouldn't be necessary, as this won't need to be virtual.

Created attachment 169707 [details] Address abarth's comments In the interest of simplification, I changed the cancel() call in SubresourceLoader::willSendRequest() to be a regular cancel, rather than using a custom ResourceError type. I don't think that error was particularly meaningful, as it's the result of MainResourceLoader calling PolicyChecker::checkNavigationPolicy() with a null ResourceRequest. It might as well be a general cancellation, as that's effectively what the WebKit layer is doing by nulling out a ResourceRequest. That will mean one more test with changing expectations when this code comes into use.

Comment on attachment 169707 [details] Address abarth's comments This looks great. Thanks for updating the patch.

Comment on attachment 169707 [details] Address abarth's comments Rejecting attachment 169707 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: unk #3 FAILED at 287. 1 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/loader/SubresourceLoader.cpp.rej patching file Source/WebCore/platform/network/ResourceLoadPriority.h patching file Source/WebCore/platform/network/cf/ResourceRequestCFNet.h patching file Source/WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/14552557

Created attachment 170693 [details] Merge + change CachedResourceLoader call The only non-merge change here is that instead of CachedResourceLoader::requestRawResource() taking a CachedResource::Type, I just split it out into a separate requestMainResource() function. This better matches how CachedResourceLoader functions are formatted post-r132157.

Comment on attachment 170693 [details] Merge + change CachedResourceLoader call ok

Comment on attachment 170693 [details] Merge + change CachedResourceLoader call Clearing flags on attachment: 170693 Committed r132520: <http://trac.webkit.org/changeset/132520>

All reviewed patches have been landed. Closing bug.

This caused the following build error on Mac: /OpenSource/Source/WebCore/loader/cache/CachedRawResource.cpp:112:24: error: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long long') to 'unsigned long' [-Werror,-Wshorten-64-to-32] m_identifier = m_loader->identifier(); ~ ^~~~~~~~~~~~~~~~~~~~~~

Please ignore my last comment.

(In reply to comment #11) > This caused the following build error on Mac: > > /OpenSource/Source/WebCore/loader/cache/CachedRawResource.cpp:112:24: error: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long long') to 'unsigned long' [-Werror,-Wshorten-64-to-32] > m_identifier = m_loader->identifier(); > ~ ^~~~~~~~~~~~~~~~~~~~~~ (In reply to comment #12) > Please ignore my last comment. Where did this comment belong? If you can't comment in a bugzilla can you email me?