Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2874353002:
PlzNavigate support in data use ascriber (Closed)
DescriptionPlzNavigate support in data use ascriber
With PlzNavigate, RenderFrameHost will not be populated in mainframe
requests. So global request ID alone should be used for mapping those
requests to main renderframe, when navigation commits later.
BUG=664233
Patch Set 1 #Patch Set 2 : rebased #Patch Set 3 : more fixes #
Total comments: 6
Patch Set 4 : rebased #Patch Set 5 : rebased #Patch Set 6 : minor fix #
Total comments: 2
Messages
Total messages: 17 (11 generated)
Description was changed from ========== PlzNavigate support in data use ascriber BUG= ========== to ========== PlzNavigate support in data use ascriber With PlzNavigate, RenderFrameHost will not be populated in mainframe requests. So global request ID alone should be used for mapping those requests to main renderframe, when navigation commits later. BUG=664233 ==========
rajendrant@google.com changed reviewers: + ryansturm@chromium.org
rajendrant@google.com changed reviewers: + rajendrant@google.com
ptal
https://codereview.chromium.org/2874353002/diff/40001/chrome/browser/data_use... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right): https://codereview.chromium.org/2874353002/diff/40001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:102: if (request_info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) { 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. https://codereview.chromium.org/2874353002/diff/40001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:202: main_render_frame_data_use_map_.find(entry->main_frame_id()); If the request is destroyed after the next page load commits in the same frame, will this get deleted correctly?
https://codereview.chromium.org/2874353002/diff/40001/chrome/browser/data_use... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right): https://codereview.chromium.org/2874353002/diff/40001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:102: if (request_info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) { On 2017/05/22 17:22:05, RyanSturm wrote: > 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. 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. https://codereview.chromium.org/2874353002/diff/40001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:202: main_render_frame_data_use_map_.find(entry->main_frame_id()); On 2017/05/22 17:22:05, RyanSturm wrote: > If the request is destroyed after the next page load commits in the same frame, > will this get deleted correctly? 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... https://codereview.chromium.org/2874353002/diff/100001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right): https://codereview.chromium.org/2874353002/diff/100001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:397: main_render_frame_data_use_map_.erase(frame_it); Moving it down, since |old_frame_entry| is accessed after erase.
https://codereview.chromium.org/2874353002/diff/40001/chrome/browser/data_use... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right): https://codereview.chromium.org/2874353002/diff/40001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:102: if (request_info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) { On 2017/05/24 07:14:09, rajendrant wrote: > On 2017/05/22 17:22:05, RyanSturm wrote: > > 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. > > 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. sgtm. https://codereview.chromium.org/2874353002/diff/40001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:202: main_render_frame_data_use_map_.find(entry->main_frame_id()); On 2017/05/24 07:14:09, rajendrant wrote: > On 2017/05/22 17:22:05, RyanSturm wrote: > > If the request is destroyed after the next page load commits in the same > frame, > > will this get deleted correctly? > > 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... 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) https://codereview.chromium.org/2874353002/diff/100001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right): https://codereview.chromium.org/2874353002/diff/100001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:397: main_render_frame_data_use_map_.erase(frame_it); On 2017/05/24 07:14:09, rajendrant wrote: > Moving it down, since |old_frame_entry| is accessed after erase. Acknowledged.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/05/24 16:08:44, RyanSturm wrote: > https://codereview.chromium.org/2874353002/diff/40001/chrome/browser/data_use... > File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right): > > https://codereview.chromium.org/2874353002/diff/40001/chrome/browser/data_use... > chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:102: if > (request_info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) { > On 2017/05/24 07:14:09, rajendrant wrote: > > On 2017/05/22 17:22:05, RyanSturm wrote: > > > 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. > > > > 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. > > sgtm. > > https://codereview.chromium.org/2874353002/diff/40001/chrome/browser/data_use... > chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:202: > main_render_frame_data_use_map_.find(entry->main_frame_id()); > On 2017/05/24 07:14:09, rajendrant wrote: > > On 2017/05/22 17:22:05, RyanSturm wrote: > > > If the request is destroyed after the next page load commits in the same > > frame, > > > will this get deleted correctly? > > > > 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... > > 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) > > https://codereview.chromium.org/2874353002/diff/100001/chrome/browser/data_us... > File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right): > > https://codereview.chromium.org/2874353002/diff/100001/chrome/browser/data_us... > chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:397: > main_render_frame_data_use_map_.erase(frame_it); > On 2017/05/24 07:14:09, rajendrant wrote: > > Moving it down, since |old_frame_entry| is accessed after erase. > > Acknowledged. I am trying to land this in https://codereview.chromium.org/2874353002/. undo lgtm to prevent conflicts.
On 2017/05/31 16:04:58, tbansal1 wrote: > On 2017/05/24 16:08:44, RyanSturm wrote: > > > https://codereview.chromium.org/2874353002/diff/40001/chrome/browser/data_use... > > File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right): > > > > > https://codereview.chromium.org/2874353002/diff/40001/chrome/browser/data_use... > > chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:102: if > > (request_info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) { > > On 2017/05/24 07:14:09, rajendrant wrote: > > > On 2017/05/22 17:22:05, RyanSturm wrote: > > > > 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. > > > > > > 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. > > > > sgtm. > > > > > https://codereview.chromium.org/2874353002/diff/40001/chrome/browser/data_use... > > chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:202: > > main_render_frame_data_use_map_.find(entry->main_frame_id()); > > On 2017/05/24 07:14:09, rajendrant wrote: > > > On 2017/05/22 17:22:05, RyanSturm wrote: > > > > If the request is destroyed after the next page load commits in the same > > > frame, > > > > will this get deleted correctly? > > > > > > 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... > > > > 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) > > > > > https://codereview.chromium.org/2874353002/diff/100001/chrome/browser/data_us... > > File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right): > > > > > https://codereview.chromium.org/2874353002/diff/100001/chrome/browser/data_us... > > chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:397: > > main_render_frame_data_use_map_.erase(frame_it); > > On 2017/05/24 07:14:09, rajendrant wrote: > > > Moving it down, since |old_frame_entry| is accessed after erase. > > > > Acknowledged. > > I am trying to land this in https://codereview.chromium.org/2874353002/. > undo lgtm to prevent conflicts. Aah, I meant: not lgtm to prevent conflicts with the other CL, and to make it clear that this should not be worked on. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
