Extensions: SidePanel: Delay .onOpened until ExtensionHost loads [chromium/src : main]
Solomon Kinard (Gerrit)
Solomon Kinard has uploaded the change for review![]( "Open in Gerrit")
Commit message
Extensions: SidePanel: Delay .onOpened until ExtensionHost loads
Previously, the chrome.sidePanel.onOpened event was dispatched
immediately when the side panel UI opened. However, at this exact
moment, the ExtensionHost had not yet finished its first load, meaning
its underlying RenderFrameHost was not fully registered with the
ProcessManager. As a result, calls to chrome.runtime.getContexts()
within an onOpened listener would fail to discover the newly created
side panel context and unexpectedly return an empty array.
This CL fixes the race condition by having ExtensionSidePanelCoordinator
observe the ExtensionHost. We now delay dispatching the onOpened event
until the host completes its first load (has_loaded_once() becomes true).
Additionally, this introduces has_dispatched_on_opened_ to track the
dispatch state. This guarantees that if a user rapidly closes the side
panel before it finishes loading (preventing onOpened from firing),
we avoid dispatching an unmatched onClosed event.
Change diff
Change information
- M chrome/browser/extensions/api/runtime/runtime_apitest.cc
- M chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc
- M chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.h
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Solomon Kinard (Gerrit)
Solomon Kinard added 1 comment![]( "Open in Gerrit")
Related details
- Kelvin Jiang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kelvin Jiang (Gerrit)
Kelvin Jiang added 9 comments![]( "Open in Gerrit")
IN_PROC_BROWSER_TEST_F(RuntimeGetContextsSidePanelTest,Curious: Were we able to successfully repro the race condition?
chrome.test.fail('Expected 1 context, but got ' + contexts.length);nit: template strings
// Wait for the result. If it fails (e.g., the bug is present and returns 0),tiny nit: extra comma
bool has_dispatched_on_opened_ = false;nit: `on_opened_dispatched_` ?
// is destroyed.nit: since it's mentioned in the CL description, let's update the comment here to mention that the actual OnClosed() event should only be dispatched if OnOpened() was dispatched prior?
OnOpened();So it's possible that OnOpened never gets called because the panel UI is open but the host never finishes loading so the extension's side panel content never gets rendered, but this would be WAI?
scoped_host_observation_.Reset();nit: let's reset the observation right after `host_` gets created, around line 265?
if (has_dispatched_on_opened_) {
OnClosed();
}Nit: noticed a few times OnClosed() was wrapped with this if statement. Why not have OnClosed() no-op if `has_dispatched_on_opened_` is false inside the function?
Related details
- Solomon Kinard
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. |
Solomon Kinard (Gerrit)
Solomon Kinard added 8 comments![]( "Open in Gerrit")
IN_PROC_BROWSER_TEST_F(RuntimeGetContextsSidePanelTest,Curious: Were we able to successfully repro the race condition?
Is the question does the test not pass before changes to the non-test files? If so, the answer is yes.
chrome.test.fail('Expected 1 context, but got ' + contexts.length);Solomon Kinardnit: template strings
Done
// Wait for the result. If it fails (e.g., the bug is present and returns 0),Solomon Kinardtiny nit: extra comma
Done
bool has_dispatched_on_opened_ = false;Solomon Kinardnit: `on_opened_dispatched_` ?
Done
nit: since it's mentioned in the CL description, let's update the comment here to mention that the actual OnClosed() event should only be dispatched if OnOpened() was dispatched prior?
Done
OnOpened();So it's possible that OnOpened never gets called because the panel UI is open but the host never finishes loading so the extension's side panel content never gets rendered, but this would be WAI?
Yes, the intention was that it is safer to not fire onOpened at all rather than fire it before the context is ready (which causes the empty array issue).
However, I might be missing a downstream consequence of leaving the extension waiting indefinitely if the host hangs. Is there a preferred way to handle this edge case?
nit: let's reset the observation right after `host_` gets created, around line 265?
- Kelvin Jiang
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. |
Kelvin Jiang (Gerrit)
Kelvin Jiang voted and added 6 comments![]( "Open in Gerrit")
Votes added by Kelvin Jiang
| Code-Review | +1 |
6 comments
Thanks Solomon! This LG though I'm passing this comment to Devlin:
https://chromium-review.git.corp.google.com/c/chromium/src/+/8008154/comment/0e1acc71_372b1030/
IN_PROC_BROWSER_TEST_F(RuntimeGetContextsSidePanelTest,Solomon KinardCurious: Were we able to successfully repro the race condition?
Is the question does the test not pass before changes to the non-test files? If so, the answer is yes.
works for me
// Trigger the action.onClicked listenerminiscule nit: missing period
OnOpened();Solomon KinardSo it's possible that OnOpened never gets called because the panel UI is open but the host never finishes loading so the extension's side panel content never gets rendered, but this would be WAI?
Yes, the intention was that it is safer to not fire onOpened at all rather than fire it before the context is ready (which causes the empty array issue).
However, I might be missing a downstream consequence of leaving the extension waiting indefinitely if the host hangs. Is there a preferred way to handle this edge case?
Someone more familiar with top level extension UI itself may be more knowledgeable on this since extension hosts aren't just for side panels?
scoped_host_observation_.Observe(host_.get());nit: move this after the `if (!host_)` block
Nit: noticed a few times OnClosed() was wrapped with this if statement. Why not have OnClosed() no-op if `has_dispatched_on_opened_` is false inside the function?
- Devlin Cronin
- Solomon Kinard
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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 2 comments![]( "Open in Gerrit")
(just responding; didn't review fully. But no high level concerns)
OnOpened();Solomon KinardSo it's possible that OnOpened never gets called because the panel UI is open but the host never finishes loading so the extension's side panel content never gets rendered, but this would be WAI?
Kelvin JiangYes, the intention was that it is safer to not fire onOpened at all rather than fire it before the context is ready (which causes the empty array issue).
However, I might be missing a downstream consequence of leaving the extension waiting indefinitely if the host hangs. Is there a preferred way to handle this edge case?
Someone more familiar with top level extension UI itself may be more knowledgeable on this since extension hosts aren't just for side panels?
ExtensionHosts aren't just for side panels, but this doesn't seem to be changing ExtensionHosts at all -- it's just using the notification from the host to trigger the OnOpened event, which is only for side panels.
I think this is fine -- if the extension never loads once, we don't dispatch the onOpened event, but that could even be working as intended. A developer would expect that the side panel be "available" once onOpened is fired, and any number of operations (such as getContexts, as in the bug, or sending a message) might fail if it's not initialized yet.
Overall, I think this is fine.
Kelvin, did you have a particular concern with regard to the extension host integration?
Related details
- Solomon Kinard
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. |
Kelvin Jiang (Gerrit)
Kelvin Jiang voted and added 4 comments![]( "Open in Gerrit")
Votes added by Kelvin Jiang
| Code-Review | +1 |
4 comments
s LGTM thanks Solomon!
// Trigger the action.onClicked listenerKelvin Jiangminiscule nit: missing period
(resolved)
OnOpened();Solomon KinardSo it's possible that OnOpened never gets called because the panel UI is open but the host never finishes loading so the extension's side panel content never gets rendered, but this would be WAI?
Kelvin JiangYes, the intention was that it is safer to not fire onOpened at all rather than fire it before the context is ready (which causes the empty array issue).
However, I might be missing a downstream consequence of leaving the extension waiting indefinitely if the host hangs. Is there a preferred way to handle this edge case?
Devlin CroninSomeone more familiar with top level extension UI itself may be more knowledgeable on this since extension hosts aren't just for side panels?
ExtensionHosts aren't just for side panels, but this doesn't seem to be changing ExtensionHosts at all -- it's just using the notification from the host to trigger the OnOpened event, which is only for side panels.
I think this is fine -- if the extension never loads once, we don't dispatch the onOpened event, but that could even be working as intended. A developer would expect that the side panel be "available" once onOpened is fired, and any number of operations (such as getContexts, as in the bug, or sending a message) might fail if it's not initialized yet.
Overall, I think this is fine.
Kelvin, did you have a particular concern with regard to the extension host integration?
no just wanted to make sure I'm not missing anything
nit: move this after the `if (!host_)` block
(Resolved)
Related details
- Solomon Kinard
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Solomon Kinard (Gerrit)
Solomon Kinard 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. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Extensions: SidePanel: Delay .onOpened until ExtensionHost loads
Previously, the chrome.sidePanel.onOpened event was dispatched
immediately when the side panel UI opened. However, at this exact
moment, the ExtensionHost had not yet finished its first load, meaning
its underlying RenderFrameHost was not fully registered with the
ProcessManager. As a result, calls to chrome.runtime.getContexts()
within an onOpened listener would fail to discover the newly created
side panel context and unexpectedly return an empty array.
This CL fixes the race condition by having ExtensionSidePanelCoordinator
observe the ExtensionHost. Dispatching now delays the onOpened event
until the host completes its first load (has_loaded_once() becomes
true). Additionally, this introduces on_opened_dispatched_ to track the
dispatch state. This guarantees that if a user rapidly closes the side
panel before it finishes loading (preventing onOpened from firing), it
avoids dispatching an unmatched onClosed event.
- M chrome/browser/extensions/api/runtime/runtime_apitest.cc
- M chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc
- M chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.h
Code-Review: +1 by Kelvin Jiang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
luci-bisection@appspot.gserviceaccount.com (Gerrit)
Message from luci-bi...@appspot.gserviceaccount.com![]( "Open in Gerrit")
LUCI Bisection has identified this change as the cause of a test failure. See the analysis: https://ci.chromium.org/ui/p/chromium/bisection/test-analysis/b/5368776729034752
Sample build with failed test: https://ci.chromium.org/b/8675502500319981073
Affected test(s):
[://chrome/test\:browser_tests!gtest::ExtensionOnClosedEventSidePanelBrowserTest#OnClosedEvent_WindowClosed](https://ci.chromium.org/ui/test/chromium/:%2F%2Fchrome%2Ftest%5C:browser_tests%21gtest::ExtensionOnClosedEventSidePanelBrowserTest%23OnClosedEvent_WindowClosed?q=VHash%3Aec8fe31d28138961)
A revert for this change was not created because the builder that this CL broke is not watched by gardeners, therefore less important. You can consider revert this CL, fix forward or let builder owners resolve it themselves.
If this is a false positive, please report it at http://b.corp.google.com/createIssue?component=1199205&description=Analysis%3A+https%3A%2F%2Fci.chromium.org%2Fui%2Fp%2Fchromium%2Fbisection%2Ftest-analysis%2Fb%2F5368776729034752&format=PLAIN&priority=P3&title=Wrongly+blamed+https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fsrc%2F%2B%2F8008154&type=BUG
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ali Hijazi (Gerrit)
Ali Hijazi has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Solomon Kinard (Gerrit)
Solomon Kinard has uploaded the change for review![]( "Open in Gerrit")
Commit message
Extensions: SidePanel: Delay .onOpened until ExtensionHost loads
Previously, the chrome.sidePanel.onOpened event was dispatched
immediately when the side panel UI opened. However, at this exact
moment, the ExtensionHost had not yet finished its first load, meaning
its underlying RenderFrameHost was not fully registered with the
ProcessManager. As a result, calls to chrome.runtime.getContexts()
within an onOpened listener would fail to discover the newly created
side panel context and unexpectedly return an empty array.
This CL fixes the race condition by having ExtensionSidePanelCoordinator
observe the ExtensionHost. Dispatching now delays the onOpened event
until the host completes its first load (has_loaded_once() becomes
true). Additionally, this introduces on_opened_dispatched_ to track the
dispatch state. This guarantees that if a user rapidly closes the side
panel before it finishes loading (preventing onOpened from firing), it
avoids dispatching an unmatched onClosed event.
Change diff
Change information
- M chrome/browser/extensions/api/runtime/runtime_apitest.cc
- M chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc
- M chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.h
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Solomon Kinard (Gerrit)
Solomon Kinard added 2 comments![]( "Open in Gerrit")
host_helper.WaitForHostCompletedFirstLoad();1. [Diff](crrev.com/c/8137935/1..2/chrome/browser/ui/views/side_panel/extensions/extension_side_panel_browsertest.cc) of reland that now waits for onOpened to be called before [continuing](https://ci.chromium.org/ui/b/8675489274430429729/test-results?q=ID:%3A%2F%2Fchrome%2Ftest%5C%3Abrowser_tests!gtest%3A%3AExtensionOnClosedEventSidePanelBrowserTest%23OnClosedEvent_WindowClosed%20V:bucket=ci%20V:builder=mac14-tests%20V:os=Mac-14%20V:test_suite=browser_tests).
1. No failed tests in [mac14-tests](https://ci.chromium.org/ui/p/chromium/builders/try/mac14-tests/360/overview).
Related details
- Kelvin Jiang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kelvin Jiang (Gerrit)
Kelvin Jiang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kelvin Jiang
| Code-Review | +1 |
1 comment
- Solomon Kinard
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Solomon Kinard (Gerrit)
Solomon Kinard 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. |