[DIPS] Add a DevTools issue for sites in redirect chains [chromium/src : main]
Joshua Hood (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ben Kelly.
Joshua Hood would like Andrey Kosyakov and Ben Kelly to review this change.
[DIPS] Add a DevTools issue for sites in redirect chains
This CL creates DevTools issues for sites in redirect chains (without
previous user interaction) that may have their state cleared by DIPS. A
separate CL will be needed to add these issues to the DevTools issue
panel in the frontend.
Bug: 1416548
Change-Id: I3d061743dbc39e8e87f6c9d2d3bd8118d935a2ee
---
M chrome/browser/dips/dips_bounce_detector.cc
M chrome/browser/dips/dips_bounce_detector_browsertest.cc
M content/browser/devtools/devtools_instrumentation.cc
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/public/mojom/devtools/inspector_issue.mojom
M third_party/blink/renderer/core/inspector/inspector_issue_conversion.cc
6 files changed, 151 insertions(+), 1 deletion(-)
To view, visit change 4404102. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Hood (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ben Kelly.
Patch set 3:Commit-Queue +1
Joshua Hood (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ben Kelly, Chromium IPC Reviews.
Joshua Hood would like Chromium IPC Reviews to review this change.
[DIPS] Add a DevTools issue for sites in redirect chains
This CL creates DevTools issues for sites in redirect chains (without
previous user interaction) that may have their state cleared by DIPS. A
separate CL will be needed to add these issues to the DevTools issue
panel in the frontend.
Bug: 1416548
Change-Id: I3d061743dbc39e8e87f6c9d2d3bd8118d935a2ee
---
M chrome/browser/dips/dips_bounce_detector.cc
M chrome/browser/dips/dips_bounce_detector_browsertest.cc
M content/browser/devtools/devtools_instrumentation.cc
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/public/mojom/devtools/inspector_issue.mojom
M third_party/blink/renderer/core/inspector/inspector_issue_conversion.cc
6 files changed, 151 insertions(+), 1 deletion(-)
To view, visit change 4404102. To unsubscribe, or for help writing mail filters, visit settings.
gwsq (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ben Kelly, Chromium IPC Reviews, Ken Buchanan.
gwsq would like Ken Buchanan to review this change authored by Joshua Hood.
gwsq (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ben Kelly, Ken Buchanan.
Joshua Hood has uploaded this change for review.
gwsq (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ben Kelly, Ken Buchanan.
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ke...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): ke...@chromium.org
Reviewer source(s):
ke...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Ben Kelly (Gerrit)
Attention is currently required from: Andrey Kosyakov, Joshua Hood, Ken Buchanan.
Patch set 3:Code-Review +1
4 comments:
Patchset:
dips LGTM with a couple minor comments.
File chrome/browser/dips/dips_bounce_detector.cc:
Patch Set #3, Line 221:
if (sites.size() == 0) {nit: sites.empty() is preferred in chromium I think. (Although I see other code in this file uses size() == 0.)
Patch Set #3, Line 229:
bounce_tracking_issue_details->tracking_sites.push_back(site);Before the loop reserve space to avoid reallocations.
`bounce_tracking_issue_details->tracking_sites.reserve(sites.size());`
File content/browser/devtools/devtools_instrumentation.cc:
Patch Set #3, Line 339:
tracking_sites_ptr->push_back(site);Again, you can call reserve() before the loop to avoid reallocations.
To view, visit change 4404102. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Hood (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ken Buchanan.
Patch set 4:Commit-Queue +1
3 comments:
File chrome/browser/dips/dips_bounce_detector.cc:
Patch Set #3, Line 221:
if (sites.size() == 0) {
nit: sites.empty() is preferred in chromium I think. […]
Done
Patch Set #3, Line 229:
bounce_tracking_issue_details->tracking_sites.push_back(site);
Before the loop reserve space to avoid reallocations. […]
Done
File content/browser/devtools/devtools_instrumentation.cc:
Patch Set #3, Line 339:
tracking_sites_ptr->push_back(site);Again, you can call reserve() before the loop to avoid reallocations.
Done
To view, visit change 4404102. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Hood (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ken Buchanan.
Joshua Hood removed jdh+wat...@chromium.org and wanderview...@chromium.org from this change.
[DIPS] Add a DevTools issue for sites in redirect chains
This CL creates DevTools issues for sites in redirect chains (without
previous user interaction) that may have their state cleared by DIPS. A
separate CL will be needed to add these issues to the DevTools issue
panel in the frontend.
Bug: 1416548
Change-Id: I3d061743dbc39e8e87f6c9d2d3bd8118d935a2ee
---
M chrome/browser/dips/dips_bounce_detector.cc
M chrome/browser/dips/dips_bounce_detector_browsertest.cc
M content/browser/devtools/devtools_instrumentation.cc
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/public/mojom/devtools/inspector_issue.mojom
M third_party/blink/renderer/core/inspector/inspector_issue_conversion.cc
6 files changed, 159 insertions(+), 1 deletion(-)
Andrey Kosyakov (Gerrit)
Attention is currently required from: Joshua Hood, Ken Buchanan.
Patch set 4:Code-Review +1
4 comments:
Patchset:
devtools lgtm with comments
File content/browser/devtools/devtools_instrumentation.cc:
Patch Set #4, Line 338:
tracking_sites_ptr->reserve(issue_details->tracking_sites.size());nit: both sides are std::vector<std::string> under the hood, so just ` *tracking_sites_ptr = issue_details->tracking_sites;` should do the trick.
Or even `SetTrackingSites(std::make_unique<protocol::Array<protocol::String>>(issue_details->tracking_sites))` below.
File third_party/blink/public/devtools_protocol/browser_protocol.pdl:
Patch Set #4, Line 786:
array of string trackingSites_site_ is a bit ambiguous, and these are origins from what I can see in the test, so perhaps just make it `trackingOrigins`? Or `tracingSiteOrigins` if you want _site_ there too.
File third_party/blink/renderer/core/inspector/inspector_issue_conversion.cc:
Patch Set #4, Line 69:
// empty value or should it return aJust make it `NOTREACHED_NORETURN()`?
To view, visit change 4404102. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Hood (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ken Buchanan.
Patch set 5:Commit-Queue +1
3 comments:
File content/browser/devtools/devtools_instrumentation.cc:
Patch Set #4, Line 338:
tracking_sites_ptr->reserve(issue_details->tracking_sites.size());
nit: both sides are std::vector<std::string> under the hood, so just ` *tracking_sites_ptr = issue_ […]
Ahh cool, thanks for this! Done.
File third_party/blink/public/devtools_protocol/browser_protocol.pdl:
Patch Set #4, Line 786:
array of string trackingSites
_site_ is a bit ambiguous, and these are origins from what I can see in the test, so perhaps just ma […]
'Site' is the term we use throughout the DIPS code and explainers to refer to eTLD+1. 'Origin' would technically imply these strings represent something more like a schemeful FQDN. In practice, being bounced through `https://a.test:80/bouncer` would result in only the string `a.test` being in `trackingSites`.
File third_party/blink/renderer/core/inspector/inspector_issue_conversion.cc:
Patch Set #4, Line 69:
// empty value or should it return aJust make it `NOTREACHED_NORETURN()`?
This note was more because I'm unsure about how `InspectorIssueCodeValue()` is used and if this should be something like `NOTREACHED_NORETURN()` or `return protocol::Audits::InspectorIssueCodeEnum::BounceTrackingIssue`.
Could you weigh in on that?
To view, visit change 4404102. To unsubscribe, or for help writing mail filters, visit settings.
Andrey Kosyakov (Gerrit)
Attention is currently required from: Joshua Hood, Ken Buchanan.
Patch set 5:Code-Review +1
2 comments:
File third_party/blink/public/devtools_protocol/browser_protocol.pdl:
Patch Set #4, Line 786:
array of string trackingSites
'Site' is the term we use throughout the DIPS code and explainers to refer to eTLD+1. […]
Hmm, ok, let's document the meaning of site then (perhaps along with an example), because the term isn't commonly used elsewhere in CDP.
File third_party/blink/renderer/core/inspector/inspector_issue_conversion.cc:
Patch Set #4, Line 69:
// empty value or should it return a
This note was more because I'm unsure about how `InspectorIssueCodeValue()` is used and if this shou […]
This is only relevant for issues that may be produced on the blink side, which is not the case with the one you're adding, so there's no point in trying to provide any graceful handling of the situation, hence NOTREACHED_NORETURN() or any other way to strongly assert this is not happening is appropriate.
To view, visit change 4404102. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Hood (Gerrit)
Attention is currently required from: Ken Buchanan.
Patch set 6:Commit-Queue +1
2 comments:
File third_party/blink/public/devtools_protocol/browser_protocol.pdl:
Patch Set #4, Line 786:
array of string trackingSites
Hmm, ok, let's document the meaning of site then (perhaps along with an example), because the term i […]
Done
File third_party/blink/renderer/core/inspector/inspector_issue_conversion.cc:
Patch Set #4, Line 69:
// empty value or should it return a
This is only relevant for issues that may be produced on the blink side, which is not the case with […]
Ahh okay, thanks for clearing that up for me!
Changed to `NOTREACHED_NORETURN()`.
To view, visit change 4404102. To unsubscribe, or for help writing mail filters, visit settings.
Ken Buchanan (Gerrit)
Attention is currently required from: Joshua Hood.
Patch set 6:Code-Review +1
Findit (Gerrit)
Attention is currently required from: Joshua Hood.
This change meets the code coverage requirements.
Patch set 6:Code-Coverage +1
Joshua Hood (Gerrit)
Attention is currently required from: Joshua Hood.
Patch set 6:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[DIPS] Add a DevTools issue for sites in redirect chains
This CL creates DevTools issues for sites in redirect chains (without
previous user interaction) that may have their state cleared by DIPS. A
separate CL will be needed to add these issues to the DevTools issue
panel in the frontend.
Bug: 1416548
Change-Id: I3d061743dbc39e8e87f6c9d2d3bd8118d935a2ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4404102
Reviewed-by: Ken Buchanan <ke...@chromium.org>
Code-Coverage: Findit <findit...@appspot.gserviceaccount.com>
Commit-Queue: Joshua Hood <j...@chromium.org>
Reviewed-by: Ben Kelly <wande...@chromium.org>
Reviewed-by: Andrey Kosyakov <ca...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1130045}
---
M chrome/browser/dips/dips_bounce_detector.cc
M chrome/browser/dips/dips_bounce_detector_browsertest.cc
M content/browser/devtools/devtools_instrumentation.cc
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/public/mojom/devtools/inspector_issue.mojom
M third_party/blink/renderer/core/inspector/inspector_issue_conversion.cc
6 files changed, 150 insertions(+), 1 deletion(-)