New issue
Advanced search Search tips

Issue 788430 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Cannot open bookmark from hamburger menu

Reported by kante...@gmail.com, Nov 24 2017

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3278.0 Safari/537.36

Steps to reproduce the problem:
1. Load Chromium 
2. Click 3-dot menu to the right of URL address bar
3. Mouse to "Bookmarks"
4. Click any Bookmark

What is the expected behavior?
The Bookmarked page would open.

What went wrong?
Nothing happens. 

Did this work before? Yes 

Chrome version: 64.0.3278.0  Channel: n/a
OS Version: OS X 10.13.1
Flash Version: 

Bookmarks can still be opened via:

1. Bookmarks menu
2. Bookmarks bar
 

Comment 1 by meh...@chromium.org, Nov 25 2017

Components: -UI UI>Browser>Bookmarks UI>Browser>Toolbar
Labels: -Pri-2 Needs-Bisect Pri-1
Status: Untriaged (was: Unconfirmed)
Thanks for the report. I can reproduce it in latest Canary Version 64.0.3278.0 on macOS 10.12.6.

Comment 2 by rsesek@chromium.org, Nov 25 2017

Labels: M-64 ReleaseBlock-Beta
Owner: tapted@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by tapted@chromium.org, Nov 26 2017

Status: Started (was: Assigned)
Ungh. This is because of

 bookmarkMenuBridge_ = nullptr;

at https://chromium-review.googlesource.com/c/chromium/src/+/778644/14/chrome/browser/ui/cocoa/app_menu/app_menu_controller.mm#398


The action gets sent after the -[NSMenuDelegate menuDidClose:]. Also after NSMenuDidEndTrackingNotification. There's also NSMenuDidSendActionNotification and NSMenuWillSendActionNotification but they're not sent to the root menu.

I think I need to give up on trying to free up that memory when the menu is closed. Proper fix is to make it able to be cached per the TODO in updateBookmarkSubMenu.
Labels: -Needs-Bisect hasbisect-per-revision Triaged-ET Needs-Triage-M64
Able to reproduce the issue on mac 10.12.6 using chrome reported version #64.0.3278.0

Bisect Information:
=====================
Good build: 64.0.3274.0    Revision(518061)
Bad Build : 64.0.3275.0    Revision(518486)

Change Log URL: 
https://chromium.googlesource.com/chromium/src/+log/f2908761285f0cfe9b2941cb69afa1bf6eac307c..cf95689a88664eb44716632cc7320d18ed3324ba

From the above change log suspecting below change
Change-Id: Idc0a28cf9483f7161dc3fbe9bd6cda717e65de98
Reviewed-on: https://chromium-review.googlesource.com/778644

tapted@ - Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.

Thanks...!!

Comment 5 by ajha@chromium.org, Nov 28 2017

M-64 will be branched in 2 days from now and would be good to have all the Beta blockers resolved before that. Please plan the fix accordingly.

Thank you!
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5650735a52f82f914e143e5c97a806c5bf93efea

commit 5650735a52f82f914e143e5c97a806c5bf93efea
Author: Trent Apted <tapted@chromium.org>
Date: Tue Nov 28 21:09:29 2017

Fix opening bookmarks from Chrome App menu.

This regressed in r518459 which added a step to clear out a stale
NSMenu tree once it could never be used again. But it turns out that
-[NSMenuDelegate menuDidClose:] is a bad signal for this. AppKit isn't
actually done with the menu at that point, and doesn't give the signal
we need.

To fix, keep the menu around as before, and improve test coverage.

Bug:  788430 
Change-Id: Idbc19cb56e6e52d699d94518b2164d0bd776ce9d
Reviewed-on: https://chromium-review.googlesource.com/790090
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519835}
[modify] https://crrev.com/5650735a52f82f914e143e5c97a806c5bf93efea/chrome/browser/ui/cocoa/app_menu/app_menu_controller.mm
[modify] https://crrev.com/5650735a52f82f914e143e5c97a806c5bf93efea/chrome/browser/ui/cocoa/app_menu/app_menu_controller_unittest.mm
[modify] https://crrev.com/5650735a52f82f914e143e5c97a806c5bf93efea/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.h

Comment 7 by tapted@chromium.org, Nov 28 2017

Status: Fixed (was: Started)

Sign in to add a comment