[Autofill] Add prerendering check to ContentAutofillDriver::GetForRFH [chromium/src : main]
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
- Christoph Schwering
- Dave Tapuska
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Christoph Schwering
- Dave Tapuska
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dave Tapuska
| Code-Review | +1 |
1 comment
CHECK(!render_frame_host->IsInLifecycleState(Let's add a comment why the ContentAutofillDriver is constructed but shouldn't be queried in this case.
Related details
- Christoph Schwering
- Ian Vollick
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
gwsq error while processing this CL: Error looking up group chromeos-commercial-r...@google.com: Streaming RPC error: status 6 error: /Expn.ExpandAddress to [2002:ac8:9a82:0:b0:4ec:7587:cac9]:4502 : APP_ERROR(13) goa-auth: error exchanging TransactDAT for GaiaMint: generic::internal: could not exchange TransactDAT for GaiaMint. Code: 20, error: code:1, debug: This DAT is all-users DAT and there is no asserted user goa-auth: error exchanging TransactDAT for GaiaMint: generic::internal: could not exchange TransactDAT for GaiaMint. Code: 20, error: code:1, debug: This DAT is all-users DAT and there is no asserted user
Christoph Schwering (Gerrit)
Christoph Schwering voted and added 2 comments![]( "Open in Gerrit")
Christoph Schwering added votes with a message
Thank you!
| Code-Review | +1 |
2 comments
friend class ContentAutofillDriver;nit: could you use a `base::PassKey<ContentAutofillDriver>` instead to limit `DriverForFrame()`?
Related details
- Ian Vollick
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 2 comments![]( "Open in Gerrit")
nit: could you use a `base::PassKey<ContentAutofillDriver>` instead to limit `DriverForFrame()`?
Good idea. Done. In the process I changed the other call to DriverForFrame within ContentAutofillDriver to use GetForRenderFrameHost (getting the parent should be ok, I think, since we shouldn't be starting from a prerendered frame and it seems like we should CHECK that this is the case using GFRFH just like we're doing everywhere else0.
Related details
- Christoph Schwering
- Dave Tapuska
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
Let's add a comment why the ContentAutofillDriver is constructed but shouldn't be queried in this case.
- Andrey Kosyakov
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasilii Sukhanov (Gerrit)
Vasilii Sukhanov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vasilii Sukhanov
| Code-Review | +1 |
1 comment
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc LGTM
Related details
- Andrey Kosyakov
- Christoph Schwering
- Dave Tapuska
- Ian Vollick
- Jan Keitel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
gwsq error while processing this CL: Error looking up group chromeos-commercial-r...@google.com: Streaming RPC error: status 6 error: /Expn.ExpandAddress to [2002:a05:690c:6::]:4680 : APP_ERROR(13) goa-auth: error exchanging TransactDAT for GaiaMint: generic::internal: could not exchange TransactDAT for GaiaMint. Code: 20, error: code:1, debug: This DAT is all-users DAT and there is no asserted user goa-auth: error exchanging TransactDAT for GaiaMint: generic::internal: could not exchange TransactDAT for GaiaMint. Code: 20, error: code:1, debug: This DAT is all-users DAT and there is no asserted user
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
gwsq error while processing this CL: Error looking up group chromeos-commercial-r...@google.com: Streaming RPC error: status 6 error: /Expn.ExpandAddress to [2002:ac8:9a82:0:b0:4ec:7587:cac9]:4502 : APP_ERROR(13) goa-auth: error exchanging TransactDAT for GaiaMint: generic::internal: could not exchange TransactDAT for GaiaMint. Code: 20, error: code:1, debug: This DAT is all-users DAT and there is no asserted user goa-auth: error exchanging TransactDAT for GaiaMint: generic::internal: could not exchange TransactDAT for GaiaMint. Code: 20, error: code:1, debug: This DAT is all-users DAT and there is no asserted user
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
The test failure is real and reveals a flow in this approach. The ContentAutofillDriverFactory advertises driver creation, so observers can access the driver without checking for prerendering. I will investigate whether or not we can defer sending creation notifications until after activation.
Related details
- Andrey Kosyakov
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
gwsq error while processing this CL: Error looking up group chromeos-commercial-r...@google.com: Streaming RPC error: status 6 error: /Expn.ExpandAddress to [2002:a05:690c:6::]:4680 : APP_ERROR(13) goa-auth: error exchanging TransactDAT for GaiaMint: generic::internal: could not exchange TransactDAT for GaiaMint. Code: 20, error: code:1, debug: This DAT is all-users DAT and there is no asserted user goa-auth: error exchanging TransactDAT for GaiaMint: generic::internal: could not exchange TransactDAT for GaiaMint. Code: 20, error: code:1, debug: This DAT is all-users DAT and there is no asserted user
Related details
gwsq (Gerrit)
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
(removing other reviewers while we iterate).
While this current patchset seems to work, it's admittedly quite ugly.
The issue seems to be that _most_ observers of OnContentAutofillDriverCreated / WillBeDeleted can cope with deferral (until activation), but some cannot and these seem to be test related (eg for injecting early enough that we can create mocks).
An alternative to creating these extra callbacks and related complexity is to just not worry about access to the driver during these create / will-be-deleted callbacks (usually used to set up observation).
Yet another approach would be to have a test-focused factory that's used whenever we have a factory, but I haven't got a clear idea of how this would work and would, I imagine, add complexity to the client initializion.
Wdyt, is this approach reasonable or should I punt on the create/delete issue or try to create some sort of for-test-factory that lets you observer undeferred events?
Related details
- Christoph Schwering
- Dave Tapuska
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 6 comments![]( "Open in Gerrit")
return GetForRenderFrameHost(parent_rfh);Do we need that? I thought a frame is prerendering iff its frame tree is prerendering -- or did I misunderstand a comment on my previous CL that removed the DCHECK?
auto iter = driver_map_.find(navigation_handle->GetRenderFrameHost());
CHECK(iter != driver_map_.end());
// We use the reference here rather than |driver| since the creation
// callbacks can change the driver and the new driver must be used when
// notifying subsequent observers and for the remainder of this function.
std::unique_ptr<ContentAutofillDriver>& driver_ref = iter->second;nit: move inside `if` below?
if (navigation_handle->IsPrerenderedPageActivation()) {Is a navigation the *only* way to activate a prerendered frame?
// We will not send creation events while prerendering, so send them now."did"?
std::vector<ContentAutofillDriver*>
ContentAutofillDriverFactory::GetExistingDrivers(
base::PassKey<ScopedAutofillManagersObservation>) {
std::vector<ContentAutofillDriver*> drivers;
drivers.reserve(driver_map_.size());
for (const std::pair<content::RenderFrameHost*,
std::unique_ptr<ContentAutofillDriver>>& entry :
driver_map_) {
drivers.push_back(entry.second.get());
}
return drivers;
}This may leak prerendering drivers. It should suffice to only add activated drivers to `drivers`.
Related details
- Dave Tapuska
- Ian Vollick
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
std::vector<ContentAutofillDriver*>
ContentAutofillDriverFactory::GetExistingDrivers(
base::PassKey<ScopedAutofillManagersObservation>) {
std::vector<ContentAutofillDriver*> drivers;
drivers.reserve(driver_map_.size());
for (const std::pair<content::RenderFrameHost*,
std::unique_ptr<ContentAutofillDriver>>& entry :
driver_map_) {
drivers.push_back(entry.second.get());
}
return drivers;
}This may leak prerendering drivers. It should suffice to only add activated drivers to `drivers`.
This might also be why the bots crash (only had a very brief look but the `ScopedWhateverObservation`, which calls `GetExistingDrivers()` seems to play a role).
Ian Vollick (Gerrit)
Ian Vollick added 6 comments![]( "Open in Gerrit")
Thanks for the review! Feel free to comment further, but I'm going to move this back over to WIP until the bots are green.
return GetForRenderFrameHost(parent_rfh);Do we need that? I thought a frame is prerendering iff its frame tree is prerendering -- or did I misunderstand a comment on my previous CL that removed the DCHECK?
IIUC, we don't render subframes for prerendering, actually (again, Hiroki can hopefully let me know if that's wrong).
As for this change, this is what caught the fact that we were announcing driver creation even for prerendered frames. This particular observer grabbed the parent https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/android/touch_to_fill_keyboard_suppressor.cc;l=50;drc=c0265133106c7647e90f9aaa4377d28190b1a6a9
So I thought it would be good to keep this check since, really, no one should be asking for the parent if we're prerendering and this will enforce that this is the case.
auto iter = driver_map_.find(navigation_handle->GetRenderFrameHost());
CHECK(iter != driver_map_.end());
// We use the reference here rather than |driver| since the creation
// callbacks can change the driver and the new driver must be used when
// notifying subsequent observers and for the remainder of this function.
std::unique_ptr<ContentAutofillDriver>& driver_ref = iter->second;nit: move inside `if` below?
I think I need it here so I can use |driver_ref| at the very end of the fn when I call Reset as well (otherwise I wouldn't be calling Reset on the mocked/injected driver).
if (navigation_handle->IsPrerenderedPageActivation()) {Is a navigation the *only* way to activate a prerendered frame?
AFAIK, yes. Hiroki, plmk if that's wrong.
// We will not send creation events while prerendering, so send them now.Ian Vollick"did"?
Done
std::vector<ContentAutofillDriver*>
ContentAutofillDriverFactory::GetExistingDrivers(
base::PassKey<ScopedAutofillManagersObservation>) {
std::vector<ContentAutofillDriver*> drivers;
drivers.reserve(driver_map_.size());
for (const std::pair<content::RenderFrameHost*,
std::unique_ptr<ContentAutofillDriver>>& entry :
driver_map_) {
drivers.push_back(entry.second.get());
}
return drivers;
}Christoph SchweringThis may leak prerendering drivers. It should suffice to only add activated drivers to `drivers`.
This might also be why the bots crash (only had a very brief look but the `ScopedWhateverObservation`, which calls `GetExistingDrivers()` seems to play a role).
Sorry, the change wasn't quite ready for review -- I was trying out a patchset where I did without the flat_set of deferred RFHs (I had two tests that were succeeding without it and I wanted to see if it could be removed). I've put this back and checked locally that all the linux-rel browser test failures are address by this (since it guarantees we'll only issue deleted callbacks if we've issued the created callbacks).
Great point about this leaking info about the prerendering drivers! I'll try to filter the vended vector as you say.
Related details
- Christoph Schwering
- Dave Tapuska
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
std::vector<ContentAutofillDriver*>
ContentAutofillDriverFactory::GetExistingDrivers(
base::PassKey<ScopedAutofillManagersObservation>) {
std::vector<ContentAutofillDriver*> drivers;
drivers.reserve(driver_map_.size());
for (const std::pair<content::RenderFrameHost*,
std::unique_ptr<ContentAutofillDriver>>& entry :
driver_map_) {
drivers.push_back(entry.second.get());
}
return drivers;
}Christoph SchweringThis may leak prerendering drivers. It should suffice to only add activated drivers to `drivers`.
Ian VollickThis might also be why the bots crash (only had a very brief look but the `ScopedWhateverObservation`, which calls `GetExistingDrivers()` seems to play a role).
Sorry, the change wasn't quite ready for review -- I was trying out a patchset where I did without the flat_set of deferred RFHs (I had two tests that were succeeding without it and I wanted to see if it could be removed). I've put this back and checked locally that all the linux-rel browser test failures are address by this (since it guarantees we'll only issue deleted callbacks if we've issued the created callbacks).
Great point about this leaking info about the prerendering drivers! I'll try to filter the vended vector as you say.
Done
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
- Christoph Schwering
- Dave Tapuska
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
Ok, I think this is a better path forward. We now always defer the creation / will-be-deleted notifications for prerendered frames, though it took some work to update tests to cope with this since these notifications are what trigger injection.
Vasilii Sukhanov (Gerrit)
Vasilii Sukhanov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vasilii Sukhanov
| Code-Review | +1 |
1 comment
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc LGTM
- Christoph Schwering
- Dave Tapuska
- Ian Vollick
- Jan Keitel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jan Keitel (Gerrit)
Jan Keitel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jan Keitel
| Code-Review | +1 |
2 comments
The changes to `//components/android_autofill` LGTM, but I don't fully follow why this CL is needed in the first place. Ultimately, I am happy to defer to Chris, though, who's worked more with the driver code.
[Autofill] Add prerendering check to ContentAutofillDriver::GetForRFHBig disclaimer: I am not aware of all of the history of the pre-rendering related changes to Autofill, so I might be missing a major point here. 😊
However, I have a naive question about this CL: I understand that we shouldn't handle pre-renderer frames, but what is the actual security risk in case that happens? I don't think I understand the need for the additional complexity.
- If a renderer is compromised, are there any risk vectors that a prerendering frame offers that a non-prerendering frame wouldn't?
- Similarly, for a non-compromised renderer in which a page "just" uses, say, JS, to try and trick the user into submitting data that they don't intend to do - what could happen? Since (almost) all of our preview and filling actions depend on user interaction with the page, I don't understand how having a pre-rendered frame could make anything worse.
Related details
- Christoph Schwering
- Dave Tapuska
- Ian Vollick
Ian Vollick (Gerrit)
Ian Vollick added 2 comments![]( "Open in Gerrit")
I'm currently looking at adding analogous code on the PWM side. I'm going to move this CL back to WIP while i do that.
[Autofill] Add prerendering check to ContentAutofillDriver::GetForRFHBig disclaimer: I am not aware of all of the history of the pre-rendering related changes to Autofill, so I might be missing a major point here. 😊
However, I have a naive question about this CL: I understand that we shouldn't handle pre-renderer frames, but what is the actual security risk in case that happens? I don't think I understand the need for the additional complexity.
- If a renderer is compromised, are there any risk vectors that a prerendering frame offers that a non-prerendering frame wouldn't?
- Similarly, for a non-compromised renderer in which a page "just" uses, say, JS, to try and trick the user into submitting data that they don't intend to do - what could happen? Since (almost) all of our preview and filling actions depend on user interaction with the page, I don't understand how having a pre-rendered frame could make anything worse.
This code is actually about the browser-side, not renderer-side. We have bad_message checks in the drivers to catch messages from a prerendered frame, but those checks wouldn't catch browser-side code due to prerendering (eg, due to a navigation event) inadvertently interacting with other frames.
The unique thing about a prerendered frame is that it shares a WebContents with an active page, but isn't part of it (it's also live in the sense that it's running script). A prerendered frame should have no impact on the active frames and the goal of this CL is to add some checks to catch cases where that might happen.
As for what sorts of issues could occur, a navigation in a prerendered frame might (if there were bugs) cause autofill / pwm popups to be hidden. This particular issue has been fixed, but IIUC there's a desire to make the autofill manager per-tab (i.e., per WebContents) which seems like it could increase the likelihood of a similar bug cropping up, so I'm hoping to introduce some checks to help avoid that.
Related details
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
Christoph Schwering (Gerrit)
Christoph Schwering added 2 comments![]( "Open in Gerrit")
[Autofill] Add prerendering check to ContentAutofillDriver::GetForRFHBig disclaimer: I am not aware of all of the history of the pre-rendering related changes to Autofill, so I might be missing a major point here. 😊
However, I have a naive question about this CL: I understand that we shouldn't handle pre-renderer frames, but what is the actual security risk in case that happens? I don't think I understand the need for the additional complexity.
- If a renderer is compromised, are there any risk vectors that a prerendering frame offers that a non-prerendering frame wouldn't?
- Similarly, for a non-compromised renderer in which a page "just" uses, say, JS, to try and trick the user into submitting data that they don't intend to do - what could happen? Since (almost) all of our preview and filling actions depend on user interaction with the page, I don't understand how having a pre-rendered frame could make anything worse.
In the [DCHECK CL](https://chromium-review.googlesource.com/c/chromium/src/+/5538182/comment/9d7a6975_eae07b3c/), Ian wrote about prerendered frames:
It's just they're live (running script), contained within the same WebContents as an active Page, but unlike iframes they aren't part of the active page shouldn't impact it (or be user visible).
I don't really see why that makes them more critical than invisible iframes or fenced frames though.
I'm not excited about the additional complexity either but I hope it's in intermediate step to [deferring Autofill{Agent,Driver} creation](
https://g-issues.chromium.org/issues/342132628). (I hope I'm not overlooking any showstoppers.)
Ian, do the prerendering checks also cover portals or other usages of MPArch? I'm very unfamiliar with MPArch applications other than BFcache, fenced frames, and prerendering.
auto iter = driver_map_.find(navigation_handle->GetRenderFrameHost());
CHECK(iter != driver_map_.end());
// We use the reference here rather than |driver| since the creation
// callbacks can change the driver and the new driver must be used when
// notifying subsequent observers and for the remainder of this function.
std::unique_ptr<ContentAutofillDriver>& driver_ref = iter->second;Ian Vollicknit: move inside `if` below?
I think I need it here so I can use |driver_ref| at the very end of the fn when I call Reset as well (otherwise I wouldn't be calling Reset on the mocked/injected driver).
- Jan Keitel
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
[Autofill] Add prerendering check to ContentAutofillDriver::GetForRFHChristoph SchweringBig disclaimer: I am not aware of all of the history of the pre-rendering related changes to Autofill, so I might be missing a major point here. 😊
However, I have a naive question about this CL: I understand that we shouldn't handle pre-renderer frames, but what is the actual security risk in case that happens? I don't think I understand the need for the additional complexity.
- If a renderer is compromised, are there any risk vectors that a prerendering frame offers that a non-prerendering frame wouldn't?
- Similarly, for a non-compromised renderer in which a page "just" uses, say, JS, to try and trick the user into submitting data that they don't intend to do - what could happen? Since (almost) all of our preview and filling actions depend on user interaction with the page, I don't understand how having a pre-rendered frame could make anything worse.
In the [DCHECK CL](https://chromium-review.googlesource.com/c/chromium/src/+/5538182/comment/9d7a6975_eae07b3c/), Ian wrote about prerendered frames:
It's just they're live (running script), contained within the same WebContents as an active Page, but unlike iframes they aren't part of the active page shouldn't impact it (or be user visible).
I don't really see why that makes them more critical than invisible iframes or fenced frames though.
I'm not excited about the additional complexity either but I hope it's in intermediate step to [deferring Autofill{Agent,Driver} creation](
https://g-issues.chromium.org/issues/342132628). (I hope I'm not overlooking any showstoppers.)Ian, do the prerendering checks also cover portals or other usages of MPArch? I'm very unfamiliar with MPArch applications other than BFcache, fenced frames, and prerendering.
Race condition -- hadn't seen Ian's comment before sending mine :)
Related details
- Ian Vollick
- Jan Keitel
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
[Autofill] Add prerendering check to ContentAutofillDriver::GetForRFHChristoph SchweringBig disclaimer: I am not aware of all of the history of the pre-rendering related changes to Autofill, so I might be missing a major point here. 😊
However, I have a naive question about this CL: I understand that we shouldn't handle pre-renderer frames, but what is the actual security risk in case that happens? I don't think I understand the need for the additional complexity.
- If a renderer is compromised, are there any risk vectors that a prerendering frame offers that a non-prerendering frame wouldn't?
- Similarly, for a non-compromised renderer in which a page "just" uses, say, JS, to try and trick the user into submitting data that they don't intend to do - what could happen? Since (almost) all of our preview and filling actions depend on user interaction with the page, I don't understand how having a pre-rendered frame could make anything worse.
Christoph SchweringIn the [DCHECK CL](https://chromium-review.googlesource.com/c/chromium/src/+/5538182/comment/9d7a6975_eae07b3c/), Ian wrote about prerendered frames:
It's just they're live (running script), contained within the same WebContents as an active Page, but unlike iframes they aren't part of the active page shouldn't impact it (or be user visible).
I don't really see why that makes them more critical than invisible iframes or fenced frames though.
I'm not excited about the additional complexity either but I hope it's in intermediate step to [deferring Autofill{Agent,Driver} creation](
https://g-issues.chromium.org/issues/342132628). (I hope I'm not overlooking any showstoppers.)Ian, do the prerendering checks also cover portals or other usages of MPArch? I'm very unfamiliar with MPArch applications other than BFcache, fenced frames, and prerendering.
Race condition -- hadn't seen Ian's comment before sending mine :)
Wdyt, Dave? Do you think we should drop this change in favor of deferral?
Related details
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Vasilii Sukhanov
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
[Autofill] Add prerendering check to ContentAutofillDriver::GetForRFHChristoph SchweringBig disclaimer: I am not aware of all of the history of the pre-rendering related changes to Autofill, so I might be missing a major point here. 😊
However, I have a naive question about this CL: I understand that we shouldn't handle pre-renderer frames, but what is the actual security risk in case that happens? I don't think I understand the need for the additional complexity.
- If a renderer is compromised, are there any risk vectors that a prerendering frame offers that a non-prerendering frame wouldn't?
- Similarly, for a non-compromised renderer in which a page "just" uses, say, JS, to try and trick the user into submitting data that they don't intend to do - what could happen? Since (almost) all of our preview and filling actions depend on user interaction with the page, I don't understand how having a pre-rendered frame could make anything worse.
Christoph SchweringIn the [DCHECK CL](https://chromium-review.googlesource.com/c/chromium/src/+/5538182/comment/9d7a6975_eae07b3c/), Ian wrote about prerendered frames:
It's just they're live (running script), contained within the same WebContents as an active Page, but unlike iframes they aren't part of the active page shouldn't impact it (or be user visible).
I don't really see why that makes them more critical than invisible iframes or fenced frames though.
I'm not excited about the additional complexity either but I hope it's in intermediate step to [deferring Autofill{Agent,Driver} creation](
https://g-issues.chromium.org/issues/342132628). (I hope I'm not overlooking any showstoppers.)Ian, do the prerendering checks also cover portals or other usages of MPArch? I'm very unfamiliar with MPArch applications other than BFcache, fenced frames, and prerendering.
Ian VollickRace condition -- hadn't seen Ian's comment before sending mine :)
Wdyt, Dave? Do you think we should drop this change in favor of deferral?
So while the ideal would be that Autofill would used a non-associated mojo channel that can be deferred.. It appears that it was originally written that way but https://issues.chromium.org/issues/40586253 was created because of dependencies the autofill code had on frame lifecycle. Christoph do those still exist?
Either way moving the mojo channel to be non associated or not binding it until later seems reasonable but with certainly higher risk. (either out of order messages autofill expects, or missed messages with the other solution).
Related details
- Christoph Schwering
- Ian Vollick
- Jan Keitel
- Vasilii Sukhanov
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
[Autofill] Add prerendering check to ContentAutofillDriver::GetForRFHChristoph SchweringBig disclaimer: I am not aware of all of the history of the pre-rendering related changes to Autofill, so I might be missing a major point here. 😊
However, I have a naive question about this CL: I understand that we shouldn't handle pre-renderer frames, but what is the actual security risk in case that happens? I don't think I understand the need for the additional complexity.
- If a renderer is compromised, are there any risk vectors that a prerendering frame offers that a non-prerendering frame wouldn't?
- Similarly, for a non-compromised renderer in which a page "just" uses, say, JS, to try and trick the user into submitting data that they don't intend to do - what could happen? Since (almost) all of our preview and filling actions depend on user interaction with the page, I don't understand how having a pre-rendered frame could make anything worse.
Christoph SchweringIn the [DCHECK CL](https://chromium-review.googlesource.com/c/chromium/src/+/5538182/comment/9d7a6975_eae07b3c/), Ian wrote about prerendered frames:
It's just they're live (running script), contained within the same WebContents as an active Page, but unlike iframes they aren't part of the active page shouldn't impact it (or be user visible).
I don't really see why that makes them more critical than invisible iframes or fenced frames though.
I'm not excited about the additional complexity either but I hope it's in intermediate step to [deferring Autofill{Agent,Driver} creation](
https://g-issues.chromium.org/issues/342132628). (I hope I'm not overlooking any showstoppers.)Ian, do the prerendering checks also cover portals or other usages of MPArch? I'm very unfamiliar with MPArch applications other than BFcache, fenced frames, and prerendering.
Ian VollickRace condition -- hadn't seen Ian's comment before sending mine :)
Dave TapuskaWdyt, Dave? Do you think we should drop this change in favor of deferral?
So while the ideal would be that Autofill would used a non-associated mojo channel that can be deferred.. It appears that it was originally written that way but https://issues.chromium.org/issues/40586253 was created because of dependencies the autofill code had on frame lifecycle. Christoph do those still exist?
Either way moving the mojo channel to be non associated or not binding it until later seems reasonable but with certainly higher risk. (either out of order messages autofill expects, or missed messages with the other solution).
It's worth noting that we're doing some reordering at the moment. Deferring messages in the autofill and pwm agents (see crrev.com/c/2970765) will change ordering, too, of events generated pre-activation. Looking at the patch description, I'd written "Since these deferred messages should not be sent if the prerender is cancelled, I don't think this deferral will cause the same issues."
This seems to address detach (since I presume we will have cancelled prerendering if we've detached), but will cause other ordering changes in messages pre-activation since the autofill messages will be deferred.
+cc rockot@. The bug (https://crbug.com/866616) mentions detach, but Ken, are there other ordering issues beyond detach? I was looking at crrev.com/c/1145692 where you'd mentioned dependencies.
Related details
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Ken Rockot
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jan Keitel (Gerrit)
Jan Keitel added 1 comment![]( "Open in Gerrit")
Hi Ian,
Re renderer vs browser: Oh, you're right - I had assumed that this was also to catch situations in which the renderer tricks the browser into interacting with a prerendered frame.
To be honest, a security concern would be the only reason for me to add this complexity. Mistakenly hiding the popup would be a bug, but a pretty mild one. It'd be annoying, but it cannot be exploited in any way.
Deferring binding, on the other hand, sounds desirable if that can be done. IIUC, that would allow us to get rid of the `DeferringAutofillDriver`, wouldn't it?
Related details
- Christoph Schwering
- Dave Tapuska
- Ian Vollick
- Ken Rockot
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Rockot (Gerrit)
Ken Rockot added 1 comment![]( "Open in Gerrit")
Sorry, I long ago lost any memory of detailed issues around that change. Even if there weren't other issues that still remain, there could always be new ones introduced since then.
I can say that I recently attempted to *undo* this very change for a new experiment (https://chromium-review.googlesource.com/c/chromium/src/+/5530979), but it's causing failures that I haven't had time to investigate yet. I suspect (hope?) they're test-only failures, but they could be a sign of something more subtle.
Related details
- Christoph Schwering
- Dave Tapuska
- Ian Vollick
- Jan Keitel
- Vasilii Sukhanov
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
[Autofill] Add prerendering check to ContentAutofillDriver::GetForRFHThanks, Ken (I've added your comments to crbug.com/342132628). Looking at the failures from the change you mention, they don't appear related to either autofill/pwm or prerender, though I may be missing things.
To be honest, a security concern would be the only reason for me to add this complexity. Mistakenly hiding the popup would be a bug, but a pretty mild one. It'd be annoying, but it cannot be exploited in any way.
The hope was to add checks to catch a class of future bug where a prerendered frame inadvertently interacts with other frames via autofill / pwm -- popup hiding was just one example.
I had some DCHECKS for this that were removed in crrev.com/c/5538182. Dave hadn't liked the previous checks, either, and suggested a better way to implement them by adding CHECKS to ContentAutofillDriver which lead to this patch.
As for complexity, it has indeed grown as I iterated on the patch. Much of the CL is migrating callers of DriverForFrame, but there is some complexity in deferring notifications from ContentAutofillDriverFactory. The fallout of that change seems mainly related to tests (which depended on the notification for setting up mocks).
I finished updating the patch to add the analogous changes for PWM, and it seems simpler on that side since we didn't have those same notifications to defer.
Deferring binding, on the other hand, sounds desirable if that can be done. IIUC, that would allow us to get rid of the DeferringAutofillDriver, wouldn't it?
It does seem ideal (and seems to avoid reordering issues prior to activation), but I'm not sure how tough it'll be or if we'll hit roadblocks. I've started to rough this out here: https://chromium-review.googlesource.com/c/chromium/src/+/5630210. It's still very early, though, and doesn't work yet.
Related details
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Ken Rockot
- Vasilii Sukhanov
Ian Vollick (Gerrit)
Ian Vollick abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |