1452660 - bookmarks toolbar only showing first item

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: Screenshot from 2018-04-09 11-20-01.png

I upgraded to the latest nightly (61.0a1 (2018-04-09) (64-bit)) and the Bookmarks Toolbar only shows the first item (I'll attach a screenshot).

I checked the Bookmarks Manager and the toolbar has all the bookmarks in it still. It's just that only the first one shows up in the toolbar.

It was fine yesterday with the 2018-04-07 nightlies. Seems like it broke today.

I'm on Ubuntu Linux 17.10. I haven't tested on other operating systems.

Comment 1

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: Screenshot from 2018-04-09 11-27-04.png

Here's a better screenshot. It shows the Bookmarks Toolbar data in the Bookmarks side-panel tab as well as the Bookmarks Toolbar which only shows the first item.

Comment 2

[Marco Bonardo [:mak]]

anything in the Browser Console when you launch the browser?
Did you customize your bookmarks toolbar, like adding something on the left/right of the default element?

Comment 3

[Will Kahn-Greene [:willkg] ET needinfo? me]

I'm not sure how to customize the bookmarks toolbar, so I don't think I did that.

Not to be coy, but there are a million things in the browser console on startup. Can you give me a filter that would help winnow it down to something helpful?

Comment 4

[Will Kahn-Greene [:willkg] ET needinfo? me]

I went back to 61.0a1 (2018-04-01) (64-bit), but didn't change my profile data. I still have the problem.

If I start with a fresh profile, then the browser toolbar shows both "Getting started" and "Most visited" default tabs.

I tried disabling the extensions I've got and that didn't help.

I'm not sure what else to try, but this sounds like it's a problem with my profile/setup. I'm not sure why it just started happening when I updated to the 2018-04-09 nightly.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Will Kahn-Greene [:willkg] ET needinfo? me from comment #3)
> Not to be coy, but there are a million things in the browser console on
> startup. Can you give me a filter that would help winnow it down to
> something helpful?

any Error involving browserPlacesViews.js or browser.js. IF you open the console immediately after browser startup and filter only on Errors there shouldn't be much.

Did you ever modify userChrome.css?

Comment 6

[Marco Bonardo [:mak]]

if you backup your bookmarks to json (from the Library window) and import that into a new profile, can you reproduce the problem?
If so, it would be of great help for debugging to get (even privately through mail) that json.

Comment 7

[Will Kahn-Greene [:willkg] ET needinfo? me]

I don't have a userChrome.css file in my profile. I don't see any browser.js or browserPlacesViews.js lines in the browser console at startup.

I backed up my bookmarks. I started a browser with nightly 2018-04-09 with a fresh profile and imported the bookmarks--the bookmarks toolbar displays fine.

I shut down the browser with the fresh profile and started it back up again--the bookmarks toolbar still displays fine.

So I'm not able to reproduce it with a fresh profile.

I backed up my profile for my primary browser. Then I restored the bookmarks in my primary browser--this wipes bookmarks and re-reads from the file. Initially the bookmarks toolbar showed nothing. On a lark, I restarted the browser and now the bookmarks toolbar looks fine.

So... I don't know what happened. Maybe something got corrupted somehow in whatever file stores bookmark information (whatever that might mean)?

My specific problem is solved, so that's super!

Is there anything I can do to help you here or should we just shrug and move on with our busy lives?

Comment 8

[Marco Bonardo [:mak]]

I'm not sure how to recover info about whatever happened. the only thing that comes to my mind is that, since you made a backup, you could try copying the old places.sqlite into a new profile and see if the problem happens. Again, if it happens, it would be great for me to debug that. Any way to reproduce this problem in a new profile with part of your original profile would be great.
Otherwise we'll probably just close as INCOMPLETE :(

My suspect is that on the toolbar you had a bookmark that somehow broke the view... But I would need to know more about that to be able to do anything.

Comment 9

[Will Kahn-Greene [:willkg] ET needinfo? me]

If I copy the places.sqlite file from the profile backup to the profile, then the bookmarks toolbar goes back to showing one item.

If I copy the places.sqlite file from the profile backup into the fresh profile, that causes the bookmarks toolbar to show only one item.

Do you want my places.sqlite file to look into it further?

Comment 10

[Marco Bonardo [:mak]]

it would be great if you could compress it, put it on https://send.firefox.com/ and send an expiring link to me: mak AT mozilla.com
Note it contains some of your private data, I won't make any bad use of it, but I must be sure you understand that.

Comment 11

[Marco Bonardo [:mak]]

OK, so the problem is that on the toolbar you had a bookmark with an empty url... I have no idea how that was created.
It looks like it was a tag query, maybe some of the recent rewrites ended up creating this empty url.
Do you have Sync connected to this device?

When the view code hits PlacesUIUtils.guessUrlSchemeForUI, it throws, and breaks the view.  It should probably not throw.

I'll investigate this deeper to ensure the recent migration may not break some kind of broken tag queries.

Comment 12

[Will Kahn-Greene [:willkg] ET needinfo? me]

I have no idea how that showed up, either. The issue is broken on my primary laptop which runs nightly. I do use sync and it's set up to sync bookmarks. I have a bunch of devices running Firefox release connected. I don't actually bookmark much these days. I do accidentally press ctrl-d periodically, though.

Anyhow, let me know if there's anything else I can do to help!

Comment 13

[Marco Bonardo [:mak]]

OK, I found the problem:
"UPDATE moz_places "
       "SET url = 'place:tag=' || ( "
          "SELECT title FROM moz_bookmarks "
          "WHERE id = CAST(get_query_param(substr(url, 7), 'folder') AS INT) "


So, this was a query pointing to a non existing folder and "string" || NULL returns NULL instead of "string".

Comment 14

[Marco Bonardo [:mak]]

Attachment: Bug 1452660 - Tag queries pointing to an invalid folder are being rewritten as empty urls.

Review commit: https://reviewboard.mozilla.org/r/235732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235732/

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8967077 [details]
Bug 1452660 - Tag queries pointing to an invalid folder are being rewritten as empty urls.

https://reviewboard.mozilla.org/r/235732/#review241716

::: toolkit/components/places/Database.cpp:1249
(Diff revision 1)
>          rv = MigrateV46Up();
>          NS_ENSURE_SUCCESS(rv, rv);
>        }
>  
> -      // Firefox 61 uses schema version 46.
> +      if (currentSchemaVersion < 47) {
> +        rv = MigrateV47Up();

Is it worth avoiding Migrate47Up if Migrade46Up has just been executed?

Just thinking that for beta/release users they shouldn't need to do 47... or do we want to run it anyway just in case?

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8967077 [details]
Bug 1452660 - Tag queries pointing to an invalid folder are being rewritten as empty urls.

https://reviewboard.mozilla.org/r/235732/#review241720

Looks good, just the one comment to consider.

Comment 17

[Marco Bonardo [:mak]]

(In reply to Mark Banner (:standard8) from comment #15)
> Is it worth avoiding Migrate47Up if Migrade46Up has just been executed?
> 
> Just thinking that for beta/release users they shouldn't need to do 47... or
> do we want to run it anyway just in case?

Per IRC discussion, I think we'd better just run it for everyone, you noticed null url errors on Sentry even before this, and since we're now legacy-addons free, I think it's a good time to do this simple cleanup.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a061dfd9f1b1
Tag queries pointing to an invalid folder are being rewritten as empty urls. r=standard8

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a061dfd9f1b1