Pass heap snapshot source locations through blink::WebNode (and subclasses), autofill::FormRef, and autofill::FieldRef. [chromium/src : main]
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
I'm curious whether you think these changes are reasonable. They help with debugging of memory leaks that go through `WebNode` (which creates a `Persistent<>`).
Some of the choices here about dependencies versus maintainability (non-duplication) versus readability weren't completely obvious to me, and I think the current set of choices in the CL is actually somewhat inconsistent but still possibly reasonable. In particular, I chose to avoid duplicating the definition of the `VERBOSE_PERSISTENT` buildflag itself and instead moving the single definition to blink/public, but then chose to duplicate the code that uses that flag as `BLINK_WEB_NODE_LOCATION_FROM_HERE` (which is basically the same as the existing `PERSISTENT_LOCATION_FROM_HERE`).
It's also not clear to me if I'm breaking any dependency rules here.
(I'll add an additional reviewer for the `autocomplete` side if you're ok with them.)
Related details
- Daniel Cheng
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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
+dom@ to help with the public API review
(Sorry for not getting to this earlier; Mondays I'm usually not in until later)
Related details
- David Baron
- Dominic Farolino
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. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
fwiw, I have created this kind of patch locally a few times (over the course of a years). I've held back of upstreaming because I thought it's only useful for myself but clearly I was wrong there.
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Code-Review | +1 |
1 comment
blink public lgtm, but I have to admit if it were broken in some super subtle way, I'm not sure I'd catch it, as the cppgc integration is a little out of my wheelhouse. but it looks right. off to dcheng@
Related details
- David Baron
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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
1. I don't feel strongly against this, but can we add some comments/documentation somewhere about when it makes sense to use the `FROM_HERE` macro?
2. Why does autofill need these? It feels a bit weird for this to bubble up outside the Blink public API, since I would have expected this sort of thing to be mostly internal to `WebNode` itself.
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
1. I don't feel strongly against this, but can we add some comments/documentation somewhere about when it makes sense to use the `FROM_HERE` macro?
2. Why does autofill need these? It feels a bit weird for this to bubble up outside the Blink public API, since I would have expected this sort of thing to be mostly internal to `WebNode` itself.
1. I've extended the comment above the `BLINK_WEB_NODE_FROM_HERE` macro in a way that hopefully answers your questions.
2. My comment for (1) answers the question of why I did it -- but it's certainly not the only choice. Other options include (probably in order of my personal preference, best to worst):
* moving to just one version of this rather than two (getting rid of `PERSISTENT_LOCATION_FROM_HERE` that this duplicates) and making it a little more clearly public
* not bothering optimizing the code size for autofill
* having a third version for autofill
Related details
- Daniel Cheng
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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
// WebNode and of any classes that derive from it or use it that want toPresumably this is referring to `WebNode` and `WebNode`-derived classes, but i found this a little hard to parse.
To better help me understand... why do classes that use `WebNode` also need to use this? Why wouldn't the location supplied by embedding a `blink::WebFormElement` as a field already point to the right thing?
Related details
- David Baron
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 Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
// WebNode and of any classes that derive from it or use it that want toPresumably this is referring to `WebNode` and `WebNode`-derived classes, but i found this a little hard to parse.
To better help me understand... why do classes that use `WebNode` also need to use this? Why wouldn't the location supplied by embedding a `blink::WebFormElement` as a field already point to the right thing?
So if I make the `web_node.h` changes an no others, then the heap dump for the bug I was investigating will say that there is a "C++ persistent root" associated with the source code location of `WebElement::WebElement`, since it's the `WebElement` constructor that calls the `WebNode` constructor. To fix that, I make the `WebElement` constructor pass through the location of *its* caller. At that point, the heap dump will say that the persistent root comes from the `WebFormElement` constructor, so I need to make *that* code pass through the location of *its* caller. etc.
Related details
- Daniel Cheng
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. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
// WebNode and of any classes that derive from it or use it that want toDavid BaronPresumably this is referring to `WebNode` and `WebNode`-derived classes, but i found this a little hard to parse.
To better help me understand... why do classes that use `WebNode` also need to use this? Why wouldn't the location supplied by embedding a `blink::WebFormElement` as a field already point to the right thing?
So if I make the `web_node.h` changes an no others, then the heap dump for the bug I was investigating will say that there is a "C++ persistent root" associated with the source code location of `WebElement::WebElement`, since it's the `WebElement` constructor that calls the `WebNode` constructor. To fix that, I make the `WebElement` constructor pass through the location of *its* caller. At that point, the heap dump will say that the persistent root comes from the `WebFormElement` constructor, so I need to make *that* code pass through the location of *its* caller. etc.
Down the line all this depends on `__builtin_source_location` which has the idea that it's used as default argument for methods to capture the _caller_ source location. So you need to wire it from the top or otherwise you just see the WebNode ctor (file and line numbers).
Bit annoying but the best we have so far.
Related details
- Daniel Cheng
- David Baron
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. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
2 comments
// WebNode and of any classes that derive from it or use it that want toDavid BaronPresumably this is referring to `WebNode` and `WebNode`-derived classes, but i found this a little hard to parse.
To better help me understand... why do classes that use `WebNode` also need to use this? Why wouldn't the location supplied by embedding a `blink::WebFormElement` as a field already point to the right thing?
Michael LippautzSo if I make the `web_node.h` changes an no others, then the heap dump for the bug I was investigating will say that there is a "C++ persistent root" associated with the source code location of `WebElement::WebElement`, since it's the `WebElement` constructor that calls the `WebNode` constructor. To fix that, I make the `WebElement` constructor pass through the location of *its* caller. At that point, the heap dump will say that the persistent root comes from the `WebFormElement` constructor, so I need to make *that* code pass through the location of *its* caller. etc.
Down the line all this depends on `__builtin_source_location` which has the idea that it's used as default argument for methods to capture the _caller_ source location. So you need to wire it from the top or otherwise you just see the WebNode ctor (file and line numbers).
Bit annoying but the best we have so far.
- David Baron
- Michael Lippautz
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. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Owners-Override | +1 |
1 comment
(I think it's OK to use OO here)
Related details
- David Baron
- Michael Lippautz
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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
David Baron1. I don't feel strongly against this, but can we add some comments/documentation somewhere about when it makes sense to use the `FROM_HERE` macro?
2. Why does autofill need these? It feels a bit weird for this to bubble up outside the Blink public API, since I would have expected this sort of thing to be mostly internal to `WebNode` itself.
1. I've extended the comment above the `BLINK_WEB_NODE_FROM_HERE` macro in a way that hopefully answers your questions.
2. My comment for (1) answers the question of why I did it -- but it's certainly not the only choice. Other options include (probably in order of my personal preference, best to worst):
* moving to just one version of this rather than two (getting rid of `PERSISTENT_LOCATION_FROM_HERE` that this duplicates) and making it a little more clearly public
* not bothering optimizing the code size for autofill
* having a third version for autofill
Acknowledged
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
I'm curious whether you think these changes are reasonable. They help with debugging of memory leaks that go through `WebNode` (which creates a `Persistent<>`).
Some of the choices here about dependencies versus maintainability (non-duplication) versus readability weren't completely obvious to me, and I think the current set of choices in the CL is actually somewhat inconsistent but still possibly reasonable. In particular, I chose to avoid duplicating the definition of the `VERBOSE_PERSISTENT` buildflag itself and instead moving the single definition to blink/public, but then chose to duplicate the code that uses that flag as `BLINK_WEB_NODE_LOCATION_FROM_HERE` (which is basically the same as the existing `PERSISTENT_LOCATION_FROM_HERE`).
It's also not clear to me if I'm breaking any dependency rules here.
(I'll add an additional reviewer for the `autocomplete` side if you're ok with them.)
Acknowledged
Related details
- David Baron
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
I wanted to make sure an autofill owner (schwering) is ok with the autofill changes as well.
Related details
- Christoph Schwering
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering voted Code-Review+1![]( "Open in Gerrit")
- David Baron
- Michael Lippautz
David Baron (Gerrit)
David Baron voted Commit-Queue+2![]( "Open in Gerrit")
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Michael Lippautz
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
4 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Pass heap snapshot source locations through blink::WebNode (and subclasses), autofill::FormRef, and autofill::FieldRef.
- M components/autofill/content/renderer/autofill_agent.cc
- M components/autofill/content/renderer/form_tracker.cc
- M components/autofill/content/renderer/form_tracker.h
- M third_party/blink/public/BUILD.gn
- M third_party/blink/public/web/web_document.h
- M third_party/blink/public/web/web_element.h
- M third_party/blink/public/web/web_form_control_element.h
- M third_party/blink/public/web/web_form_element.h
- M third_party/blink/public/web/web_input_element.h
- M third_party/blink/public/web/web_label_element.h
- M third_party/blink/public/web/web_meta_element.h
- M third_party/blink/public/web/web_node.h
- M third_party/blink/public/web/web_option_element.h
- M third_party/blink/public/web/web_select_element.h
- M third_party/blink/renderer/core/exported/web_node.cc
- M third_party/blink/renderer/platform/heap/BUILD.gn
- M third_party/blink/renderer/platform/heap/cross_thread_handle_internal.h
- M third_party/blink/renderer/platform/heap/persistent.h
- M third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
Code-Review: +1 by Daniel Cheng, +1 by Dominic Farolino, +1 by Christoph Schwering
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Tan (Gerrit)
Victor Tan 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. |