[actor] Support coordinate TOCTOU for all window types. [chromium/src : main]
Mark Foltz (Gerrit)
Mark Foltz added 2 comments![]( "Open in Gerrit")
if (target_node.IsNull()) {@bokan: I wasn't sure if this was necessary; the target_node should come from a hit test result in Blink IIUC. But we don't want to pass an `IsNull()` target_node to `observed_target_node.ContainsViaFlatTree` below.
if (hit_element.IsNull()) {Similar question here, can a hit test return a null WebElement?
Related details
- David Bokan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan voted and added 5 comments![]( "Open in Gerrit")
Votes added by David Bokan
| Code-Review | +1 |
5 comments
- Guard the ActorTabData access in RequestTabObservation() with a nullGiven the point above, when does this happen?
- Shorten kActorObservationDelayTimeout in GlicActorUiTest to 1s to
prevent test timeouts on slow or headless test bots.Can you elaborate on how this fixes the issue? Won't this likely lead to more flakiness as tests now unintentionally hit the now-shorter kActorObservationDelayTimeout?
// Disable TOCTOU validation because these tests do not populate
// observed_target in MakeScrollTo, which would cause validation to failhmm, tests shouldn't have to manually populate this. observed_target is computed in the browser as part of executing an action [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/actor/tools/page_tool.cc;l=496;drc=94b8dff8418534977a39c201e209e2efc30b48d7).
These tests call GetPageContextForActorTab which [should set](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/glic/host/glic_actor_interactive_uitest_common.cc;l=663;drc=94b8dff8418534977a39c201e209e2efc30b48d7) `last_observed_page_content` on the ActorTabData and the tests below inject an action using ExecuteAction which starts their journey at the [Glic API layer](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/glic/host/glic_actor_interactive_uitest_common.cc;l=128;drc=94b8dff8418534977a39c201e209e2efc30b48d7) so I would expect that this should work? Is there something else going on here or am I missing something?
if (target_node.IsNull()) {@bokan: I wasn't sure if this was necessary; the target_node should come from a hit test result in Blink IIUC. But we don't want to pass an `IsNull()` target_node to `observed_target_node.ContainsViaFlatTree` below.
The [comment](https://source.chromium.org/chromium/chromium/src/+/main:chrome/renderer/actor/tool_base.h;l=35;drc=73cf19fa4199ab7f2874f58cc7eda83261f017f5) in the header file says this can be null but I don't think it can. When the comment and struct were [added](https://chromium-review.git.corp.google.com/c/chromium/src/+/6684992) I think this _could_ be null but not for the reason the comment says - we null checked and returned base::unexpected if the DOMNodeId was not found - but because a coordinate hit test was extracted using GetElement. I believe Blink hit tests will always return DocumentNode as a last resort so GetElement would return null. But we [now](https://source.chromium.org/chromium/chromium/src/+/main:chrome/renderer/actor/tool_base.cc;l=168;drc=63ec5d57642d302b1f2be8207a6672edd989c034) use `GetNodeOrPseudoNode` which _I think_ should never return a null WebNode. That said, that's a hard expectation to verify visually so I'm not opposed to leaving the guard in place
if (hit_element.IsNull()) {Similar question here, can a hit test return a null WebElement?
yes, if you hit outside of any elements (e.g. document scrollbars IIRC or outside the <html> element) then you'd get the DocumentNode which is null in GetElement
Related details
- Mark Foltz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Foltz (Gerrit)
Mark Foltz voted and added 5 comments![]( "Open in Gerrit")
Votes added by Mark Foltz
| Commit-Queue | +1 |
5 comments
- Guard the ActorTabData access in RequestTabObservation() with a nullGiven the point above, when does this happen?
Good catch, it doesn't. I reverted this change to ActorKeyedService.
- Shorten kActorObservationDelayTimeout in GlicActorUiTest to 1s to
prevent test timeouts on slow or headless test bots.Can you elaborate on how this fixes the issue? Won't this likely lead to more flakiness as tests now unintentionally hit the now-shorter kActorObservationDelayTimeout?
Waiting the full, default 10s for the page to settle was causing some tests to hit test timeouts of 45s. The test pages themselves are very simple and should be stable in less than 1s, but something was causing page stability not to return.
I'll increase this to 3s to try to find a balance here.
// Disable TOCTOU validation because these tests do not populate
// observed_target in MakeScrollTo, which would cause validation to failhmm, tests shouldn't have to manually populate this. observed_target is computed in the browser as part of executing an action [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/actor/tools/page_tool.cc;l=496;drc=94b8dff8418534977a39c201e209e2efc30b48d7).
These tests call GetPageContextForActorTab which [should set](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/glic/host/glic_actor_interactive_uitest_common.cc;l=663;drc=94b8dff8418534977a39c201e209e2efc30b48d7) `last_observed_page_content` on the ActorTabData and the tests below inject an action using ExecuteAction which starts their journey at the [Glic API layer](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/glic/host/glic_actor_interactive_uitest_common.cc;l=128;drc=94b8dff8418534977a39c201e209e2efc30b48d7) so I would expect that this should work? Is there something else going on here or am I missing something?
After more digging the problem is the tests that use `ExecuteScrollToActionWithNodeId`. This helper extracts a nodeId directly from the DOM and attempts to scroll to DOM nodes that are not represented in the APC. Attempting TOCTOU on these actions is bound to fail.
This tool seems to be incompatible with actor's implementation of TOCTOU and maybe there should be a tool-specific way to opt out. For now I have restructured the test to disable TOCTOU for those tests specifically.
if (target_node.IsNull()) {David Bokan@bokan: I wasn't sure if this was necessary; the target_node should come from a hit test result in Blink IIUC. But we don't want to pass an `IsNull()` target_node to `observed_target_node.ContainsViaFlatTree` below.
The [comment](https://source.chromium.org/chromium/chromium/src/+/main:chrome/renderer/actor/tool_base.h;l=35;drc=73cf19fa4199ab7f2874f58cc7eda83261f017f5) in the header file says this can be null but I don't think it can. When the comment and struct were [added](https://chromium-review.git.corp.google.com/c/chromium/src/+/6684992) I think this _could_ be null but not for the reason the comment says - we null checked and returned base::unexpected if the DOMNodeId was not found - but because a coordinate hit test was extracted using GetElement. I believe Blink hit tests will always return DocumentNode as a last resort so GetElement would return null. But we [now](https://source.chromium.org/chromium/chromium/src/+/main:chrome/renderer/actor/tool_base.cc;l=168;drc=63ec5d57642d302b1f2be8207a6672edd989c034) use `GetNodeOrPseudoNode` which _I think_ should never return a null WebNode. That said, that's a hard expectation to verify visually so I'm not opposed to leaving the guard in place
OK, it sounds like I should leave this null check in.
If you want I can add a temporary DCHECK here that will generate a crash report if we hit this case in the wild, but then you'll need to check for reports and followup 😊
if (hit_element.IsNull()) {David BokanSimilar question here, can a hit test return a null WebElement?
yes, if you hit outside of any elements (e.g. document scrollbars IIRC or outside the <html> element) then you'd get the DocumentNode which is null in GetElement
- David Bokan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Foltz (Gerrit)
Mark Foltz added 1 comment![]( "Open in Gerrit")
- Alison Gale
- David Bokan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan voted and added 2 comments![]( "Open in Gerrit")
Votes added by David Bokan
| Code-Review | +1 |
2 comments
- Shorten kActorObservationDelayTimeout in GlicActorUiTest to 1s to
prevent test timeouts on slow or headless test bots.Mark FoltzCan you elaborate on how this fixes the issue? Won't this likely lead to more flakiness as tests now unintentionally hit the now-shorter kActorObservationDelayTimeout?
Waiting the full, default 10s for the page to settle was causing some tests to hit test timeouts of 45s. The test pages themselves are very simple and should be stable in less than 1s, but something was causing page stability not to return.
I'll increase this to 3s to try to find a balance here.
Ack - might be good to include a comment explaining why we're using the shortened delay in tests and that if that's fixed we could use the normal timeouts in tests too.
// Disable TOCTOU validation because these tests do not populate
// observed_target in MakeScrollTo, which would cause validation to failMark Foltzhmm, tests shouldn't have to manually populate this. observed_target is computed in the browser as part of executing an action [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/actor/tools/page_tool.cc;l=496;drc=94b8dff8418534977a39c201e209e2efc30b48d7).
These tests call GetPageContextForActorTab which [should set](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/glic/host/glic_actor_interactive_uitest_common.cc;l=663;drc=94b8dff8418534977a39c201e209e2efc30b48d7) `last_observed_page_content` on the ActorTabData and the tests below inject an action using ExecuteAction which starts their journey at the [Glic API layer](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/glic/host/glic_actor_interactive_uitest_common.cc;l=128;drc=94b8dff8418534977a39c201e209e2efc30b48d7) so I would expect that this should work? Is there something else going on here or am I missing something?
After more digging the problem is the tests that use `ExecuteScrollToActionWithNodeId`. This helper extracts a nodeId directly from the DOM and attempts to scroll to DOM nodes that are not represented in the APC. Attempting TOCTOU on these actions is bound to fail.
This tool seems to be incompatible with actor's implementation of TOCTOU and maybe there should be a tool-specific way to opt out. For now I have restructured the test to disable TOCTOU for those tests specifically.
Ah, I'm not sure it's specific to the tool. If the elements we're targeting here aren't added to APC (e.g. because they're not actionable / have no content?) then I think that's a test-bug, scroll_to should only be used to target things nodes in the cached APC.
Some tests are checking for things like "the specified DOMNodeId doesn't exist" which could happen in the real world cases, either because of an error server-side or because the page removed the DOMNodeID (in which case it should exist in cached APC).
I think the ideal fix would be to ensure the targeted nodes are put into the cached APC but I wouldn't block on this - maybe just leave a note?
Related details
- Alison Gale
- Mark Foltz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Foltz (Gerrit)
Mark Foltz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mark Foltz
| Commit-Queue | +1 |
2 comments
- Shorten kActorObservationDelayTimeout in GlicActorUiTest to 1s to
prevent test timeouts on slow or headless test bots.Mark FoltzCan you elaborate on how this fixes the issue? Won't this likely lead to more flakiness as tests now unintentionally hit the now-shorter kActorObservationDelayTimeout?
David BokanWaiting the full, default 10s for the page to settle was causing some tests to hit test timeouts of 45s. The test pages themselves are very simple and should be stable in less than 1s, but something was causing page stability not to return.
I'll increase this to 3s to try to find a balance here.
Ack - might be good to include a comment explaining why we're using the shortened delay in tests and that if that's fixed we could use the normal timeouts in tests too.
Done - added a comment where I set the timeout.
// Disable TOCTOU validation because these tests do not populate
// observed_target in MakeScrollTo, which would cause validation to failMark Foltzhmm, tests shouldn't have to manually populate this. observed_target is computed in the browser as part of executing an action [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/actor/tools/page_tool.cc;l=496;drc=94b8dff8418534977a39c201e209e2efc30b48d7).
These tests call GetPageContextForActorTab which [should set](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/glic/host/glic_actor_interactive_uitest_common.cc;l=663;drc=94b8dff8418534977a39c201e209e2efc30b48d7) `last_observed_page_content` on the ActorTabData and the tests below inject an action using ExecuteAction which starts their journey at the [Glic API layer](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/glic/host/glic_actor_interactive_uitest_common.cc;l=128;drc=94b8dff8418534977a39c201e209e2efc30b48d7) so I would expect that this should work? Is there something else going on here or am I missing something?
David BokanAfter more digging the problem is the tests that use `ExecuteScrollToActionWithNodeId`. This helper extracts a nodeId directly from the DOM and attempts to scroll to DOM nodes that are not represented in the APC. Attempting TOCTOU on these actions is bound to fail.
This tool seems to be incompatible with actor's implementation of TOCTOU and maybe there should be a tool-specific way to opt out. For now I have restructured the test to disable TOCTOU for those tests specifically.
Ah, I'm not sure it's specific to the tool. If the elements we're targeting here aren't added to APC (e.g. because they're not actionable / have no content?) then I think that's a test-bug, scroll_to should only be used to target things nodes in the cached APC.
Some tests are checking for things like "the specified DOMNodeId doesn't exist" which could happen in the real world cases, either because of an error server-side or because the page removed the DOMNodeID (in which case it should exist in cached APC).
I think the ideal fix would be to ensure the targeted nodes are put into the cached APC but I wouldn't block on this - maybe just leave a note?
Added a note to the test case. Do you know who wrote this tool?
Related details
- Alison Gale
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Gale (Gerrit)
Alison Gale voted Code-Review+1![]( "Open in Gerrit")
- Mark Foltz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Bokan
| Code-Review | +1 |
1 comment
// Disable TOCTOU validation because these tests do not populate
// observed_target in MakeScrollTo, which would cause validation to failMark Foltzhmm, tests shouldn't have to manually populate this. observed_target is computed in the browser as part of executing an action [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/actor/tools/page_tool.cc;l=496;drc=94b8dff8418534977a39c201e209e2efc30b48d7).
These tests call GetPageContextForActorTab which [should set](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/glic/host/glic_actor_interactive_uitest_common.cc;l=663;drc=94b8dff8418534977a39c201e209e2efc30b48d7) `last_observed_page_content` on the ActorTabData and the tests below inject an action using ExecuteAction which starts their journey at the [Glic API layer](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/glic/host/glic_actor_interactive_uitest_common.cc;l=128;drc=94b8dff8418534977a39c201e209e2efc30b48d7) so I would expect that this should work? Is there something else going on here or am I missing something?
David BokanAfter more digging the problem is the tests that use `ExecuteScrollToActionWithNodeId`. This helper extracts a nodeId directly from the DOM and attempts to scroll to DOM nodes that are not represented in the APC. Attempting TOCTOU on these actions is bound to fail.
This tool seems to be incompatible with actor's implementation of TOCTOU and maybe there should be a tool-specific way to opt out. For now I have restructured the test to disable TOCTOU for those tests specifically.
Mark FoltzAh, I'm not sure it's specific to the tool. If the elements we're targeting here aren't added to APC (e.g. because they're not actionable / have no content?) then I think that's a test-bug, scroll_to should only be used to target things nodes in the cached APC.
Some tests are checking for things like "the specified DOMNodeId doesn't exist" which could happen in the real world cases, either because of an error server-side or because the page removed the DOMNodeID (in which case it should exist in cached APC).
I think the ideal fix would be to ensure the targeted nodes are put into the cached APC but I wouldn't block on this - maybe just leave a note?
Added a note to the test case. Do you know who wrote this tool?
IIRC it was added in https://chromium-review.git.corp.google.com/c/chromium/src/+/6827047
Mark Foltz (Gerrit)
Mark Foltz voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |