IssueTracker

Thanks for the detailed analysis and sample. I've prepared a CL, but I'd also be happy to review an external contribution since you already went through the trouble of identifying a fix. Just let me know here.

If you have a CL ready go for it.

That being said, I just went down git history and found when this was introduced: 8d4765883ff24f0f79bf64fea23d4687d50cec85 (and the null case was added in 740e50456ff7aa7b93d40e7bdc59f8a619197ed5.

As you can see, this was introduced to fix a "memory leak" though I don't quite understand why creating a new callback wrapper for every setSupportActionBar() call is a problem.

I vaguely remember this. The issue was iirc:

• You use multiple fragments (say switched via tabs). Each fragment calls setSupportActionBar() when visible.
• The old code would wrap the current Window.Callback with something from ToolbarActionBar (TAB), then set it as the window callback.
• When the user switches to Tab #2, we'd do the same thing, but this time the callback wrapper chain would be (((Root callback) TBAB1Wrapper) TBAB2Wrapper). TBAB1Wrapper would keep a reference a reference to the views even though the #1 fragment has likely been destroyed.

I've just +2'd Alan's CL, but the proper fix is properly to unwrap the callback from Window.getCallback().

Let me uh... let me go back and properly fix that.

One thing though, you can't quite unwrap because there's no guarantee you'll get the same callback (e.g. it might have been wrapped again).

It seems like what we want here is:

1. ensure the callback is set at least once
2. get a reference to it later

Is there a way the internal code can keep a ref to that new callback once its set, and reference it instead of installing or unwrap?

there's no guarantee you'll get the same callback (e.g. it might have been wrapped again).

Do you mean the app code may have wrapped the callback?

Yep. That's what got this issue filed after all ;-)

Heh, of course.

Is there a way the internal code can keep a ref to that new callback once its set, and reference it instead of installing or unwrap?

This goes beyond my familiarity with (and available time to invest in understanding) the existing codebase -- I'd have to defer to Chris.

I don't think there's a perfect solution here to be honest. I'm thinking that reverting to the previous solution is the only 'correct' way forward for the 80% case.

If apps want to change the window callback, they can override setSupportActionBar() and do it in there, after calling super.