PlzNavigate support in data use ascriber

chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/data_use_measurement/chrome_data_use_ascriber.h"
 
 #include <string>
 
 #include "base/memory/ptr_util.h"
 #include "chrome/browser/data_use_measurement/chrome_data_use_recorder.h"
@@ skipping 39 matching lines @@
 ChromeDataUseRecorder* ChromeDataUseAscriber::GetOrCreateDataUseRecorder(
     net::URLRequest* request) {
   DataUseRecorderEntry entry = GetOrCreateDataUseRecorderEntry(request);
   return entry == data_use_recorders_.end() ? nullptr : &(*entry);
 }
 
 ChromeDataUseRecorder* ChromeDataUseAscriber::GetDataUseRecorder(
     const net::URLRequest& request) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  // TODO(ryansturm): Handle PlzNavigate (http://crbug/664233).
-  if (content::IsBrowserSideNavigationEnabled())
-    return nullptr;
-
   // If a DataUseRecorder has already been set as user data, then return that.
   auto* user_data = static_cast<DataUseRecorderEntryAsUserData*>(
       request.GetUserData(DataUseRecorderEntryAsUserData::kUserDataKey));
   return user_data ? &(*user_data->recorder_entry()) : nullptr;
 }
 
 ChromeDataUseAscriber::DataUseRecorderEntry
 ChromeDataUseAscriber::GetOrCreateDataUseRecorderEntry(
     net::URLRequest* request) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  // TODO(ryansturm): Handle PlzNavigate (http://crbug/664233).
-  if (content::IsBrowserSideNavigationEnabled())
-    return data_use_recorders_.end();
-
   // If a DataUseRecorder has already been set as user data, then return that.
   auto* user_data = static_cast<DataUseRecorderEntryAsUserData*>(
       request->GetUserData(DataUseRecorderEntryAsUserData::kUserDataKey));
   if (user_data)
     return user_data->recorder_entry();
 
   // If request is associated with a ChromeService, create a new
   // DataUseRecorder for it. There is no reason to aggregate URLRequests
   // from ChromeServices into the same DataUseRecorder instance.
   DataUseUserData* service = static_cast<DataUseUserData*>(
       request->GetUserData(DataUseUserData::kUserDataKey));
   if (service) {
     DataUseRecorderEntry entry =
         CreateNewDataUseRecorder(request, DataUse::TrafficType::SERVICES);
     entry->data_use().set_description(
         DataUseUserData::GetServiceNameAsString(service->service_name()));
     return entry;
   }
 
+  const content::ResourceRequestInfo* request_info =
+      content::ResourceRequestInfo::ForRequest(request);
+  if (!request_info ||
+      request_info->GetGlobalRequestID() == content::GlobalRequestID()) {
+    // Create a new DataUseRecorder for all other requests.
+    DataUseRecorderEntry entry =
+        CreateNewDataUseRecorder(request, DataUse::TrafficType::UNKNOWN);
+    DataUse& data_use = entry->data_use();
+    data_use.set_url(request->url());
+    return entry;
+  }
+
+  if (request_info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {

[RyanSturm, 2017/05/22 17:22:05]

Should this clause be before the other clause you are adding (more specifically
before the request_info->GetGlobalRequestID() == content::GlobalRequestID()
check)?

AFAIK, request_info->GetGlobalRequestID() == content::GlobalRequestID() could
happen in plzNavigate (not sure), in which case 
pending_navigation_data_use_map_ won't have an entry for it.

[rajendrant, 2017/05/24 07:14:09]

I guess, your question is about the following bug ?
https://bugs.chromium.org/p/chromium/issues/detail?id=680841

I think, only in some tests it may not be populated. Maybe this check can be
removed. I am being conservative here.

[RyanSturm, 2017/05/24 16:08:44]

sgtm.

+    DataUseRecorderEntry new_entry =
+        CreateNewDataUseRecorder(request, DataUse::TrafficType::USER_TRAFFIC);
+    new_entry->set_main_frame_request_id(request_info->GetGlobalRequestID());
+    pending_navigation_data_use_map_.insert(
+        std::make_pair(request_info->GetGlobalRequestID(), new_entry));
+    return new_entry;
+  }
+
   int render_process_id = -1;
   int render_frame_id = -1;
   bool has_valid_frame = content::ResourceRequestInfo::GetRenderFrameForRequest(
       request, &render_process_id, &render_frame_id);
   if (has_valid_frame &&
       render_frame_id != SpecialRoutingIDs::MSG_ROUTING_NONE) {
     DCHECK(render_process_id >= 0 || render_frame_id >= 0);
 
     // Browser tests may not set up DataUseWebContentsObservers in which case
     // this class never sees navigation and frame events so DataUseRecorders
     // will never be destroyed. To avoid this, we ignore requests whose
     // render frames don't have a record. However, this can also be caused by
     // URLRequests racing the frame create events.
     // TODO(kundaji): Add UMA.
     RenderFrameHostID frame_key(render_process_id, render_frame_id);
     auto main_frame_key_iter = subframe_to_mainframe_map_.find(frame_key);
     if (main_frame_key_iter == subframe_to_mainframe_map_.end()) {
       return data_use_recorders_.end();
     }
     auto frame_iter =
         main_render_frame_data_use_map_.find(main_frame_key_iter->second);
     if (frame_iter == main_render_frame_data_use_map_.end()) {
       return data_use_recorders_.end();
     }
 
-    const content::ResourceRequestInfo* request_info =
-        content::ResourceRequestInfo::ForRequest(request);
-    content::ResourceType resource_type =
-        request_info ? request_info->GetResourceType()
-                     : content::RESOURCE_TYPE_LAST_TYPE;
-
-    if (resource_type == content::RESOURCE_TYPE_MAIN_FRAME) {
-      content::GlobalRequestID navigation_key =
-          request_info->GetGlobalRequestID();
-
-      DataUseRecorderEntry new_entry =
-          CreateNewDataUseRecorder(request, DataUse::TrafficType::USER_TRAFFIC);
-      new_entry->set_main_frame_request_id(navigation_key);
-      pending_navigation_data_use_map_.insert(
-          std::make_pair(navigation_key, new_entry));
-
-      return new_entry;
-    }
-
-    DCHECK(frame_iter != main_render_frame_data_use_map_.end());
     auto entry = frame_iter->second;
     request->SetUserData(
         DataUseRecorderEntryAsUserData::kUserDataKey,
         base::MakeUnique<DataUseRecorderEntryAsUserData>(entry));
     entry->AddPendingURLRequest(request);
     return entry;
   }
 
   // Create a new DataUseRecorder for all other requests.
   DataUseRecorderEntry entry = CreateNewDataUseRecorder(
@@ skipping 34 matching lines @@
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
   // TODO(rajendrant): GetDataUseRecorder is sufficient and
   // GetOrCreateDataUseRecorderEntry is not needed. The entry gets created in
   // DataUseAscriber::OnBeforeUrlRequest().
   DataUseRecorderEntry entry = GetOrCreateDataUseRecorderEntry(request);
 
   if (entry == data_use_recorders_.end())
     return;
 
-  RenderFrameHostID frame_key = entry->main_frame_id();
-  auto frame_iter = main_render_frame_data_use_map_.find(frame_key);
-  bool is_in_render_frame_map =
-      frame_iter != main_render_frame_data_use_map_.end() &&
-      frame_iter->second->HasPendingURLRequest(request);
-
+  bool will_datause_complete = false;
   const content::ResourceRequestInfo* request_info =
       content::ResourceRequestInfo::ForRequest(request);
-  bool is_in_pending_navigation_map =
-      request_info &&
-      request_info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME &&
-      pending_navigation_data_use_map_.find(entry->main_frame_request_id()) !=
-          pending_navigation_data_use_map_.end();
+
+  if (request_info &&
+      request_info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
+    will_datause_complete =
+        pending_navigation_data_use_map_.find(entry->main_frame_request_id()) ==
+        pending_navigation_data_use_map_.end();
+  } else {
+    // Non-mainframe, Services, and other requests.
+    auto frame_iter =
+        main_render_frame_data_use_map_.find(entry->main_frame_id());

[RyanSturm, 2017/05/22 17:22:05]

If the request is destroyed after the next page load commits in the same frame,
will this get deleted correctly? 

[rajendrant, 2017/05/24 07:14:09]

Yes. That happens in mainframe delete or next pageload commit, whichever is
sooner.

https://cs.chromium.org/chromium/src/chrome/browser/data_use_measurement/chro...
https://cs.chromium.org/chromium/src/chrome/browser/data_use_measurement/chro...

[RyanSturm, 2017/05/24 16:08:44]

I was thinking more along the lines of:

1) request A created for frame (1,1)
2) navigation in frame (1,1) to new page load
3) request A destroyed
4) main_render_frame_data_use_map_.find(entry->main_frame_id()) returns the next
page load because (1,1) is still around
5) frame_iter->second->HasPendingURLRequest(request) returns false since that
page load is active

wouldn't it better to also verify that (entry == frame_iter->second) and that
entry->HasPendingURLRequest, so that you check:
1) the frame is gone (via frame_iter == main_render_frame_data_use_map_.end())
or the page load is gone in the frame (via entry == frame_iter->second)
2) data use is complete for entry (via entry->HasPendingURLRequest as opposed to
frame_iter->second)

+    will_datause_complete =
+        frame_iter == main_render_frame_data_use_map_.end() ||
+        !frame_iter->second->HasPendingURLRequest(request);
+  }
 
   DataUseAscriber::OnUrlRequestDestroyed(request);
   request->RemoveUserData(DataUseRecorderEntryAsUserData::kUserDataKey);
 
-  if (entry->IsDataUseComplete() && !is_in_render_frame_map &&
-      !is_in_pending_navigation_map) {
+  if (entry->IsDataUseComplete() && will_datause_complete) {
     OnDataUseCompleted(entry);
     data_use_recorders_.erase(entry);
   }
 }
 
 void ChromeDataUseAscriber::RenderFrameCreated(int render_process_id,
                                                int render_frame_id,
                                                int main_render_process_id,
                                                int main_render_frame_id) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  if (content::IsBrowserSideNavigationEnabled())
-    return;
-
   if (main_render_process_id != -1 && main_render_frame_id != -1) {
     // Create an entry in |subframe_to_mainframe_map_| for this frame mapped to
     // it's parent frame.
     subframe_to_mainframe_map_.insert(std::make_pair(
         RenderFrameHostID(render_process_id, render_frame_id),
         RenderFrameHostID(main_render_process_id, main_render_frame_id)));
   } else {
     subframe_to_mainframe_map_.insert(
         std::make_pair(RenderFrameHostID(render_process_id, render_frame_id),
                        RenderFrameHostID(render_process_id, render_frame_id)));
     auto frame_iter = main_render_frame_data_use_map_.find(
         RenderFrameHostID(render_process_id, render_frame_id));
     DCHECK(frame_iter == main_render_frame_data_use_map_.end());
     DataUseRecorderEntry entry =
         CreateNewDataUseRecorder(nullptr, DataUse::TrafficType::UNKNOWN);
     entry->set_main_frame_id(
         RenderFrameHostID(render_process_id, render_frame_id));
     main_render_frame_data_use_map_.insert(std::make_pair(
         RenderFrameHostID(render_process_id, render_frame_id), entry));
   }
 }
 
 void ChromeDataUseAscriber::RenderFrameDeleted(int render_process_id,
                                                int render_frame_id,
                                                int main_render_process_id,
                                                int main_render_frame_id) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  if (content::IsBrowserSideNavigationEnabled())
-    return;
-
   RenderFrameHostID key(render_process_id, render_frame_id);
 
   if (main_render_process_id == -1 && main_render_frame_id == -1) {
     auto frame_iter = main_render_frame_data_use_map_.find(key);
     DataUseRecorderEntry entry = frame_iter->second;
     if (entry->IsDataUseComplete()) {
       OnDataUseCompleted(entry);
       data_use_recorders_.erase(entry);
     }
     main_render_frame_data_use_map_.erase(frame_iter);
@@ skipping 168 matching lines @@
                                                    int new_render_process_id,
                                                    int new_render_frame_id) {
   if (visible_main_render_frames_.find(
           RenderFrameHostID(old_render_process_id, old_render_frame_id)) !=
       visible_main_render_frames_.end()) {
     WasShownOrHidden(new_render_process_id, new_render_frame_id, true);
   }
 }
 
 }  // namespace data_use_measurement