Some database might be missing the favicons guid index

RESOLVED FIXED in Firefox 41

Status

()

Toolkit
Places
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mak, Assigned: Timur Valeev, Mentored)

Tracking

unspecified
mozilla41
Points:
2
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [good next bug][lang=cpp])

Attachments

(1 attachment, 1 obsolete attachment)

CREATE_IDX_MOZ_FAVICONS_GUI is invoked on migration, but not on creation, that means new profiles might be missing this index.
Points: --- → 2
Flags: qe-verify-
Flags: in-testsuite?
Flags: firefox-backlog+
Mentor: mak77@bonardo.net
Whiteboard: [good next bug][lang=cpp]
(Assignee)

Comment 1

3 years ago
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)?
Flags: needinfo?(mak77)
(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/
Flags: needinfo?(mak77)
(Assignee)

Comment 3

3 years ago
Thank you for the information. Some of these days I'll try to create the modifications.
ok, let me assign the bug so you can track it in the bugzilla dashboard.
Assignee: nobody → tvaleev
(Assignee)

Comment 5

3 years ago
Created attachment 8610171 [details] [diff] [review]
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.
Attachment #8610171 - Flags: review?(mak77)
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
Attachment #8610171 - Flags: review?(mak77) → feedback+
(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.
also, please edit toolkit/components/places/tests/migration/xpcshell.ini and add the places sqlite file to it
(Assignee)

Comment 9

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

Comment 10

3 years ago
Created attachment 8611182 [details] [diff] [review]
bug-1127763-fix-0.2.patch

New patch
Attachment #8610171 - Attachment is obsolete: true
Attachment #8611182 - Flags: review?(mak77)
Attachment #8611182 - Flags: review?(mak77) → review+
Thank you for your contribution, much appreciated!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1efb7424460c
Keywords: checkin-needed
Flags: in-testsuite? → in-testsuite-
(Assignee)

Comment 12

3 years ago
Thank you too. Could you recommend some interesting issues? :)
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.
(Assignee)

Comment 14

3 years ago
Thank you for the information!

Comment 15

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/5670c0c09c2c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5670c0c09c2c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Flags: in-testsuite- → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1169753
You need to log in before you can comment on or make changes to this bug.