1127763 - Some database might be missing the favicons guid index

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

CREATE_IDX_MOZ_FAVICONS_GUI is invoked on migration, but not on creation, that means new profiles might be missing this index.

Comment 1

[Timur Valeev]

Hello. 
Is it needed to add ExecuteSimpleSQL(CREATE_IDX_MOZ_FAVICONS_GUID) to Database::InitSchema method in the else conditional branch (when is creating all tables and indices for new database)?

Comment 2

[Marco Bonardo [:mak]]

(In reply to Timur Valeev from comment #1)
> Hello. 
> Is it needed to add ExecuteSimpleSQL(CREATE_IDX_MOZ_FAVICONS_GUID) to
> Database::InitSchema method in the else conditional branch (when is creating
> all tables and indices for new database)?

Yes, that would fix the reported problem.

But it's not enough, indeed we would still have databases in the wild missing this index.
For that, we also need a migration step, that means
* bump DATABASE_SCHEMA_VERSION in Database.h
* add a MigrateV29Up method that tries to create the index to Database.h/cpp
* bump CURRENT_SCHEMA_VERSION in toolkit/components/places/tests/head common.js
* add a places_v29.sqlite file into toolkit/components/places/tests/migration/ (for this it's enough to create a new profile with the bumped up compiled version, then using either sqlite3.exe or Sqlite Manager Add-on run VACUUM on the places.sqlite file from that profile). Eventually I can take care of creating the db if that's a problem.
* run tests with ./mach xpcshell-test toolkit/components/places/

Comment 3

[Timur Valeev]

Thank you for the information. Some of these days I'll try to create the modifications.

Comment 4

[Marco Bonardo [:mak]]

ok, let me assign the bug so you can track it in the bugzilla dashboard.

Comment 5

[Timur Valeev]

Attachment: bug-1127763-fix-0.1.patch

Hi. I created the patch. But I'm not sure about places_v29.sqlite file. 
I used "Compact database" command from Sqlite Manager Add-on to new places.sqlite file but the size remains the same.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8610171 [details] [diff] [review]
bug-1127763-fix-0.1.patch

Review of attachment 8610171 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks good.

I have a couple minor comments:

::: toolkit/components/places/Database.cpp
@@ +753,5 @@
>  
> +      if (currentSchemaVersion < 29) {
> +        rv = MigrateV29Up();
> +        NS_ENSURE_SUCCESS(rv, rv);
> +      }

please add a // Firefox 41 uses schema version 29. comment just below this migration (see previous ones)

@@ +1619,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return NS_OK;
> +
> +}

please remove the newline before the closing brace

Comment 7

[Marco Bonardo [:mak]]

(In reply to Timur Valeev from comment #5)
> I used "Compact database" command from Sqlite Manager Add-on to new
> places.sqlite file but the size remains the same.

The db file should be about 1MB after vacuuming it.

Comment 8

[Marco Bonardo [:mak]]

also, please edit toolkit/components/places/tests/migration/xpcshell.ini and add the places sqlite file to it

Comment 9

[Timur Valeev]

Ok. I will change my patch. Yes db file is about 1MB (like places_v27.sqlite or places_v28.sqlite).

Comment 10

[Timur Valeev]

Attachment: bug-1127763-fix-0.2.patch

New patch

Comment 11

[Marco Bonardo [:mak]]

Thank you for your contribution, much appreciated!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1efb7424460c

Comment 12

[Timur Valeev]

Thank you too. Could you recommend some interesting issues? :)

Comment 13

[Marco Bonardo [:mak]]

Sure, thank you for your help!
I'm looking at the list of bugs I'm mentoring, but you can find more mentored bugs at https://developer.mozilla.org/en-US/docs/Introduction#Step_2_-_Find_something_to_work_on if you can't see anything of your interest here.

if you want something very involving (Will tak some time), there is:
Bug 1039200 - Add Migration for new tag tables
it has patches from another contributor that stopped working on it due to his real job taking time. The patches should likely be updated to the current codebase, the second part patch needs comments addressed and then it needs a third part as explained in https://bugzilla.mozilla.org/show_bug.cgi?id=1039200#c21
note: in the meanwhile we introduced new APIs, so the API built there might need a little bit of refreshing to make it coherent.

This is still in cpp world, should not take much time:
Bug 1166166 - use mAsyncExecutionThreadIsAlive to shrink memory on the async thread when possible.

These should be easier, in js:
Bug 1092607 - Intermittent test_PlacesUtils_asyncGetBookmarkIds.js
Bug 1167229 - Make nsBrowserGlue instantiations use nsISupports where nsIBrowserGlue is not needed
Bug 416611 - support importing tags from bookmarks.html exports (del.icio.us and others)
Bug 820876 - Use bookmarks-observer category for the tagging service
Bug 962999 - Because of encoding error, users can not import bookmarks which exported from 360 browser

Apart the first one that might require to dedicate some more time, the others should be feasible in spare time.

Comment 14

[Timur Valeev]

Thank you for the information!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5670c0c09c2c

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5670c0c09c2c