When opening the "Show all bookmarks" window on Mac, I'm currently seeing two errors: ypeError: can't redefine non-configurable property "gEditItemOverlay"[Learn More] XPCOMUtils.jsm:234:7 TypeError: gBrowserInit is undefined[Learn More] macBrowserOverlay.xul:77:43 This also leads to a lot of the menu items being enabled when they shouldn't be (e.g. View menu). This is a regression from bug 1381853 which changed how the edit overlay item was being loaded: https://hg.mozilla.org/mozilla-central/rev/3a5168e0b5e4 On Mac, the places.xul loads macBrowserOverlay.xul which in turn loads browser.js which defines the lazy getter. However, places.xul is also loading editBookmarksOverlay.js which also defines its own defineLazyGetter for gEditItemOverlay. Hence, at some stage everything blows up. From what I can tell the only visible effects are the menu issues, and the warnings on the console.

Notes: Ideally I think we probably need to reflect on what macBrowserOverlay.xul brings to places.xul and if we need it importing as much, since Windows & Linux obviously import a lot less. However I'm not sure that's worth the effort at the moment. Florian, I couldn't think of an easy way to test this, since I believe testing the main menus on Mac is difficult. If you've got any ideas, please let me know.

[Tracking Requested - why for this release]: regression in 57

Comment on attachment 8910891 [details] Bug 1402059 - Don't import editBookmarkOverlay.js in places.xul on Mac because it is imported elsewhere. https://reviewboard.mozilla.org/r/182372/#review187928 It's unfortunate that we import something as big as browser.js in all windows on Mac. But that's not a new problem. (In reply to Mark Banner (:standard8) from comment #1) > Florian, I couldn't think of an easy way to test this, since I believe > testing the main menus on Mac is difficult. If you've got any ideas, please > let me know. Testing the actual main menubar is probably difficult or even impossible, but grabbing DOM nodes and generating artificial popupshowing/command events where needed is probably enough. I think that would be enough to make a test that could catch future regressions there. It's sad that we didn't catch this before it was too late to fix for 56, so a test would be appreciated... but I'm not requiring it, so r+.

And I forgot to say, thanks for fixing this! :-)

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/06df6fc90565 Don't import editBookmarkOverlay.js in places.xul on Mac because it is imported elsewhere. r=florian

Comment on attachment 8910891 [details] Bug 1402059 - Don't import editBookmarkOverlay.js in places.xul on Mac because it is imported elsewhere. Approval Request Comment [Feature/Bug causing the regression]: bug 1381853 [User impact if declined]: broken menus in the places window, on Mac only. [Is this code covered by automated tests?]: no. [Has the fix been verified in Nightly?]: not yet [Needs manual test from QE? If yes, steps to reproduce]: on Mac, open the "Show All Bookmarks" window, and look at the menus. Lots of items should be disabled, and weren't before this patch landed. There are also errors in the browser console. [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: it just avoids including the same script file once lazily and once synchronously (thus conflicting with the lazy getter) in the same window. [String changes made/needed]: none

(In reply to Marco Bonardo [::mak] from comment #2) > [Tracking Requested - why for this release]: regression in 57 The status flags says that it is a regression in 56. Who is correct?

You are right, it's also in 56, but too late to fix it there.

(In reply to Sylvestre Ledru [:sylvestre] from comment #10) > (In reply to Marco Bonardo [::mak] from comment #2) > > [Tracking Requested - why for this release]: regression in 57 > > The status flags says that it is a regression in 56. Who is correct? Sorry, typed this too quickly. It's a regression caused by photon-performance patches I did "for 57", but they landed in 56 (in bug 1381853).

Comment on attachment 8910891 [details] Bug 1402059 - Don't import editBookmarkOverlay.js in places.xul on Mac because it is imported elsewhere. Fix a recent regression. Taking it. Should be in 57b3

Reproduced on Firefox 56.0.2. Verified as fixed using Firefox 57 beta 12 and latest Nightly 58.0a1 2017-10-30 under Mac OS X 10.13.