Fix reentrancy crash when hint popover closing mutates auto ancestor [chromium/src : main]
Mason Freed (Gerrit)
Mason Freed voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Auto-Submit | +1 |
3 comments
When showing an `auto` or `hint` popover, `CloseEntirePopoverStack`Mason FreedIs this specific to `CloseEntirePopoverStack`, or does it also apply to `HideAllPopoversUntil`?
I've reworded the commit description. LMK what you think.
"the popover.");Mason Freedmaybe "while hiding all hint popovers"?
(The existing message below also seems like it could be improved.)
Done
if (auto* auto_ancestor =Mason FreedIt seems a bit weird that the handling for the "auto popover" case and the error handling for the "hint popover with an ancestor in the auto stack" case are different, given that the underlying behavior (close the whole hint stack and part of the auto stack) seems to be the same.
Is there a reason you didn't want the same (simpler, I think) fix in this branch?
Hmm, yes I agree this is a bit weird. And still broken. I've attempted to merge these and add a test case proving the problem. LMK if that's what you were going for.
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 voted and added 3 comments![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
3 comments
append_to_stack = &auto_stack;So you're not changing anything here -- but I'm a little confused as to why (both before and after this change) you sometimes append hint popovers to the auto stack. Maybe that at least deserves a comment?
if (auto* auto_ancestor =Mason FreedIt seems a bit weird that the handling for the "auto popover" case and the error handling for the "hint popover with an ancestor in the auto stack" case are different, given that the underlying behavior (close the whole hint stack and part of the auto stack) seems to be the same.
Is there a reason you didn't want the same (simpler, I think) fix in this branch?
Hmm, yes I agree this is a bit weird. And still broken. I've attempted to merge these and add a test case proving the problem. LMK if that's what you were going for.
ok, I didn't realize the difference actually *mattered*, I just throught it was odd.
Related details
- Mason Freed
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. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
append_to_stack = &auto_stack;So you're not changing anything here -- but I'm a little confused as to why (both before and after this change) you sometimes append hint popovers to the auto stack. Maybe that at least deserves a comment?
Yeah, that's the subject of a long (and only recent) discussion:
https://github.com/whatwg/html/issues/12304#issuecomment-4173002767
I'm loath to add a comment pointing to that in the codebase. But I also don't have a great rationale that stands up to scrutiny. This behavior is likely to be changing soon.
LMK what you think I should do here.
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. |