a11y-canvas: Add heuristics to detect inaccessible canvases. [chromium/src : main]
Ramin Halavati (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Justin Novosad
- Lucas Radaelli
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
TaskRunnerTimer<AccessibilityManager> uma_timer_;nit: Blink Style Guide: Naming - Precede boolean values with words like “is” and “did”. Consider renaming `uma_recorded_` to `is_uma_recorded_` or `did_record_uma_`.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
- Justin Novosad
- Lucas Radaelli
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. |
Justin Novosad (Gerrit)
Justin Novosad added 8 comments![]( "Open in Gerrit")
// LINT.IfChange(CanvasHeuristicResult)nit: The label `CanvasHeuristicResult` should probably be changed to match the name of the enum.
class CORE_EXPORT AccessibilityManager {This class is big enough to be put in its own file. Also, you should probably give it a name that makes it clear that it is canvas-specific. Suggestion: HTMLCanvasAccessibilityManager
accessibility_manager_->SetVisible();How come we never reset AccessibilityManager::is_visible_ to false when the canvas is not longer visible? I wouldhave expected this code to be
```
if (accessibility_manager_) {
accessibility_manager_->SetVisible(displayed_);
}
```
If there is a reason not to do this, it should be explained in a comment, because right now it looks like a bug.
if (accessibility_manager_) {I don't think ContextDestroyed is the right place to put this code. The purpose of ContextDestroyed is to manage the mutual reference between the canvas element and the context because the object garbage collection order is non-deterministic. The accessibility manager does not interact with this reference cycle and is not impacted by the destruction order between canvas and context. Calling `FlushUmaIfNeeded` in the `Dispose` method (as you're already doing) should be sufficient.
Also, Why do we even need to call `FlushUmaIfNeeded` given that the destructor of AccesibilityManager already does it. If it's just to make unit tests work, you could instead teardown the document and trigger GC manually in your tests.
bool first_child_is_element = firstElementChild() != nullptr;The name of this variable does not correctly express what you're checking. Consider `has_child_element` or `has_fallback_element`
ElementTraversal::FirstChild(*canvas_element) != nullptr;This replicates logic you added in `HTMLCanvasElement::ChildrenChanged`. Consider adding a `HasFallbackElementContent()` method to HTMLCanvasElement. That way if we ever need to modify how this is determined, we won't need to worry about tracking down all the places in the code that need updating.
if (canvas_element->IsDisplayed()) {
SetVisible();
}Why not `SetVisible(canvas_element->IsDisplayed());`? Is there a reason _is_visible should never be reset to false?
uma_timer_.StartOneShot(base::Seconds(1), FROM_HERE);Why is this delay necessary? and why one second?
Related details
- Lucas Radaelli
- Ramin Halavati
Ramin Halavati (Gerrit)
Ramin Halavati added 9 comments![]( "Open in Gerrit")
nit: Blink Style Guide: Naming - Precede boolean values with words like “is” and “did”. Consider renaming `uma_recorded_` to `is_uma_recorded_` or `did_record_uma_`.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
nit: The label `CanvasHeuristicResult` should probably be changed to match the name of the enum.
Done
This class is big enough to be put in its own file. Also, you should probably give it a name that makes it clear that it is canvas-specific. Suggestion: HTMLCanvasAccessibilityManager
Done
accessibility_manager_->SetVisible();How come we never reset AccessibilityManager::is_visible_ to false when the canvas is not longer visible? I wouldhave expected this code to be
```
if (accessibility_manager_) {
accessibility_manager_->SetVisible(displayed_);
}
```
If there is a reason not to do this, it should be explained in a comment, because right now it looks like a bug.
Done,
The actual reason was (leaked from my subconscious) that I had not made up my mind about whether we want to switch back from "needs a11y support" to "doesn't need" or not. But now that I thought more about it, if we concluded (right or wrong) that it doesn't need a11y support, it's better to avoid unnecessary computations.
if (accessibility_manager_) {I don't think ContextDestroyed is the right place to put this code. The purpose of ContextDestroyed is to manage the mutual reference between the canvas element and the context because the object garbage collection order is non-deterministic. The accessibility manager does not interact with this reference cycle and is not impacted by the destruction order between canvas and context. Calling `FlushUmaIfNeeded` in the `Dispose` method (as you're already doing) should be sufficient.
Also, Why do we even need to call `FlushUmaIfNeeded` given that the destructor of AccesibilityManager already does it. If it's just to make unit tests work, you could instead teardown the document and trigger GC manually in your tests.
Part 1: You are correct, removed it.
Part 2: As far as I can see, the destructor isn't reliably called. I added logs in the constructor and descructor and the later is almost never seen. I don't know what garbage collection is doing behind the scenes, but I am even wondering if it's useful to be in the destructor or I would just remove it from there.
bool first_child_is_element = firstElementChild() != nullptr;The name of this variable does not correctly express what you're checking. Consider `has_child_element` or `has_fallback_element`
Done
ElementTraversal::FirstChild(*canvas_element) != nullptr;This replicates logic you added in `HTMLCanvasElement::ChildrenChanged`. Consider adding a `HasFallbackElementContent()` method to HTMLCanvasElement. That way if we ever need to modify how this is determined, we won't need to worry about tracking down all the places in the code that need updating.
Done
if (canvas_element->IsDisplayed()) {
SetVisible();
}Why not `SetVisible(canvas_element->IsDisplayed());`? Is there a reason _is_visible should never be reset to false?
Done
uma_timer_.StartOneShot(base::Seconds(1), FROM_HERE);Why is this delay necessary? and why one second?
Added a constant and a comment on top of the new file. One second is chosen arbitrarily. I have no idea about how to choose it based on knowledge.
I expect that most probably after the initial load of the page (and a11y is/gets enabled), the canvas is already either accessible or not, don't you think so?
Related details
- Justin Novosad
- Lucas Radaelli
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. |
Lucas Radaelli (Gerrit)
Lucas Radaelli added 10 comments![]( "Open in Gerrit")
void OnUpdate();nit: style guide defines methods before variables, so please move this up.
// its accessibility related information.nit: consider moving all this logic to the .cc
if (has_layoutsubtree_ == has_layoutsubtree) {nit: prefer to move these to the .cc
void ReadAriaAttributes(const HTMLCanvasElement* element);why do we need to pass by argument the html canvas element if it is received in the constructor?
HTMLCanvasElement* canvas_element);nit: please document lifecycle of this pointer. The html element needs to outlive this manager?
// canvas element, determines if the canvas element needs accessibility support,nit: is it one manager per canvas html element or a manager can hold multiple elements?
// Delay to ensure the canvas element has had a chance to update itsnit: add blank line between namespace and first comment.
nit: add the anonymousnamespace inside the blink one. namespace blink {namespace { } // namespace ...... ... } // namespace blink.
bool has_fallback_element_content =Now I understand why you are passing the element on methods, just document in the constructor (.h), that you are not holding a pointer to the canvas.
std::unique_ptr<HTMLCanvasAccessibilityManager> accessibility_manager_;nit: can we make the AXObject that represents the canvas hold the object, or even the AXObjectCache? Here, every canvas element will have a pointer / variable to this object, even when it is not used, and that will increase memory usage on the blink side.
Related details
- Justin Novosad
- Ramin Halavati
Ramin Halavati (Gerrit)
Ramin Halavati added 10 comments![]( "Open in Gerrit")
nit: style guide defines methods before variables, so please move this up.
Done
nit: consider moving all this logic to the .cc
Done
nit: prefer to move these to the .cc
Done
void ReadAriaAttributes(const HTMLCanvasElement* element);why do we need to pass by argument the html canvas element if it is received in the constructor?
I changed the code to keep the owner canvas elements, so the argument is not needed anymore.
nit: please document lifecycle of this pointer. The html element needs to outlive this manager?
Done
// canvas element, determines if the canvas element needs accessibility support,nit: is it one manager per canvas html element or a manager can hold multiple elements?
Just one, updated the comment.
// Delay to ensure the canvas element has had a chance to update itsnit: add blank line between namespace and first comment.
Done
nit: add the anonymousnamespace inside the blink one. namespace blink {namespace { } // namespace ...... ... } // namespace blink.
Done
Now I understand why you are passing the element on methods, just document in the constructor (.h), that you are not holding a pointer to the canvas.
Actually I changed it to the other way! I noted that I need the pointer to the canvas later at least for UKM collections, and it looks neater if the owner link is kept.
std::unique_ptr<HTMLCanvasAccessibilityManager> accessibility_manager_;nit: can we make the AXObject that represents the canvas hold the object, or even the AXObjectCache? Here, every canvas element will have a pointer / variable to this object, even when it is not used, and that will increase memory usage on the blink side.
`accessibility_manager_` exists per each canvas element, and is created only when AXObject is created and therefore as long as a11y is not enabled, it does not exist.
Keeping it at this level gives us easier access from Canvas element (for updates regarding rendering text, etc.), and to Canvas element for reading feature and sending requests like storing UKM.
Related details
- Justin Novosad
- Lucas Radaelli
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. |
Justin Novosad (Gerrit)
Justin Novosad added 3 comments![]( "Open in Gerrit")
accessibility_manager_->SetVisible();Ramin HalavatiHow come we never reset AccessibilityManager::is_visible_ to false when the canvas is not longer visible? I wouldhave expected this code to be
```
if (accessibility_manager_) {
accessibility_manager_->SetVisible(displayed_);
}
```
If there is a reason not to do this, it should be explained in a comment, because right now it looks like a bug.
Done,
The actual reason was (leaked from my subconscious) that I had not made up my mind about whether we want to switch back from "needs a11y support" to "doesn't need" or not. But now that I thought more about it, if we concluded (right or wrong) that it doesn't need a11y support, it's better to avoid unnecessary computations.
Acknowledged
if (accessibility_manager_) {Ramin HalavatiI don't think ContextDestroyed is the right place to put this code. The purpose of ContextDestroyed is to manage the mutual reference between the canvas element and the context because the object garbage collection order is non-deterministic. The accessibility manager does not interact with this reference cycle and is not impacted by the destruction order between canvas and context. Calling `FlushUmaIfNeeded` in the `Dispose` method (as you're already doing) should be sufficient.
Also, Why do we even need to call `FlushUmaIfNeeded` given that the destructor of AccesibilityManager already does it. If it's just to make unit tests work, you could instead teardown the document and trigger GC manually in your tests.
Part 1: You are correct, removed it.
Part 2: As far as I can see, the destructor isn't reliably called. I added logs in the constructor and descructor and the later is almost never seen. I don't know what garbage collection is doing behind the scenes, but I am even wondering if it's useful to be in the destructor or I would just remove it from there.
The destructor does not get called because this is a garbage collected object. This class does however register a prefinalizer (i.e. the `Dispose` method), which oilpan will call prior to de-allocation. This `Dispose` method is where you'd want to call `FlushUmaIfNeeded`. To trigger immediate finalization consistently in a test, you need to remove all references to the object, for example either tear down the document or remove the canvas to the DOM and set all variables that point the canvas to `nullptr`. Then you can manually force an immediate GC by calling `ThreadState::Current()->CollectAllGarbageForTesting()`. This should consistently result in HTMLCanvasElement::Dispose() being called.
uma_timer_.StartOneShot(base::Seconds(1), FROM_HERE);Ramin HalavatiWhy is this delay necessary? and why one second?
Added a constant and a comment on top of the new file. One second is chosen arbitrarily. I have no idea about how to choose it based on knowledge.
I expect that most probably after the initial load of the page (and a11y is/gets enabled), the canvas is already either accessible or not, don't you think so?
I still don't understand why the delay is necessary. Is it because SetHeuristicResult gets called multiple times and you only want to capture the last value it is set to? If that is the case, I think this way of doing thing adds unnecessary complexity. Perhaps you could just record a histogram sample in `Dispose`.
Related details
- Justin Novosad
- Lucas Radaelli
- Ramin Halavati
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. |
Lucas Radaelli (Gerrit)
Lucas Radaelli added 1 comment![]( "Open in Gerrit")
std::unique_ptr<HTMLCanvasAccessibilityManager> accessibility_manager_;Ramin Halavatinit: can we make the AXObject that represents the canvas hold the object, or even the AXObjectCache? Here, every canvas element will have a pointer / variable to this object, even when it is not used, and that will increase memory usage on the blink side.
`accessibility_manager_` exists per each canvas element, and is created only when AXObject is created and therefore as long as a11y is not enabled, it does not exist.
Keeping it at this level gives us easier access from Canvas element (for updates regarding rendering text, etc.), and to Canvas element for reading feature and sending requests like storing UKM.
but doesn't that mean that every time a canvas element is initiated on a page, even though a11y is not on, the object still holds a pointer to it, wasting memory?
Related details
- Justin Novosad
- Ramin Halavati
Ramin Halavati (Gerrit)
Ramin Halavati added 3 comments![]( "Open in Gerrit")
std::unique_ptr<HTMLCanvasAccessibilityManager> accessibility_manager_;Ramin Halavatinit: can we make the AXObject that represents the canvas hold the object, or even the AXObjectCache? Here, every canvas element will have a pointer / variable to this object, even when it is not used, and that will increase memory usage on the blink side.
Lucas Radaelli`accessibility_manager_` exists per each canvas element, and is created only when AXObject is created and therefore as long as a11y is not enabled, it does not exist.
Keeping it at this level gives us easier access from Canvas element (for updates regarding rendering text, etc.), and to Canvas element for reading feature and sending requests like storing UKM.
but doesn't that mean that every time a canvas element is initiated on a page, even though a11y is not on, the object still holds a pointer to it, wasting memory?
You are right, each canvas will have an uninitialized `Member<HTMLCanvasAccessibilityManager>` object which takes 4-8 bytes.
However moving ownership of this object to AXObject would require a lot of boilerplate code to get it when it is needed in the canvas object (to pass data and events). Also note that due to the layering of Blink, it would be quite expensive (in terms of code) to move the class to `modules/accessibility` and make it available by an object in `core`.
I can create a CL to showcase it if you insist, but I think considering all these required boilerplate, the current solution is easier to read and maybe even more compact in memory.
We can reconsider this decision later when the code is in a stable state and optimizations can be done with fewer considerations.
if (accessibility_manager_) {Ramin HalavatiI don't think ContextDestroyed is the right place to put this code. The purpose of ContextDestroyed is to manage the mutual reference between the canvas element and the context because the object garbage collection order is non-deterministic. The accessibility manager does not interact with this reference cycle and is not impacted by the destruction order between canvas and context. Calling `FlushUmaIfNeeded` in the `Dispose` method (as you're already doing) should be sufficient.
Also, Why do we even need to call `FlushUmaIfNeeded` given that the destructor of AccesibilityManager already does it. If it's just to make unit tests work, you could instead teardown the document and trigger GC manually in your tests.
Justin NovosadPart 1: You are correct, removed it.
Part 2: As far as I can see, the destructor isn't reliably called. I added logs in the constructor and descructor and the later is almost never seen. I don't know what garbage collection is doing behind the scenes, but I am even wondering if it's useful to be in the destructor or I would just remove it from there.
The destructor does not get called because this is a garbage collected object. This class does however register a prefinalizer (i.e. the `Dispose` method), which oilpan will call prior to de-allocation. This `Dispose` method is where you'd want to call `FlushUmaIfNeeded`. To trigger immediate finalization consistently in a test, you need to remove all references to the object, for example either tear down the document or remove the canvas to the DOM and set all variables that point the canvas to `nullptr`. Then you can manually force an immediate GC by calling `ThreadState::Current()->CollectAllGarbageForTesting()`. This should consistently result in HTMLCanvasElement::Dispose() being called.
Please let me know to schedule a quick call if this is becoming frustrating or I am not getting something here.
A few points before the actual reply:
1- I have not added any of the mechanism here to make tests pass, it's just based on my observation on how things run (on the surface).
2- Frankly I am not sure how often the heuristic states change on a canvas and this UMA is just to give us a rough estimate of how much different cases happen. I have added a timer based on the assumption that if it doesn't change for 1s, that is good enough to collect.
Gemini believes (and it matches what I observe) that using garbage collection pre-finalizers is an anti-pattern for important tasks because GC timing is non-deterministic and skipped on shutdown. It says if there would be no memory pressure, GC might be delayed until actual browser shutdown and at that time Chromium performs a fast shutdown which will result in renderer process termination and skipping final garbage collection.
It's recommended preferred solution (implied in the class comment of ExecutionContextLifecycleObserver) is to do it in `ContextDestroyed`.
My personal observations (based on LOGs) also show that the only reliable call is in `HTMLCanvasElement::ContextDestroyed()` (which is only skipped if browser shuts down while the tab is still on. So my preference is still `ContextDestroyed` + timer and if timer looks bad, just `ContextDestroyed`.
WDYT?
uma_timer_.StartOneShot(base::Seconds(1), FROM_HERE);Ramin HalavatiWhy is this delay necessary? and why one second?
Justin NovosadAdded a constant and a comment on top of the new file. One second is chosen arbitrarily. I have no idea about how to choose it based on knowledge.
I expect that most probably after the initial load of the page (and a11y is/gets enabled), the canvas is already either accessible or not, don't you think so?
I still don't understand why the delay is necessary. Is it because SetHeuristicResult gets called multiple times and you only want to capture the last value it is set to? If that is the case, I think this way of doing thing adds unnecessary complexity. Perhaps you could just record a histogram sample in `Dispose`.
Replied in more details on the other comment. This metric collected just as a best-effort, so it doesn't matter that much if it is not really the last value, and a stable value is good enough. (I will update metric's description if we proceed with the timer.)
Related details
- Justin Novosad
- Justin Novosad
- Lucas Radaelli
Justin Novosad (Gerrit)
Justin Novosad added 2 comments![]( "Open in Gerrit")
if (accessibility_manager_) {Ramin HalavatiI don't think ContextDestroyed is the right place to put this code. The purpose of ContextDestroyed is to manage the mutual reference between the canvas element and the context because the object garbage collection order is non-deterministic. The accessibility manager does not interact with this reference cycle and is not impacted by the destruction order between canvas and context. Calling `FlushUmaIfNeeded` in the `Dispose` method (as you're already doing) should be sufficient.
Also, Why do we even need to call `FlushUmaIfNeeded` given that the destructor of AccesibilityManager already does it. If it's just to make unit tests work, you could instead teardown the document and trigger GC manually in your tests.
Justin NovosadPart 1: You are correct, removed it.
Part 2: As far as I can see, the destructor isn't reliably called. I added logs in the constructor and descructor and the later is almost never seen. I don't know what garbage collection is doing behind the scenes, but I am even wondering if it's useful to be in the destructor or I would just remove it from there.
Ramin HalavatiThe destructor does not get called because this is a garbage collected object. This class does however register a prefinalizer (i.e. the `Dispose` method), which oilpan will call prior to de-allocation. This `Dispose` method is where you'd want to call `FlushUmaIfNeeded`. To trigger immediate finalization consistently in a test, you need to remove all references to the object, for example either tear down the document or remove the canvas to the DOM and set all variables that point the canvas to `nullptr`. Then you can manually force an immediate GC by calling `ThreadState::Current()->CollectAllGarbageForTesting()`. This should consistently result in HTMLCanvasElement::Dispose() being called.
Please let me know to schedule a quick call if this is becoming frustrating or I am not getting something here.
A few points before the actual reply:
1- I have not added any of the mechanism here to make tests pass, it's just based on my observation on how things run (on the surface).
2- Frankly I am not sure how often the heuristic states change on a canvas and this UMA is just to give us a rough estimate of how much different cases happen. I have added a timer based on the assumption that if it doesn't change for 1s, that is good enough to collect.Gemini believes (and it matches what I observe) that using garbage collection pre-finalizers is an anti-pattern for important tasks because GC timing is non-deterministic and skipped on shutdown. It says if there would be no memory pressure, GC might be delayed until actual browser shutdown and at that time Chromium performs a fast shutdown which will result in renderer process termination and skipping final garbage collection.
It's recommended preferred solution (implied in the class comment of ExecutionContextLifecycleObserver) is to do it in `ContextDestroyed`.My personal observations (based on LOGs) also show that the only reliable call is in `HTMLCanvasElement::ContextDestroyed()` (which is only skipped if browser shuts down while the tab is still on. So my preference is still `ContextDestroyed` + timer and if timer looks bad, just `ContextDestroyed`.
WDYT?
Thanks for the explanations. This makes sense. Please add a few more comments in the code to explain theses things.
uma_timer_.StartOneShot(base::Seconds(1), FROM_HERE);Ramin HalavatiWhy is this delay necessary? and why one second?
Justin NovosadAdded a constant and a comment on top of the new file. One second is chosen arbitrarily. I have no idea about how to choose it based on knowledge.
I expect that most probably after the initial load of the page (and a11y is/gets enabled), the canvas is already either accessible or not, don't you think so?
Ramin HalavatiI still don't understand why the delay is necessary. Is it because SetHeuristicResult gets called multiple times and you only want to capture the last value it is set to? If that is the case, I think this way of doing thing adds unnecessary complexity. Perhaps you could just record a histogram sample in `Dispose`.
Replied in more details on the other comment. This metric collected just as a best-effort, so it doesn't matter that much if it is not really the last value, and a stable value is good enough. (I will update metric's description if we proceed with the timer.)
- Justin Novosad
- Lucas Radaelli
- Ramin Halavati
Justin Novosad (Gerrit)
Justin Novosad voted Code-Review+1![]( "Open in Gerrit")
- Lucas Radaelli
- Ramin Halavati
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. |
Ramin Halavati (Gerrit)
Ramin Halavati added 1 comment![]( "Open in Gerrit")
if (accessibility_manager_) {Ramin HalavatiI don't think ContextDestroyed is the right place to put this code. The purpose of ContextDestroyed is to manage the mutual reference between the canvas element and the context because the object garbage collection order is non-deterministic. The accessibility manager does not interact with this reference cycle and is not impacted by the destruction order between canvas and context. Calling `FlushUmaIfNeeded` in the `Dispose` method (as you're already doing) should be sufficient.
Also, Why do we even need to call `FlushUmaIfNeeded` given that the destructor of AccesibilityManager already does it. If it's just to make unit tests work, you could instead teardown the document and trigger GC manually in your tests.
Justin NovosadPart 1: You are correct, removed it.
Part 2: As far as I can see, the destructor isn't reliably called. I added logs in the constructor and descructor and the later is almost never seen. I don't know what garbage collection is doing behind the scenes, but I am even wondering if it's useful to be in the destructor or I would just remove it from there.
Ramin HalavatiThe destructor does not get called because this is a garbage collected object. This class does however register a prefinalizer (i.e. the `Dispose` method), which oilpan will call prior to de-allocation. This `Dispose` method is where you'd want to call `FlushUmaIfNeeded`. To trigger immediate finalization consistently in a test, you need to remove all references to the object, for example either tear down the document or remove the canvas to the DOM and set all variables that point the canvas to `nullptr`. Then you can manually force an immediate GC by calling `ThreadState::Current()->CollectAllGarbageForTesting()`. This should consistently result in HTMLCanvasElement::Dispose() being called.
Justin NovosadPlease let me know to schedule a quick call if this is becoming frustrating or I am not getting something here.
A few points before the actual reply:
1- I have not added any of the mechanism here to make tests pass, it's just based on my observation on how things run (on the surface).
2- Frankly I am not sure how often the heuristic states change on a canvas and this UMA is just to give us a rough estimate of how much different cases happen. I have added a timer based on the assumption that if it doesn't change for 1s, that is good enough to collect.Gemini believes (and it matches what I observe) that using garbage collection pre-finalizers is an anti-pattern for important tasks because GC timing is non-deterministic and skipped on shutdown. It says if there would be no memory pressure, GC might be delayed until actual browser shutdown and at that time Chromium performs a fast shutdown which will result in renderer process termination and skipping final garbage collection.
It's recommended preferred solution (implied in the class comment of ExecutionContextLifecycleObserver) is to do it in `ContextDestroyed`.My personal observations (based on LOGs) also show that the only reliable call is in `HTMLCanvasElement::ContextDestroyed()` (which is only skipped if browser shuts down while the tab is still on. So my preference is still `ContextDestroyed` + timer and if timer looks bad, just `ContextDestroyed`.
WDYT?
Thanks for the explanations. This makes sense. Please add a few more comments in the code to explain theses things.
- Evan Liu
- Lucas Radaelli
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. |
Evan Liu (Gerrit)
Evan Liu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Evan Liu
| Code-Review | +1 |
1 comment
enum class HeuristicResult {nit: this should be prefixed with the comment
// These values are persisted to logs. Entries should not be renumbered and
// numeric values should never be reused.
https://chromium.googlesource.com/chromium/src/tools/+/HEAD/metrics/histograms/README.md
Related details
- Lucas Radaelli
- Ramin Halavati
Code-Coverage
Code-Owners