I decided to switch strategies mid-fix after a discussion with@markshannon this morning: instead of hacking the frame to look "incomplete" just before teardown, I think unlinking the framebefore clearing it (instead of after) is a better approach that is easier to reason about and keeps extra code out of the hot path when popping frames.

This is going to need a manual backport to 3.11. I'll start on it now to hopefully unblock releases.

CC:@pablogsal

Issue:Frame teardown can create frame objects #99729

brandtbucher added4 commits

November 26, 2022 03:55

Add failing regression test

5070ff9

Unlink frames before clearing them

ca87470

blurb add

6210626

Require working threading for new test

af155c7

brandtbucher added interpreter-core

(Objects, Python, Grammar, and Parser dirs)

type-crash

A hard crash of the interpreter, possibly with a core dump

needs backport to 3.11only security fixes labels

Dec 5, 2022

brandtbucher requested a review frommarkshannon as acode owner

December 5, 2022 20:37

brandtbucher self-assigned this

Dec 5, 2022

bedevere-bot added the awaiting core review label

Dec 5, 2022

brandtbucher changed the title~~Frame teardown~~GH-99729: Unlink frames before clearing them

Dec 5, 2022

bedevere-bot mentioned this pull request

Dec 5, 2022

Frame teardown can create frame objects#99729

Closed

Copy link

MemberAuthor

brandtbucher commentedDec 5, 2022

Not sure if we want buildbots on this (normally I would run them, but I also don't want to further delay the releases with a bunch of extra CI checks). I'm confident in the fix, though.

pablogsal added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Dec 5, 2022

Copy link

bedevere-bot commentedDec 5, 2022

🤖 New build scheduled with the buildbot fleet by@pablogsal for commit6210626 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Dec 5, 2022

Copy link

Member

pablogsal commentedDec 5, 2022

Not sure if we want buildbots on this (normally I would run them, but I also don't want to further delay the releases with a bunch of extra CI checks). I'm confident in the fix, though.

Thanks a lot for fixing this ❤️

I am executing the buildbots at least to see the quick ones to double check there is nothing unexpected. I will manually test Refleaks after before the release.

Copy link

MemberAuthor

brandtbucher commentedDec 5, 2022

Hm, looks like we need to skip the new test on webassembly builds.

brandtbucher added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Dec 5, 2022

Copy link

bedevere-bot commentedDec 5, 2022

🤖 New build scheduled with the buildbot fleet by@brandtbucher for commitaf155c7 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Dec 5, 2022

brandtbucher mentioned this pull request

Dec 6, 2022

[3.11] GH-99729: Unlink frames before clearing them#100047

Merged

Copy link

Member

markshannon commentedDec 6, 2022

Looks good. I think the buildbot failures are unrelated.

pablogsal merged commitb72014c intopython:main

Dec 6, 2022

Copy link

Contributor

miss-islington commentedDec 6, 2022

Thanks@brandtbucher for the PR, and@pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

bedevere-bot removed the awaiting core review label

Dec 6, 2022

Copy link

Contributor

miss-islington commentedDec 6, 2022

Sorry,@brandtbucher and@pablogsal, I could not cleanly backport this to3.11 due to a conflict.
Please backport usingcherry_picker on command line.
cherry_picker b72014c783e5698beb18ee1249597e510b8bcb5a 3.11