chromedriver: forward unknown responses to children [chromium/src : main]
Alex N. Jose (Gerrit)
Alex N. Jose added 1 comment![]( "Open in Gerrit")
Can you add a test to [run_py_tests.py](https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/chromedriver/test/run_py_tests.py;l=6317?q=run_py_tests.py&ss=chromium%2Fchromium%2Fsrc) that covers this scenario?
+ @alexr...@chromium.org, how does Puppeteer handle this?
Related details
- David Stevens
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Stevens (Gerrit)
David Stevens added 1 comment![]( "Open in Gerrit")
Can you add a test to [run_py_tests.py](https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/chromedriver/test/run_py_tests.py;l=6317?q=run_py_tests.py&ss=chromium%2Fchromium%2Fsrc) that covers this scenario?
+ @alexr...@chromium.org, how does Puppeteer handle this?
I've spent a while trying, and I can't manage to write a regression test that covers this issue. I've uploaded the chromedriver logs captured during a repro of this issue from the linked bug [1]. The specific command that ends up hanging in that trace is `Target.setAutoAttach (id=1384)`. It looks like that event is related to an ads frame, and it's racing with a Page.frameDetached event. Do you have any advice as to how to simulate that sort of sequence of events in the chromedriver tests?
[1] https://drive.google.com/file/d/1BVb9l2qfS6OSAhKZSMrdT3lYjBZYcFRs/view?usp=sharing
Related details
- Alex N. Jose
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. |
Alex N. Jose (Gerrit)
Alex N. Jose added 1 comment![]( "Open in Gerrit")
David StevensCan you add a test to [run_py_tests.py](https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/chromedriver/test/run_py_tests.py;l=6317?q=run_py_tests.py&ss=chromium%2Fchromium%2Fsrc) that covers this scenario?
+ @alexr...@chromium.org, how does Puppeteer handle this?
I've spent a while trying, and I can't manage to write a regression test that covers this issue. I've uploaded the chromedriver logs captured during a repro of this issue from the linked bug [1]. The specific command that ends up hanging in that trace is `Target.setAutoAttach (id=1384)`. It looks like that event is related to an ads frame, and it's racing with a Page.frameDetached event. Do you have any advice as to how to simulate that sort of sequence of events in the chromedriver tests?
[1] https://drive.google.com/file/d/1BVb9l2qfS6OSAhKZSMrdT3lYjBZYcFRs/view?usp=sharing
I'll try to go through the log early next week. Can you include the python test you created so I can try correlating? I'm concerned if we've introduced a regression. Does the test work as expected without the change?
Related details
- David Stevens
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 Stevens (Gerrit)
David Stevens added 1 comment![]( "Open in Gerrit")
David StevensCan you add a test to [run_py_tests.py](https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/chromedriver/test/run_py_tests.py;l=6317?q=run_py_tests.py&ss=chromium%2Fchromium%2Fsrc) that covers this scenario?
+ @alexr...@chromium.org, how does Puppeteer handle this?
Alex N. JoseI've spent a while trying, and I can't manage to write a regression test that covers this issue. I've uploaded the chromedriver logs captured during a repro of this issue from the linked bug [1]. The specific command that ends up hanging in that trace is `Target.setAutoAttach (id=1384)`. It looks like that event is related to an ads frame, and it's racing with a Page.frameDetached event. Do you have any advice as to how to simulate that sort of sequence of events in the chromedriver tests?
[1] https://drive.google.com/file/d/1BVb9l2qfS6OSAhKZSMrdT3lYjBZYcFRs/view?usp=sharing
I'll try to go through the log early next week. Can you include the python test you created so I can try correlating? I'm concerned if we've introduced a regression. Does the test work as expected without the change?
The logs are from one of my team's performance tests [1]. Running on an Android desktop device with 4GB of ram, the test fails at least 20% of the time due to this issue hanging web driver.
What I wasn't able to do was write a test in run_py_tests.py that reproduces the hang. Specifically, the issue seems to be caused by chromedriver issuing a `Target.setAutoAttach` command (id=1384 in the log) racing with a frame swap (the `Page.frameDetached` event at timestamp 1761773007.690) in a way that causes the browser to return a "Session with given id not found" response for the `Target.setAutoAttach` command. I tried causing the test page to navigate or close itself, but that resulted in a `Inspector.detached` event, which properly causes chromedriver to go down a different path that doesn't hang. I'm having trouble figuring out how to set up iframes in a way that will trigger the same `Page.frameDetached` path that we've observed in the performance test.
Related details
- Alex N. Jose
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. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 2 comments![]( "Open in Gerrit")
child client from being blocked indefinitely, it's necessary for theWe're generally supposed to know when a session is disconnected, through Target.detachedFromTarget event. This would be the right place to resolve all pending callbacks within the session.
auto result = client->ProcessOrphanedCommandResponse(response);You can't just send a response to a random session, given that command ids are per-session.
Related details
- Alex N. Jose
- David Stevens
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 Stevens (Gerrit)
David Stevens added 2 comments![]( "Open in Gerrit")
child client from being blocked indefinitely, it's necessary for theWe're generally supposed to know when a session is disconnected, through Target.detachedFromTarget event. This would be the right place to resolve all pending callbacks within the session.
There isn't a `Target.detachedFromTarget` for the session that's getting hung up.
That said, are frames supposed to be hierarchical? In trace [1], session 98137CE7779FEECC578D7EC1ED14D650 is the one that gets hung up. It never gets a Target.detachedFromTarget. However, there is a Target.detachedFromTarget for the session associated with its parentFrameId. Similarly in trace [2], session 08776C2E070D71D35250C2ABA796AE24 gets hung up, doesn't receive a Target.detachedFromTarget, but its parentFrameId does.
[1] https://drive.google.com/file/d/1BVb9l2qfS6OSAhKZSMrdT3lYjBZYcFRs/view?usp=sharing
[2] https://drive.google.com/file/d/13o1FccDYYGrL1ZOBz4oJoVXvAaGY8T4J/view?usp=sharing
auto result = client->ProcessOrphanedCommandResponse(response);You can't just send a response to a random session, given that command ids are per-session.
I thought child sessions always allocated their command ids from their parent session [1]. If the parent and all children are unique command ids from a shared namespace, then only the child that actually used the command id will process the response. I don't know if that's part of the CDP spec, but I think it should work as long as chromedriver stays consistent. Although I guess things could get weird if you try to do log replay on a trace that doesn't provide the guarantee.
Related details
- Alex N. Jose
- Andrey Kosyakov
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 Stevens (Gerrit)
David Stevens added 1 comment![]( "Open in Gerrit")
child client from being blocked indefinitely, it's necessary for theDavid StevensWe're generally supposed to know when a session is disconnected, through Target.detachedFromTarget event. This would be the right place to resolve all pending callbacks within the session.
There isn't a `Target.detachedFromTarget` for the session that's getting hung up.
That said, are frames supposed to be hierarchical? In trace [1], session 98137CE7779FEECC578D7EC1ED14D650 is the one that gets hung up. It never gets a Target.detachedFromTarget. However, there is a Target.detachedFromTarget for the session associated with its parentFrameId. Similarly in trace [2], session 08776C2E070D71D35250C2ABA796AE24 gets hung up, doesn't receive a Target.detachedFromTarget, but its parentFrameId does.
[1] https://drive.google.com/file/d/1BVb9l2qfS6OSAhKZSMrdT3lYjBZYcFRs/view?usp=sharing
[2] https://drive.google.com/file/d/13o1FccDYYGrL1ZOBz4oJoVXvAaGY8T4J/view?usp=sharing
After going through the browser devtools implementation, the session that hangs doesn't get a `Target.detachedFromTarget` event because of this check [1]. The parent frame's session is being detached first, which causes that check to drop the child frame's `Target.detachedFromTarget` event.
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 2 comments![]( "Open in Gerrit")
child client from being blocked indefinitely, it's necessary for theDavid StevensWe're generally supposed to know when a session is disconnected, through Target.detachedFromTarget event. This would be the right place to resolve all pending callbacks within the session.
David StevensThere isn't a `Target.detachedFromTarget` for the session that's getting hung up.
That said, are frames supposed to be hierarchical? In trace [1], session 98137CE7779FEECC578D7EC1ED14D650 is the one that gets hung up. It never gets a Target.detachedFromTarget. However, there is a Target.detachedFromTarget for the session associated with its parentFrameId. Similarly in trace [2], session 08776C2E070D71D35250C2ABA796AE24 gets hung up, doesn't receive a Target.detachedFromTarget, but its parentFrameId does.
[1] https://drive.google.com/file/d/1BVb9l2qfS6OSAhKZSMrdT3lYjBZYcFRs/view?usp=sharing
[2] https://drive.google.com/file/d/13o1FccDYYGrL1ZOBz4oJoVXvAaGY8T4J/view?usp=sharing
After going through the browser devtools implementation, the session that hangs doesn't get a `Target.detachedFromTarget` event because of this check [1]. The parent frame's session is being detached first, which causes that check to drop the child frame's `Target.detachedFromTarget` event.
Well, frames are hierarchical, but sessions, provided are just 2 tiers (provided {flatten: true} in `Target.setAutoAttach`, which it is. It looks like [1] in your last comment may be something we should fix on the backend side then, thanks a lot for investigating!
auto result = client->ProcessOrphanedCommandResponse(response);David StevensYou can't just send a response to a random session, given that command ids are per-session.
I thought child sessions always allocated their command ids from their parent session [1]. If the parent and all children are unique command ids from a shared namespace, then only the child that actually used the command id will process the response. I don't know if that's part of the CDP spec, but I think it should work as long as chromedriver stays consistent. Although I guess things could get weird if you try to do log replay on a trace that doesn't provide the guarantee.
Thanks for pointing out, I missed that (I'm not well faimiliar with the chromedriver implmentation and had a bit of expectation bias based on other client implementations). I still think we better fix `Target.detachedFromTarget` to be properly dispatched.
Related details
- Alex N. Jose
- David Stevens
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. |