The Refresh Firefox feature currently resets preferences to default which hides the bookmarks toolbar. Users who do a refresh and have bookmarks on that toolbar often think their bookmarks are lost when actually they are just hidden. I propose that we stop hiding the bookmarks toolbar when a user does a refresh if that user has any non-default bookmarks on the bookmarks toolbar.

I thought we had code that checks if there are any bookmarks in the toolbar and then auto-shows it if the number is greater than some threshold which is what you're asking for… maybe that regressed or was removed?

So I'm not crazy as I found bug 756080 reporting the behaviour you want as a defect but I can't find the code that's supposed to do this.

I'm running mozregression now to find when this broke/changed.

Last good revision: 69ec3dc408a2a720cb2b8210fea33e3504aeec22 (2016-02-20) First bad revision: af6356a3e8c56036b74ba097395356d9c6e6c5a3 (2016-02-21) Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=69ec3dc408a2a720cb2b8210fea33e3504aeec22&tochange=af6356a3e8c56036b74ba097395356d9c6e6c5a3 This is a regression from bug 1249608. The deleted code was https://dxr.mozilla.org/mozilla-central/rev/d5daf10d3b74b04e8fa63c4e5429de8a0adf79f8/browser/components/nsBrowserGlue.js#1848-1871 IIUC Bug 1249608 comment 2 even pointed out that this would regress behaviour :( This is possibly a source of the recurring complaints about a Refresh losing bookmarks.

What regressed wasn't specific to Refresh, it affects any "new" profile (Refresh makes a new profile) so this code should like outside the migration directory so that it handles a profile directory which isn't the 1st e.g. dropping places.sqlite in an empty folder and making that a profile folder.

Bug 1280999 later removed this code altogether.

Comment on attachment 8926763 [details] Bug 1415692 - Show the bookmark toolbar in new profiles with > 3 bookmarks on it. https://reviewboard.mozilla.org/r/198002/#review203200 ::: browser/components/nsBrowserContentHandler.js:536 (Diff revision 1) > case OVERRIDE_NEW_PROFILE: > // New profile. Of course this approach doesn't work for a Refresh since we set browser.startup.homepage_override.mstone then to avoid the firstrun pages: https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/browser/components/migration/FirefoxProfileMigrator.js#177

Comment on attachment 8926763 [details] Bug 1415692 - Show the bookmark toolbar in new profiles with > 3 bookmarks on it. https://reviewboard.mozilla.org/r/198002/#review203216 I'm not sure about the callsites of the checks here. The unfortunate thing is that automigration is currently async, and as a result there's no guarantee that it will have run when this code is running, so bookmarks may not be present (yet). Because of this, if the aim here is to ensure we do the right thing specifically after a startup migration like either on initial profile setup or from a reset/refresh, I think it would be better to do this type of thing in the migrator iff: - we're doing a startup migration AND - we've migrated at least one bookmarks resource type rather than in this UI migration code. Separately, I'm not sure under what circumstances on a clean profile we'd ever have persisted customized state such that there's a persisted currentset for the toolbar. How would that scenario occur? (I'm assuming it can given that the code caters for this case, but I don't believe refresh keeps either localstore or customization prefs, nor do we import it from other browsers...) ::: browser/components/migration/tests/marionette/test_refresh_firefox.py:219 (Diff revision 2) > let bookmarkIds = PlacesUtils.bookmarks.getBookmarkIdsForURI(makeURI(url), {}, {}); > return bookmarkIds.length == 1 ? PlacesUtils.bookmarks.getItemTitle(bookmarkIds[0]) : ""; > """, script_args=(self._bookmarkURL,)) > self.assertEqual(titleInBookmarks, self._bookmarkText) > > + def checkBookmarksOnToolbar(self): Can you name this differently so it's explicit about checking the collapsed state of the toolbar rather than the presence of the bookmarks themselves?

(In reply to :Gijs (slow, PTO recovery mode) from comment #10) > I'm not sure about the callsites of the checks here. The unfortunate thing > is that automigration is currently async, and as a result there's no > guarantee that it will have run when this code is running, so bookmarks may > not be present (yet). Oh yeah, I wasn't thinking about automigration. I don't understand the state of that project TBH. > Because of this, if the aim here is to ensure we do the right thing > specifically after a startup migration like either on initial profile setup > or from a reset/refresh, I think it would be better to do this type of thing > in the migrator iff: > - we're doing a startup migration AND > - we've migrated at least one bookmarks resource type > > rather than in this UI migration code. > > Separately, I'm not sure under what circumstances on a clean profile we'd > ever have persisted customized state such that there's a persisted > currentset for the toolbar. How would that scenario occur? (I'm assuming it > can given that the code caters for this case, but I don't believe refresh > keeps either localstore or customization prefs, nor do we import it from > other browsers...) As I hinted at in comment 5, I put it back in _migrateUI where it originally was so that it handles someone doing a Refresh/Reset by hand, creating a profile from a template (that contains places.sqlite or similar), and distribution default bookmarks (I think that's still a thing). > ::: browser/components/migration/tests/marionette/test_refresh_firefox.py:219 > (Diff revision 2) > > let bookmarkIds = PlacesUtils.bookmarks.getBookmarkIdsForURI(makeURI(url), {}, {}); > > return bookmarkIds.length == 1 ? PlacesUtils.bookmarks.getItemTitle(bookmarkIds[0]) : ""; > > """, script_args=(self._bookmarkURL,)) > > self.assertEqual(titleInBookmarks, self._bookmarkText) > > > > + def checkBookmarksOnToolbar(self): > > Can you name this differently so it's explicit about checking the collapsed > state of the toolbar rather than the presence of the bookmarks themselves? Sure

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #11) > (In reply to :Gijs (slow, PTO recovery mode) from comment #10) > > I'm not sure about the callsites of the checks here. The unfortunate thing > > is that automigration is currently async, and as a result there's no > > guarantee that it will have run when this code is running, so bookmarks may > > not be present (yet). > > Oh yeah, I wasn't thinking about automigration. I don't understand the state > of that project TBH. Me neither. Well, let's leave that out for now, then, and we can always make automigration do something else separately...

Comment on attachment 8926763 [details] Bug 1415692 - Show the bookmark toolbar in new profiles with > 3 bookmarks on it. https://reviewboard.mozilla.org/r/198002/#review203246 r=me with the name nit fixed, then. Thanks for clarifying!

I'm not completely convinced that the presence of bookmarks in the toolbar is a sign that the user had it visible, we may have users with bookmarks in the toolbar that don't really care about them, and due to Sync bugs duplicating default bookmarks we may end up showing a toolbar full of things the user doesn't need. And there's no simple "X" button to remove the toolbar. From what I see, the migration UI step that was removed was for when we decided to hide the toolbar by default, but we wanted to preserve it open for users having more than 3 bookmarks on it. I don't think the removal of that code can be considered a regression. I'm not particularly against the change, but off-hand it sounds like something that will have deeper implications. The count could likely be a bit larger than 3, it would be indeed trivial to regress this by adding new default bookmarks (unlikely but...). How can we prevent that? And we could have those bogus sync duplicates, I've seen users having 2 or 3 copies of "Most visited" on the toolbar. I'd suggest the user should have at least 6 bookmarks, but again, I am just shouting numbers without good data to do a best pick. I'll take a bit more time to think about other possible implications of the change. With the purpose of fixing this before the merge obviously.

(In reply to Marco Bonardo [::mak] from comment #14) > The count could likely be a bit larger than 3, it would be indeed trivial to > regress this by adding new default bookmarks (unlikely but...). How can we > prevent that? Maybe add a test that asserts the number of default bookmarks == 3 (with a comment recommending that the magic number 3 in this bug's toolbar refresh code and test be incremented)? > I'd suggest the user should have at least 6 bookmarks, but again, I am just > shouting numbers without good data to do a best pick. OTOH, people are nervous about losing their personal data when they refresh Firefox. I think we should err on the side of showing the toolbar if there is a chance the toolbar was visible. The user anxiety of thinking their toolbar bookmarks are lost is much greater than the user frustration of having to (figure out how to) hide the toolbar if they hadn't been using it. Off-topic: I don't know why we hide the bookmarks toolbar by default. I have 20+ bookmarks (minimized to favicons without a title) in my toolbar and they are my main navigation tool!

(In reply to Marco Bonardo [::mak] from comment #14) > I'm not completely convinced that the presence of bookmarks in the toolbar > is a sign that the user had it visible, we may have users with bookmarks in > the toolbar that don't really care about them, and due to Sync bugs > duplicating default bookmarks we may end up showing a toolbar full of things > the user doesn't need. And there's no simple "X" button to remove the > toolbar. > > From what I see, the migration UI step that was removed was for when we > decided to hide the toolbar by default, but we wanted to preserve it open > for users having more than 3 bookmarks on it. I don't think the removal of > that code can be considered a regression. Well it's a regression from the viewpoint of migrator as it depended on this behaviour. > I'm not particularly against the change, but off-hand it sounds like > something that will have deeper implications. > The count could likely be a bit larger than 3, it would be indeed trivial to > regress this by adding new default bookmarks (unlikely but...). Sure, I'm looking as this bug from the angle of restoring the behaviour we had before the code was removed as it didn't intend to regress this behaviour. > How can we prevent that? I agree with Chris that we can write a test. I considered it last night but was thinking it would be a lot of boilerplate… I have an idea to make it as a m-bc test. > And we could have those bogus sync duplicates, I've seen users having 2 or 3 > copies of "Most visited" on the toolbar. > > I'd suggest the user should have at least 6 bookmarks, but again, I am just > shouting numbers without good data to do a best pick. Yeah, I don't know a good number either so that's why I'm just restoring the old number. > I'll take a bit more time to think about other possible implications of the > change. With the purpose of fixing this before the merge obviously. Thanks The main blocker to landing now is browser/base/content/test/performance/browser_startup.js: > resource://gre/modules/PlacesUtils.jsm is not allowed before opening first browser window Unfortunately that test is actually testing startup in a new profile (I think unintentionally) but I don't want to add exceptions for all the Places files that get imported as they shouldn't be used that early on a second-run case…

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #16) > Unfortunately that test is actually testing startup in a new profile (I > think unintentionally) but I don't want to add exceptions for all the Places > files that get imported as they shouldn't be used that early on a second-run > case… The toolbar adds a cost to startup regardless of the run. If the toolbar is visible, we pay a cost to initialize Places sooner than usual. We had some projects to reduce this cost (async populating of the toolbar) but didn't have time to do it. (In reply to Chris Peterson [:cpeterson] from comment #15) > Off-topic: I don't know why we hide the bookmarks toolbar by default. I have > 20+ bookmarks (minimized to favicons without a title) in my toolbar and they > are my main navigation tool! The toolbar has a cost on startup, and in general bookmarks have been often considered a second citizen to the new tab page thumbs. I think the only idea I saw around to revert that, was to show the toolbar only in new/empty tabs.

(In reply to Marco Bonardo [::mak] from comment #17) > (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from > comment #16) > > Unfortunately that test is actually testing startup in a new profile (I > > think unintentionally) but I don't want to add exceptions for all the Places > > files that get imported as they shouldn't be used that early on a second-run > > case… > > The toolbar adds a cost to startup regardless of the run. If the toolbar is > visible, we pay a cost to initialize Places sooner than usual. We had some > projects to reduce this cost (async populating of the toolbar) but didn't > have time to do it. Right, but the problem here wasn't the bookmark toolbar being visible since it's not in a clean profile used for browser_startup.js, it's the implementation of _maybeToggleBookmarkToolbarVisibility running PlacesUtils.getFolderContents. My workaround is to skip UI migrations for the performance directory which should allow us to have stricter blocklists for the common startup case.

Comment on attachment 8926763 [details] Bug 1415692 - Show the bookmark toolbar in new profiles with > 3 bookmarks on it. https://reviewboard.mozilla.org/r/198002/#review203696 It sounds like the benefit of not surprising users overwhelm my fears, so let's do this. The only thing I don't like is that this whole thing is synchronous and in the future we'll have to figure out a way to make it async... but that's still far away, maybe by that time we decided to completely change the toolbar to something else. ::: browser/components/migration/tests/marionette/test_refresh_firefox.py:58 (Diff revision 3) > """, script_args=(self._bookmarkURL, self._bookmarkText)) > > + def createBookmarksOnToolbar(self): > + self.marionette.execute_script(""" > + for (let i = 1; i <= 5; i++) { > + PlacesUtils.bookmarks.insertBookmark(PlacesUtils.toolbarFolderId, It would be great if this code would use the new async bookmarking API, rather than the old one, maybe in a follow-up? ::: browser/components/migration/tests/marionette/test_refresh_firefox.py:67 (Diff revision 3) > + > def createHistory(self): > error = self.runAsyncCode(""" > // Copied from PlacesTestUtils, which isn't available in Marionette tests. > let didReturn; > PlacesUtils.asyncHistory.updatePlaces( As well as it would be great if it would use PlacesUtils.history.insertMany... (again, a follow-up bug would be fine) ::: browser/components/nsBrowserGlue.js:1759 (Diff revision 3) > + const BROWSER_DOCURL = "chrome://browser/content/browser.xul"; > + let xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore); > + > + if (!xulStore.hasValue(BROWSER_DOCURL, "PersonalToolbar", "collapsed")) { > + // We consider the toolbar customized if it has more than > + // 3 children, or if it has a persisted currentset value. let's put the number of children in a const, and refer the const name here and later, so that only one code line needs to be changed in case we should ever need to change it. ::: browser/components/nsBrowserGlue.js:1789 (Diff revision 3) > Services.prefs.setIntPref("browser.migration.version", UI_VERSION); > + > + // New profiles may have existing bookmarks (imported from another browser or > + // copied into the profile) and we want to show the bookmark toolbar for them > + // in some cases. > + this._maybeToggleBookmarkToolbarVisibility(); This is not a critical part of the migration, and as such I'd probably wrap it in a try/catch (with a reportError... though can we do that this early?). Suppose places.sqlite is corrupt for some reason, we don't want this to break startup. ::: browser/components/tests/browser/browser_default_bookmark_toolbar_visibility.js:1 (Diff revision 3) > +/** please add a PD license header. While tests default to PD, according to Gerv we are still supposed to add a license header. ::: browser/components/tests/browser/browser_default_bookmark_toolbar_visibility.js:3 (Diff revision 3) > +/** > + * Test _maybeToggleBookmarkToolbarVisibility() code running for new profiles. > + * Ensure that in a default configuration that the bookmark toolbar is hidden. too many "that"? also "bookmarkS toolbar"

Comment on attachment 8926763 [details] Bug 1415692 - Show the bookmark toolbar in new profiles with > 3 bookmarks on it. https://reviewboard.mozilla.org/r/198002/#review203696 > This is not a critical part of the migration, and as such I'd probably wrap it in a try/catch (with a reportError... though can we do that this early?). > Suppose places.sqlite is corrupt for some reason, we don't want this to break startup. Good idea. reportError empirically works fine at that point and I didn't think there was problems with use then anyways. > please add a PD license header. While tests default to PD, according to Gerv we are still supposed to add a license header. Were you told this recently? I think most people on the team don't include them so this warrants a mailing list post.

Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/d3c111ae8e20 Show the bookmark toolbar in new profiles with > 3 bookmarks on it. r=Gijs,mak

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #21) > Were you told this recently? I think most people on the team don't include > them so this warrants a mailing list post. No, it was time ago when we made tests default to PD, I assumed I could avoid boilerplate, but he said in a bug comment (can't find it atm :() that those are not optional, the fallback is just for tests missing a header or files that can't have one.

Backed out changeset d3c111ae8e20 (bug 1415692) for leaks in windows clipboard tests https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d3c111ae8e200b6f02e04db1a27d94061db293c9&filter-searchStr=42a33b920a3bcd293db0d86e0790616393982030 https://treeherder.mozilla.org/logviewer.html#?job_id=144002423&repo=autoland&lineNumber=20390 https://hg.mozilla.org/mozilla-central/rev/aabfc14671b55983e1c3053989a4c3b7c5691aaa

It makes very little sense that this bug would cause that leak since according to the logs and inspection, this patch shouldn't affect any clipboard jobs…

Rebased try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be28bd661df2b28807d982980a7401c136c70190

Again but forcing the bookmark toolbar count to 1 without calling Places APIs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2ee67811e5c016d9394f603164ef58775860177 I can't repro this locally on Windows 8.1 32-bit debug :(

It sounds like this is one issue, amongst a few, leading to people thinking that they lost bookmarks on updating (because of being prompted to refresh if they've got an old profile). Can we prioritize this?

Yeah, we have a record number of lapsed users reactivating, and so likely more refreshes than we've seen before from people who are not regular Firefox users.

(In reply to Matthew N. [:MattN] (away Nov. 20) from comment #29) > Again but forcing the bookmark toolbar count to 1 without calling Places > APIs: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=a2ee67811e5c016d9394f603164ef58775860177 > > I can't repro this locally on Windows 8.1 32-bit debug :( So, this trypush has no leaks (no places access), whereas the one from comment 28 does. So it seems like, on infra, places is leaking here. That's... unfortunate. :-\ Marco, any ideas why this would leak? Any obvious things the code should be doing to avoid this?

There are 2 possibilities: either the code fetching the childCount is leaking, or mochitests somehow end up showing the bookmarks toolbar with the new code, and that somehow causes a leak we never detected because we don't show it anymore by default, so that's pretty much untested. It would be easy to exclude the former, just move the return 1 after toolbarFolder.containerOpen = false; I'll see if I can reproduce locally in the meanwhile.

(In reply to Marco Bonardo [::mak] from comment #33) > It would be easy to exclude the former, just move the return 1 after > toolbarFolder.containerOpen = false; https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d5914e1f78c7a49f5d5e53bdcd81017980f39ce

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #34) > (In reply to Marco Bonardo [::mak] from comment #33) > > It would be easy to exclude the former, just move the return 1 after > > toolbarFolder.containerOpen = false; > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=6d5914e1f78c7a49f5d5e53bdcd81017980f39ce This leaks still so the problem is that "the code fetching the childCount is leaking". Also, when I was running the tests locally on my Win 8 machine (that didn't report leaks), the bookmark toolbar wasn't shown.

I confirm that I cannot reproduce the leak locally. I wonder if it's a real leak, or just the fact chrome mochitests are unlikely to run all the gc/cc we run in mochitest-browser tests (https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/testing/mochitest/browser-test.js#902). The history results are cycle collected, maybe by the time the chrome mochitests end we didn't cycle collect the result yet.

IIUC the failing test is dom/events/test/test_bug1327798.html which is actually mochitest-plain btw. With it being in the clipboard subsuite it makes it a bit more annoying to make reduced try pushes and try to repro locally since you can't just run the directory. I did a try push for just the path dom/events/test/ and AFAICT it didn't leak and still ran test_bug1327798.html: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8928592272d68ea1b37daf501189679323c8bd83 I was hoping to find that it would leak so I would know for sure that changes to this test or the .ini for it would solve the immediate problem but that wasn't the case. Also note that it's the content ("tab") process that's leaking IIUC which makes me think this is related to the history service which PlacesUtils uses. Could it be as simple as disabling history via prefs for this test dir. as a workaround? The other obvious workaround to get this landed is to move the code to the migrator like was talked about earlier.

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #37) > Could it be as simple as disabling history via prefs for this test dir. as a > workaround? Try push with places.history.enabled=false (I'm not sure if this will disable enough): https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cf36aedfa1ec7f12ccd493a42f427699516569d We probably wouldn't want to land with this applying to the whole directory as it may break other tests but `prefs` are only supported in `[DEFAULT]` but if this works we could move the test to another (new sub-)directory as a workaround.

May this code be async? We could easily add a special async helper in PlacesUtils to get the number of children of a given folder, it wouldn't create a whole history result, that is likely what creates problems here.

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #38) > (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from > comment #37) > > Could it be as simple as disabling history via prefs for this test dir. as a > > workaround? > > Try push with places.history.enabled=false (I'm not sure if this will > disable enough): > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=8cf36aedfa1ec7f12ccd493a42f427699516569d Unfortunately this still leaked. :-( (In reply to Marco Bonardo [::mak] from comment #39) > May this code be async? In principle, yes. In practice, we'd need to make sure that when applying the toolbar state, we also do it in any windows that have opened by that time. > We could easily add a special async helper in PlacesUtils to get the number > of children of a given folder, it wouldn't create a whole history result, > that is likely what creates problems here. "likely" makes me nervous - how confident are we that that would work? If disabling history in that trypush didn't work - this sounds like substantially more work to test, but maybe I misunderstand how easy this would be to implement...

it would pretty much be a method in PlacesUtils like: async getChildCount(parentGuid) { let db = await this.promiseDBConnection(); let rows = await db.execute(SQL_TO_PICK_COUNT); return rows[0].getElementByName(...); }

note that we are moving off synchronous I/O, thus in any case sooner or later this code should become async.

I'm going to move this code to the migrator to get it landed and fixed for the common case.

Of course that isn't so straightforward since Places is started in the migrator only for non-Refresh migrations… I'm a bit nervous about changing when we init Places for a Refresh if we want to uplift this. The other simple workaround to get this landed without fixing the leak would be to use the same workaround as https://reviewboard.mozilla.org/r/198002/diff/4#file5752808 but for the DOM test which reports the leak… it would be unusual to have DOM tests set a browser.* pref but it's probably not the first time.

So the workaround to set the browser.migration.version pref for that DOM directory works (as one would hope): https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b192924fb2041cb87e728cd44dcf3aa9bdd6aa8 https://reviewboard-hg.mozilla.org/gecko/rev/851522bfe156#l6.1 I know it's not great but it seems like recently discussed approaches were already suggesting approaches to simply avoid the leak so I don't think this is much worse. Bug 1398563 can be used to track actually fixing the leak. Thoughts?

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #46) > I know it's not great but it seems like recently discussed approaches were > already suggesting approaches to simply avoid the leak so I don't think this > is much worse. Bug 1398563 can be used to track actually fixing the leak. I'm ok with workarounding the problem for now, in the end I don't think this is a real leak, I really think it's just our harness not doing proper/complete CC. On the other side, I'd like to see a follow-up bug to convert this code to async, because it will have to be done sooner or later, and I don't feel like Bug 1398563 is a good bug to track that work.

I concur with Marco.

Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/8312a6286023 Show the bookmark toolbar in new profiles with > 3 bookmarks on it. r=Gijs,mak

Backed out for failing to parse dom/events/test/mochitest.ini on Android due pref usage: https://hg.mozilla.org/integration/autoland/rev/aca86338b40340ddf9dafe60007cb52f4116c000 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8312a62860233cd38bda9689f95b23d62e69673a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=149931454&repo=autoland [task 2017-12-05T17:47:10.435Z] 17:47:10 INFO - Android sdk version '18'; will use this to filter manifests [task 2017-12-05T17:47:11.972Z] 17:47:11 INFO - Checking for ssltunnel processes... [task 2017-12-05T17:47:11.980Z] 17:47:11 INFO - Checking for xpcshell processes... [task 2017-12-05T17:47:12.538Z] 17:47:12 INFO - 0 ERROR parsing dom/events/test/mochitest.ini: runByManifest mode must be enabled to set the `prefs` key [task 2017-12-05T17:47:12.538Z] 17:47:12 INFO - 1 ERROR Automation Error: Exception caught while running tests

AFAICT that error is useless in this case since the whole directory has `skip-if = toolkit == 'android'` on the line right below the `prefs =`… :( Try push to try and skip that check if the test is disabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e74fbff3e5af3beb9d425e257858001200fe8c02

Comment on attachment 8934746 [details] Bug 1415692 - Don't enforce runByManifest for `prefs` in manifests on disabled tests. https://reviewboard.mozilla.org/r/205624/#review211556 Thanks

Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/49358c06a122 Show the bookmark toolbar in new profiles with > 3 bookmarks on it. r=Gijs,mak https://hg.mozilla.org/integration/autoland/rev/6b8a627984d8 Don't enforce runByManifest for `prefs` in manifests on disabled tests. r=ahal

Comment on attachment 8926763 [details] Bug 1415692 - Show the bookmark toolbar in new profiles with > 3 bookmarks on it. Approval Request Comment [Feature/Bug causing the regression]: bug 1249608 [User impact if declined]: Users who do a Firefox Refresh or import of data from another browser resulting in bookmarks on their bookmark toolbar won't see the bookmark toolbar by default. This is leading some users to believe their bookmarks have been lost or not imported. [Is this code covered by automated tests?]: Yes, but first-run browser migration doesn't have great functional test coverage because it was traditionally not possible to test with our harnesses. [Has the fix been verified in Nightly?]: Not yet [Needs manual test from QE? If yes, steps to reproduce]: Probably a good idea since . * Test that the bookmark toolbar is shown by default after a Refresh if the bookmark toolbar has >3 bookmarks on it beforehand. * Test that the bookmark toolbar is shown by default after a new user migration wizard (remove profiles.ini) if the source browser's bookmark toolbar has >3 bookmarks on it beforehand. * Test both of the above with 3 or fewer bookmarks on the toolbar and ensure that the bookmark toolbar isn't shown by default. * Test that the bookmark toolbar isn't shown in a new profile that cancels the migration wizard. [List of other uplifts needed for the feature/fix]: The other attachment in this bug to fix an automation issue. [Is the change risky?]: Not really [Why is the change risky/not risky?]: Because it's restoring deleted code that used to run for years. The code should only run for brand new profiles so is very low risk to existing profiles. [String changes made/needed]: None

Comment on attachment 8934746 [details] Bug 1415692 - Don't enforce runByManifest for `prefs` in manifests on disabled tests. See comment 56

https://hg.mozilla.org/mozilla-central/rev/49358c06a122 https://hg.mozilla.org/mozilla-central/rev/6b8a627984d8

Comment on attachment 8926763 [details] Bug 1415692 - Show the bookmark toolbar in new profiles with > 3 bookmarks on it. ok let's do this in 58. Is there a followup bug for the async work Marco mentioned in comment 47?

https://hg.mozilla.org/releases/mozilla-beta/rev/3c0c4b0d577f https://hg.mozilla.org/releases/mozilla-beta/rev/2b215d128833

(In reply to Marco Bonardo [::mak] (Away 16 Dec - 2 Jan) from comment #47) > (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from > comment #46) > > I know it's not great but it seems like recently discussed approaches were > > already suggesting approaches to simply avoid the leak so I don't think this > > is much worse. Bug 1398563 can be used to track actually fixing the leak. > > I'm ok with workarounding the problem for now, in the end I don't think this > is a real leak, I really think it's just our harness not doing > proper/complete CC. Yeah, FTR I agree it's not a real leak (or at least not one that most users would hit), it seems to be related to the chunking and how that affects the during of the session the browser runs in. > On the other side, I'd like to see a follow-up bug to convert this code to > async, because it will have to be done sooner or later, and I don't feel > like Bug 1398563 is a good bug to track that work. (In reply to Julien Cristau [:jcristau] from comment #59) > Is there a followup bug for the async work Marco mentioned in comment 47? I filed bug 1424079 for using Async Places APIs

I managed to reproduce the bug using an older version of Nightly (2017-11-15) on Windows 10 x 64. I retested everything on latest Nightlt 59 and beta 58.0b10 using Windows 10 x64, Mac OS 10.11 and Ubuntu 16.04 x32 and I got these results: 1. Test that the bookmark toolbar is shown by default after a Refresh if the bookmark toolbar has >3 bookmarks on it beforehand. 2. Test that the bookmark toolbar is shown by default after a new user migration wizard (remove profiles.ini) if the source browser's bookmark toolbar has >3 bookmarks on it beforehand. 3. Test both of the above with 3 or fewer bookmarks on the toolbar and ensure that the bookmark toolbar isn't shown by default. This part I would say that it's verified, but on the last method from comment 56, I am not sure what exactly is supposed to happen. Matthew, can you please give more information about that step? Thank you.

If you mean: > * Test that the bookmark toolbar isn't shown in a new profile that cancels the migration wizard. then I mean to remove/move profiles.ini and start Firefox so it will show the migration wizard. Close/cancel the migration wizard and ensure that the bookmark toolbar isn't visible by default.

I tested on the latest Nightly 59 and beta 58.0b11 using Windows 10 x64, macOS 10.13 and Ubuntu 16.06 x32. I removed the profile.ini and started Firefox so that it will show the migration wizard. I canceled the migration and the bookmark toolbar wasn't activated by default. I think this bug is fixed.

Comment on attachment 8926763 [details] Bug 1415692 - Show the bookmark toolbar in new profiles with > 3 bookmarks on it. As m-r is 58 now, remove approval‑mozilla‑release flag.