Skip to content

Commit

Permalink
[M103] Fix on ListMenuButton's content is used by two parentViews
Browse files Browse the repository at this point in the history
The crash log still says the contentView is used when it has already
had a parent.

This crash appears after crrev.com/c/3509075 and the crash log does not
tell which feature client is causing this. Add a speculative fix
for now.

Merge of
https://chromium-review.googlesource.com/c/chromium/src/+/3669975
https://chromium-review.googlesource.com/c/chromium/src/+/3652000

Bug: 1323202
Change-Id: Ie101d62af8423ec85339843fb9265b3c8beddde8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3688241
Commit-Queue: Theresa Sullivan <twellington@chromium.org>
Auto-Submit: Lijin Shen <lazzzis@google.com>
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Cr-Commit-Position: refs/branch-heads/5060@{#521}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
Lijin Shen authored and Chromium LUCI CQ committed Jun 3, 2022
1 parent 229636c commit da8b2d7
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import android.text.TextUtils;
import android.util.AttributeSet;
import android.view.View;
import android.view.ViewGroup;
import android.view.ViewParent;

import org.chromium.base.CollectionUtil;
import org.chromium.base.ObserverList;
Expand Down Expand Up @@ -130,6 +132,7 @@ public void dismiss() {
*/
public void showMenu() {
if (!mIsAttachedToWindow) return;
dismiss();
initPopupWindow();
mPopupMenu.show();
notifyPopupListeners(true);
Expand All @@ -153,6 +156,11 @@ private void initPopupWindow() {
menu.addContentViewClickRunnable(this::dismiss);

final View contentView = menu.getContentView();
ViewParent viewParent = contentView.getParent();
// TODO(crbug.com/1323202): figure out why contentView is not removed from popup menu.
if (viewParent instanceof ViewGroup) {
((ViewGroup) viewParent).removeView(contentView);
}
mPopupMenu = new AnchoredPopupWindow(getContext(), this,
new ColorDrawable(Color.TRANSPARENT), contentView, mDelegate.getRectProvider(this));
mPopupMenu.setVerticalOverlapAnchor(mMenuVerticalOverlapAnchor);
Expand Down Expand Up @@ -253,4 +261,8 @@ private void notifyPopupListeners(boolean shown) {
}
});
}

void setAttachedToWindowForTesting() {
mIsAttachedToWindow = true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import android.content.Context;
import android.support.test.InstrumentationRegistry;
import android.view.View;

import androidx.test.filters.SmallTest;

Expand All @@ -19,6 +20,7 @@
import org.chromium.base.test.BaseJUnit4ClassRunner;
import org.chromium.base.test.util.Batch;
import org.chromium.components.browser_ui.widget.R;
import org.chromium.content_public.browser.test.util.TestThreadUtils;
import org.chromium.ui.test.util.BlankUiTestActivity;

/**
Expand Down Expand Up @@ -51,4 +53,31 @@ public void testA11yLabel() {
Assert.assertEquals(mContext.getString(R.string.accessibility_list_menu_button, title),
button.getContentDescription());
}

@Test
@SmallTest
public void testTriggerShowMenuTwice() {
ListMenuButton button = new ListMenuButton(mContext, null);
button.setAttachedToWindowForTesting();
View view = new View(mContext);
button.setDelegate(() -> new ListMenu() {
@Override
public View getContentView() {
return view;
}

@Override
public void addContentViewClickRunnable(Runnable runnable) {}

@Override
public int getMaxItemWidth() {
return 0;
}
}, true);
// Expect no crash when calling showMenu twice.
TestThreadUtils.runOnUiThreadBlocking(() -> {
button.showMenu();
button.showMenu();
});
}
}

0 comments on commit da8b2d7

Please sign in to comment.