Fixed

Bug

Status Update

No update yet.

Description

py...@gmail.com

created issue #1

Apr 30, 2021 06:11PM

Component used: AndroidX AppCompat
Version used: latest, 1.3.0-rc01 (all versions really)
Devices/Android versions reproduced on: All Android versions, all devices.

Summary

Calling AppCompatActivity.setSupportActionBar() replaces the window callback with a delegate callback that delegates to a previously installed AppCompatWindowCallback instead of delegating to the current window callback, thus breaking developers expectations around window callback delegation.

Root cause

Every android.view.Window instance can have a single android.view.Window.Callback set (via Window.setCallback). The correct pattern to support multiple callbacks is to replace the window callback and then delegate to the previously set callback. This correct pattern happens when calling AppCompatActivity.onCreate(), as we can see in AppCompatDelegateImpl.attachToWindow().
AppCompatActivity.setSupportActionBar() does not follow that correct pattern. It replaces the callback but then delegates to the previously installed AppCompatWindowCallback instead of the current window callback, as we can see in AppCompatDelegateImpl.setSupportActionBar().

Repro

This bug was originally reported square/curtains#14. After investigating, I determined this is actually an AppCompat bug. I'm attaching the repro project and video demonstrating the bug from that ticket.

Here's an example activity that reproduces the issue:

package com.lb.recordappsample

import android.os.Bundle
import android.util.Log
import android.view.MotionEvent
import android.view.Window
import androidx.appcompat.app.AppCompatActivity

class MainActivity : AppCompatActivity() {

  override fun onCreate(savedInstanceState: Bundle?) {
    super.onCreate(savedInstanceState)
    setContentView(R.layout.activity_main)

    // At this point, window.callback returns AppCompatWindowCallback which delegates
    // to Activity.

    val previousCallback = window.callback

    window.callback = object : Window.Callback by previousCallback {
      override fun dispatchTouchEvent(event: MotionEvent?): Boolean {
        Log.d("MainActivity", "Touch: $event")
        return previousCallback.dispatchTouchEvent(event)
      }
    }

    // At this point, window.callback returns our custom callback which delegates to
    // AppCompatWindowCallback which delegates to Activity.

    setSupportActionBar(findViewById(R.id.toolbar))

    // At this point, window.callback returns ToolbarActionBar.ToolbarCallbackWrapper
    // which delegates to AppCompatWindowCallback which delegates to Activity. Our
    // custom callback is not in the delegate chain any more! That's the bug.
  }
}

The fix is likely that AppCompatDelegateImpl.setSupportActionBar() should pass mWindow.getCallback() to the ToolbarActionBar constructor, instead of mAppCompatWindowCallback.

no logs.mp4

1.7 MB

Download

RecordAppSample Bug.zip

145 KB

Download

Comments

al...@google.com <al...@google.com> #2Apr 30, 2021 06:18PM

Accepted by al...@google.com.

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

py...@gmail.com <py...@gmail.com> #3Apr 30, 2021 06:35PM

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.

ap...@google.com <ap...@google.com> #4May 4, 2021 08:25AM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-main

commit 200ccecbc8c7c5ba555e7fa15890edff67affb58
Author: Alan Viverette <alanv@google.com>
Date: Fri Apr 30 14:55:07 2021

Fix case where setSupportActionBar overwrites the window callback

RelNote: """Fix scenario where calling setSupportActionBar after setting the
window callback would overwrite the callback."""
Test: SupportActionBarTestCase
Fixes: 186791590
Change-Id: Ie43eedf8322caa44e7b201a95cbc64197953e020

A appcompat/appcompat/src/androidTest/java/androidx/appcompat/app/SupportActionBarTestCase.kt
M appcompat/appcompat/src/main/java/androidx/appcompat/app/AppCompatDelegateImpl.java

https://android-review.googlesource.com/1692567

lb...@gmail.com <lb...@gmail.com> #5May 4, 2021 08:26AM

@4 May all bug be fixed this quick :)

ch...@gmail.com <ch...@gmail.com> #6May 4, 2021 08:33AM

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().

al...@google.com <al...@google.com> #7May 4, 2021 01:48PM

Accepted by al...@google.com.

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

py...@gmail.com <py...@gmail.com> #8May 4, 2021 04:14PM

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:

ensure the callback is set at least once
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?

al...@google.com <al...@google.com> #9May 4, 2021 04:33PM

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?

py...@gmail.com <py...@gmail.com> #10May 4, 2021 05:13PM

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

al...@google.com <al...@google.com> #11May 4, 2021 05:20PM

Assigned to ch...@google.com.

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.

ch...@google.com <ch...@google.com> #12May 5, 2021 12:59PM

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.

py...@gmail.com <py...@gmail.com> #13May 5, 2021 01:06PM

That won't work for libraries. The callback delegation allows libraries to play nice together. We shouldn't one library (appcompat) break all othee libraries.

py...@gmail.com <py...@gmail.com> #14May 5, 2021 01:17PM

Looking into this a bit more, it seems that what we really need to do is:

Install the callback only once and keep a ref to it.
Decouple the callback from ToolbarActionBar, so that we can swap the ToolbarActionBar instance receiving things (or set it to null)

Maybe 1) and 2) means actually killing the callback and adding a nullable ToolbarActionBar to the default callback already install? That'd simplify things a lot it seems.

Thoughts? I can maybe take a stab in between 2 diaper changes.

ch...@google.com <ch...@google.com> #15May 5, 2021 01:18PM

Yeah, that should be do-able. Looking now.

ch...@google.com <ch...@google.com> #16May 5, 2021 02:42PM

https://android-review.googlesource.com/c/platform/frameworks/support/+/1696341

ap...@google.com <ap...@google.com> #17May 6, 2021 01:21PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-main

commit 32144c85d339f680dbffd6ce9d16d57d15cdd65b
Author: Chris Banes <chrisbanes@google.com>
Date: Thu May 06 12:07:14 2021

Re-work support action bar window callback handling

Instead of wrapping the Window.Callback when an action bar
is set, we now just updated our existing callback so
that it receives the necessary events.

Follow-on from Ie43eedf8322caa44e7b201a95cbc64197953e020.

Test: SupportActionBarTestCase
Fixes: 186791590
Change-Id: I091cb6826ce4896cd5a500da7aa741cef862fc64

M appcompat/appcompat/src/main/java/androidx/appcompat/app/AppCompatDelegateImpl.java
M appcompat/appcompat/src/main/java/androidx/appcompat/app/ToolbarActionBar.java

https://android-review.googlesource.com/1696341

IssueTracker

AppCompatActivity.setSupportActionBar() incorrectly erases window callback

Status Update

Description

Summary

Root cause

Repro

Comments

al...@google.com <al...@google.com> #2Apr 30, 2021 06:18PM

py...@gmail.com <py...@gmail.com> #3Apr 30, 2021 06:35PM

ap...@google.com <ap...@google.com> #4May 4, 2021 08:25AM

lb...@gmail.com <lb...@gmail.com> #5May 4, 2021 08:26AM

ch...@gmail.com <ch...@gmail.com> #6May 4, 2021 08:33AM

al...@google.com <al...@google.com> #7May 4, 2021 01:48PM

py...@gmail.com <py...@gmail.com> #8May 4, 2021 04:14PM

al...@google.com <al...@google.com> #9May 4, 2021 04:33PM

py...@gmail.com <py...@gmail.com> #10May 4, 2021 05:13PM

al...@google.com <al...@google.com> #11May 4, 2021 05:20PM

ch...@google.com <ch...@google.com> #12May 5, 2021 12:59PM

py...@gmail.com <py...@gmail.com> #13May 5, 2021 01:06PM

py...@gmail.com <py...@gmail.com> #14May 5, 2021 01:17PM

ch...@google.com <ch...@google.com> #15May 5, 2021 01:18PM

ch...@google.com <ch...@google.com> #16May 5, 2021 02:42PM

ap...@google.com <ap...@google.com> #17May 6, 2021 01:21PM

Issue 186791590

Description

Summary

Root cause

Repro

Issue summary

Comments

al...@google.com <al...@google.com> #2Apr 30, 2021 06:18PM

py...@gmail.com <py...@gmail.com> #3Apr 30, 2021 06:35PM

ap...@google.com <ap...@google.com> #4May 4, 2021 08:25AM

lb...@gmail.com <lb...@gmail.com> #5May 4, 2021 08:26AM

ch...@gmail.com <ch...@gmail.com> #6May 4, 2021 08:33AM

al...@google.com <al...@google.com> #7May 4, 2021 01:48PM

py...@gmail.com <py...@gmail.com> #8May 4, 2021 04:14PM

al...@google.com <al...@google.com> #9May 4, 2021 04:33PM

py...@gmail.com <py...@gmail.com> #10May 4, 2021 05:13PM

al...@google.com <al...@google.com> #11May 4, 2021 05:20PM

ch...@google.com <ch...@google.com> #12May 5, 2021 12:59PM

py...@gmail.com <py...@gmail.com> #13May 5, 2021 01:06PM

py...@gmail.com <py...@gmail.com> #14May 5, 2021 01:17PM

ch...@google.com <ch...@google.com> #15May 5, 2021 01:18PM

ch...@google.com <ch...@google.com> #16May 5, 2021 02:42PM

ap...@google.com <ap...@google.com> #17May 6, 2021 01:21PM

Add comment

Issue metadata