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.

The issue is that Jetifier modifies jars and does not remove signatures. Below is the original class:

Classfile /tmp/non-jetified/org/bouncycastle/jce/provider/ExtCRLException.class
Last modified Mar 1, 2015; size 362 bytes
MD5 checksum 3968a716f9e60e1ad36c34157bd0239f
class org.bouncycastle.jce.provider.ExtCRLException extends java.security.cert.CRLException
minor version: 0
major version: 49
flags: ACC_SUPER
Constant pool:
#1 = Utf8 <init>
#2 = Utf8 cause
#3 = Utf8 getCause
#4 = Utf8 java/security/cert/CRLException
#5 = Utf8 org/bouncycastle/jce/provider/ExtCRLException
#6 = Class #4 // java/security/cert/CRLException
#7 = Class #5 // org/bouncycastle/jce/provider/ExtCRLException
#8 = Utf8 Ljava/lang/Throwable;
#9 = Utf8 (Ljava/lang/String;)V
#10 = Utf8 ()Ljava/lang/Throwable;
#11 = Utf8 (Ljava/lang/String;Ljava/lang/Throwable;)V
#12 = NameAndType #2:#8 // cause:Ljava/lang/Throwable;
#13 = NameAndType #1:#9 // "<init>":(Ljava/lang/String;)V
#14 = Fieldref #7.#12 // org/bouncycastle/jce/provider/ExtCRLException.cause:Ljava/lang/Throwable;
#15 = Methodref #6.#13 // java/security/cert/CRLException."<init>":(Ljava/lang/String;)V
#16 = Utf8 Code
{
java.lang.Throwable cause;
descriptor: Ljava/lang/Throwable;
flags:

org.bouncycastle.jce.provider.ExtCRLException(java.lang.String, java.lang.Throwable);
descriptor: (Ljava/lang/String;Ljava/lang/Throwable;)V
flags:
Code:
stack=2, locals=3, args_size=3
0: aload_0
1: aload_1
2: invokespecial #15 // Method java/security/cert/CRLException."<init>":(Ljava/lang/String;)V
5: aload_0
6: aload_2
7: putfield #14 // Field cause:Ljava/lang/Throwable;
10: return

public java.lang.Throwable getCause();
descriptor: ()Ljava/lang/Throwable;
flags: ACC_PUBLIC
Code:
stack=1, locals=1, args_size=1
0: aload_0
1: getfield #14 // Field cause:Ljava/lang/Throwable;
4: areturn
}

One processed with Jetifier:

Classfile /tmp/jetified/org/bouncycastle/jce/provider/ExtCRLException.class
Last modified Oct 14, 2019; size 362 bytes
MD5 checksum 76bc557706ee3480e53d88b21316913f
class org.bouncycastle.jce.provider.ExtCRLException extends java.security.cert.CRLException
minor version: 0
major version: 49
flags: ACC_SUPER
Constant pool:
#1 = Utf8 org/bouncycastle/jce/provider/ExtCRLException
#2 = Class #1 // org/bouncycastle/jce/provider/ExtCRLException
#3 = Utf8 java/security/cert/CRLException
#4 = Class #3 // java/security/cert/CRLException
#5 = Utf8 cause
#6 = Utf8 Ljava/lang/Throwable;
#7 = Utf8 <init>
#8 = Utf8 (Ljava/lang/String;Ljava/lang/Throwable;)V
#9 = Utf8 (Ljava/lang/String;)V
#10 = NameAndType #7:#9 // "<init>":(Ljava/lang/String;)V
#11 = Methodref #4.#10 // java/security/cert/CRLException."<init>":(Ljava/lang/String;)V
#12 = NameAndType #5:#6 // cause:Ljava/lang/Throwable;
#13 = Fieldref #2.#12 // org/bouncycastle/jce/provider/ExtCRLException.cause:Ljava/lang/Throwable;
#14 = Utf8 getCause
#15 = Utf8 ()Ljava/lang/Throwable;
#16 = Utf8 Code
{
java.lang.Throwable cause;
descriptor: Ljava/lang/Throwable;
flags:

org.bouncycastle.jce.provider.ExtCRLException(java.lang.String, java.lang.Throwable);
descriptor: (Ljava/lang/String;Ljava/lang/Throwable;)V
flags:
Code:
stack=2, locals=3, args_size=3
0: aload_0
1: aload_1
2: invokespecial #11 // Method java/security/cert/CRLException."<init>":(Ljava/lang/String;)V
5: aload_0
6: aload_2
7: putfield #13 // Field cause:Ljava/lang/Throwable;
10: return

public java.lang.Throwable getCause();
descriptor: ()Ljava/lang/Throwable;
flags: ACC_PUBLIC
Code:
stack=1, locals=1, args_size=1
0: aload_0
1: getfield #13 // Field cause:Ljava/lang/Throwable;
4: areturn
}

Changes in the constants pool cause changes in the code (indices change), which causes different MD5 checksums. This means that signatures will not match any more.

This started happening in 3.6.0-beta01 because we are using Jetifier with copyUnmodifiedLibsAlso=true flag (previously it was false).

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

@Chris You can work around this by updating gradle.properties with:

android.jetifier.blacklist=.*bcprov.*

This will avoid Jetifying the bouncycastle library.

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

Marked as fixed.

Thanks Ivan!

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

Pending fix in:

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

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

did this fix make it in for 3.6?

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

Accepted by al...@google.com.

Jetifier 1.0.0-beta08 was released and has a fix for this.

To avoid waiting for new Android Studio you can workaround this in the meantime by forcing usage of a new version of jetifier in gradle:
buildscript {
dependencies {
classpath 'com.android.tools.build:gradle:x.y.z'
classpath 'com.android.tools.build.jetifier:jetifier-processor:1.0.0-beta08' // <- Add this
}
}

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

Jetifier 1.0.0-beta08, which has the fix for this issue, will be integrated into AGP 3.6.0-rc01 and 4.0.0-alpha06:

https://android.googlesource.com/platform/tools/base/+/ef5e56702b626b6be642ecce7d0078b7b1a81bcb

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