diff options
5 files changed, 123 insertions, 21 deletions
diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc b/content/browser/loader/resource_dispatcher_host_impl.cc index 87d949a..4b0637d 100644 --- a/content/browser/loader/resource_dispatcher_host_impl.cc +++ b/content/browser/loader/resource_dispatcher_host_impl.cc @@ -1009,10 +1009,13 @@ void ResourceDispatcherHostImpl::UpdateRequestForTransfer( GlobalRequestID new_request_id(child_id, request_id); // Clear out data that depends on |info| before updating it. + // We always need to move the memory stats to the new process. In contrast, + // stats.num_requests is only tracked for some requests (those that require + // file descriptors for their shared memory buffer). IncrementOutstandingRequestsMemory(-1, *info); - OustandingRequestsStats empty_stats = { 0, 0 }; - OustandingRequestsStats old_stats = GetOutstandingRequestsStats(*info); - UpdateOutstandingRequestsStats(*info, empty_stats); + bool should_update_count = info->counted_as_in_flight_request(); + if (should_update_count) + IncrementOutstandingRequestsCount(-1, info); pending_loaders_.erase(old_request_id); // ResourceHandlers should always get state related to the request from the @@ -1025,8 +1028,9 @@ void ResourceDispatcherHostImpl::UpdateRequestForTransfer( // Update maps that used the old IDs, if necessary. Some transfers in tests // do not actually use a different ID, so not all maps need to be updated. pending_loaders_[new_request_id] = loader; - UpdateOutstandingRequestsStats(*info, old_stats); IncrementOutstandingRequestsMemory(1, *info); + if (should_update_count) + IncrementOutstandingRequestsCount(1, info); if (old_routing_id != new_routing_id) { if (blocked_loaders_map_.find(old_routing_id) != blocked_loaders_map_.end()) { @@ -1734,23 +1738,28 @@ ResourceDispatcherHostImpl::IncrementOutstandingRequestsMemory( ResourceDispatcherHostImpl::OustandingRequestsStats ResourceDispatcherHostImpl::IncrementOutstandingRequestsCount( int count, - const ResourceRequestInfoImpl& info) { + ResourceRequestInfoImpl* info) { DCHECK_EQ(1, abs(count)); num_in_flight_requests_ += count; - OustandingRequestsStats stats = GetOutstandingRequestsStats(info); + // Keep track of whether this request is counting toward the number of + // in-flight requests for this process, in case we need to transfer it to + // another process. This should be a toggle. + DCHECK_NE(info->counted_as_in_flight_request(), count > 0); + info->set_counted_as_in_flight_request(count > 0); + + OustandingRequestsStats stats = GetOutstandingRequestsStats(*info); stats.num_requests += count; DCHECK_GE(stats.num_requests, 0); - UpdateOutstandingRequestsStats(info, stats); + UpdateOutstandingRequestsStats(*info, stats); return stats; } bool ResourceDispatcherHostImpl::HasSufficientResourcesForRequest( - const net::URLRequest* request_) { - const ResourceRequestInfoImpl* info = - ResourceRequestInfoImpl::ForRequest(request_); - OustandingRequestsStats stats = IncrementOutstandingRequestsCount(1, *info); + net::URLRequest* request) { + ResourceRequestInfoImpl* info = ResourceRequestInfoImpl::ForRequest(request); + OustandingRequestsStats stats = IncrementOutstandingRequestsCount(1, info); if (stats.num_requests > max_num_in_flight_requests_per_process_) return false; @@ -1761,10 +1770,9 @@ bool ResourceDispatcherHostImpl::HasSufficientResourcesForRequest( } void ResourceDispatcherHostImpl::FinishedWithResourcesForRequest( - const net::URLRequest* request_) { - const ResourceRequestInfoImpl* info = - ResourceRequestInfoImpl::ForRequest(request_); - IncrementOutstandingRequestsCount(-1, *info); + net::URLRequest* request) { + ResourceRequestInfoImpl* info = ResourceRequestInfoImpl::ForRequest(request); + IncrementOutstandingRequestsCount(-1, info); } void ResourceDispatcherHostImpl::StartNavigationRequest( diff --git a/content/browser/loader/resource_dispatcher_host_impl.h b/content/browser/loader/resource_dispatcher_host_impl.h index b4548ba..91a384b 100644 --- a/content/browser/loader/resource_dispatcher_host_impl.h +++ b/content/browser/loader/resource_dispatcher_host_impl.h @@ -249,12 +249,12 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl // sending it to the renderer. Returns true if there are enough file // descriptors available for the shared memory buffer. If false is returned, // the request should cancel. - bool HasSufficientResourcesForRequest(const net::URLRequest* request_); + bool HasSufficientResourcesForRequest(net::URLRequest* request); // Called by a ResourceHandler after it has finished its request and is done // using its shared memory buffer. Frees up that file descriptor to be used // elsewhere. - void FinishedWithResourcesForRequest(const net::URLRequest* request_); + void FinishedWithResourcesForRequest(net::URLRequest* request); // PlzNavigate // Called by NavigationRequest to start a navigation request in the node @@ -343,12 +343,12 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl int count, const ResourceRequestInfoImpl& info); - // Called every time an in flight request is issued or finished. |count| - // indicates whether the request is issuing or finishing. |count| must be 1 - // or -1. + // Called when an in flight request allocates or releases a shared memory + // buffer. |count| indicates whether the request is issuing or finishing. + // |count| must be 1 or -1. OustandingRequestsStats IncrementOutstandingRequestsCount( int count, - const ResourceRequestInfoImpl& info); + ResourceRequestInfoImpl* info); // Estimate how much heap space |request| will consume to run. static int CalculateApproximateMemoryCost(net::URLRequest* request); diff --git a/content/browser/loader/resource_dispatcher_host_unittest.cc b/content/browser/loader/resource_dispatcher_host_unittest.cc index 31ed3f1..ecbdaae 100644 --- a/content/browser/loader/resource_dispatcher_host_unittest.cc +++ b/content/browser/loader/resource_dispatcher_host_unittest.cc @@ -2286,6 +2286,88 @@ TEST_F(ResourceDispatcherHostTest, TransferNavigationHtml) { CheckSuccessfulRequest(msgs[1], kResponseBody); } +// Test transferring two navigations with text/html, to ensure the resource +// accounting works. +TEST_F(ResourceDispatcherHostTest, TransferTwoNavigationsHtml) { + // This test expects the cross site request to be leaked, so it can transfer + // the request directly. + CrossSiteResourceHandler::SetLeakRequestsForTesting(true); + + EXPECT_EQ(0, host_.pending_requests()); + + int render_view_id = 0; + int request_id = 1; + + // Configure initial request. + const std::string kResponseBody = "hello world"; + SetResponse("HTTP/1.1 200 OK\n" + "Content-Type: text/html\n\n", + kResponseBody); + + HandleScheme("http"); + + // Temporarily replace ContentBrowserClient with one that will trigger the + // transfer navigation code paths. + TransfersAllNavigationsContentBrowserClient new_client; + ContentBrowserClient* old_client = SetBrowserClientForTesting(&new_client); + + // Make the first request. + MakeTestRequestWithResourceType(filter_.get(), render_view_id, request_id, + GURL("http://example.com/blah"), + RESOURCE_TYPE_MAIN_FRAME); + + // Make a second request from the same process. + int second_request_id = 2; + MakeTestRequestWithResourceType(filter_.get(), render_view_id, + second_request_id, + GURL("http://example.com/foo"), + RESOURCE_TYPE_MAIN_FRAME); + + // Flush all the pending requests to get the response through the + // BufferedResourceHandler. + while (net::URLRequestTestJob::ProcessOnePendingMessage()) {} + + // Restore, now that we've set up a transfer. + SetBrowserClientForTesting(old_client); + + // This second filter is used to emulate a second process. + scoped_refptr<ForwardingFilter> second_filter = MakeForwardingFilter(); + + // Transfer the first request. + int new_render_view_id = 1; + int new_request_id = 5; + ResourceHostMsg_Request request = + CreateResourceRequest("GET", RESOURCE_TYPE_MAIN_FRAME, + GURL("http://example.com/blah")); + request.transferred_request_child_id = filter_->child_id(); + request.transferred_request_request_id = request_id; + + ResourceHostMsg_RequestResource transfer_request_msg( + new_render_view_id, new_request_id, request); + host_.OnMessageReceived(transfer_request_msg, second_filter.get()); + base::MessageLoop::current()->RunUntilIdle(); + + // Transfer the second request. + int new_second_request_id = 6; + ResourceHostMsg_Request second_request = + CreateResourceRequest("GET", RESOURCE_TYPE_MAIN_FRAME, + GURL("http://example.com/foo")); + request.transferred_request_child_id = filter_->child_id(); + request.transferred_request_request_id = second_request_id; + + ResourceHostMsg_RequestResource second_transfer_request_msg( + new_render_view_id, new_second_request_id, second_request); + host_.OnMessageReceived(second_transfer_request_msg, second_filter.get()); + base::MessageLoop::current()->RunUntilIdle(); + + // Check generated messages. + ResourceIPCAccumulator::ClassifiedMessages msgs; + accum_.GetClassifiedMessages(&msgs); + + ASSERT_EQ(2U, msgs.size()); + CheckSuccessfulRequest(msgs[0], kResponseBody); +} + // Test transferred navigations with text/plain, which causes // BufferedResourceHandler to buffer the response to sniff the content // before the transfer occurs. diff --git a/content/browser/loader/resource_request_info_impl.cc b/content/browser/loader/resource_request_info_impl.cc index 418f5b0..2b16896 100644 --- a/content/browser/loader/resource_request_info_impl.cc +++ b/content/browser/loader/resource_request_info_impl.cc @@ -127,6 +127,7 @@ ResourceRequestInfoImpl::ResourceRequestInfoImpl( has_user_gesture_(has_user_gesture), enable_load_timing_(enable_load_timing), was_ignored_by_handler_(false), + counted_as_in_flight_request_(false), resource_type_(resource_type), transition_type_(transition_type), memory_cost_(0), diff --git a/content/browser/loader/resource_request_info_impl.h b/content/browser/loader/resource_request_info_impl.h index 27f8cbc..866e38b 100644 --- a/content/browser/loader/resource_request_info_impl.h +++ b/content/browser/loader/resource_request_info_impl.h @@ -148,6 +148,16 @@ class ResourceRequestInfoImpl : public ResourceRequestInfo, was_ignored_by_handler_ = value; } + // Whether this request has been counted towards the number of in flight + // requests, which is only true for requests that require a file descriptor + // for their shared memory buffer. + bool counted_as_in_flight_request() const { + return counted_as_in_flight_request_; + } + void set_counted_as_in_flight_request(bool was_counted) { + counted_as_in_flight_request_ = was_counted; + } + // The approximate in-memory size (bytes) that we credited this request // as consuming in |outstanding_requests_memory_cost_map_|. int memory_cost() const { return memory_cost_; } @@ -180,6 +190,7 @@ class ResourceRequestInfoImpl : public ResourceRequestInfo, bool has_user_gesture_; bool enable_load_timing_; bool was_ignored_by_handler_; + bool counted_as_in_flight_request_; ResourceType resource_type_; ui::PageTransition transition_type_; int memory_cost_; |