Closed Bug 478035 Opened 12 years ago Closed 2 years ago

change initRoots to check for roots validity

Categories

(Toolkit :: Places, defect, P1)

x86
Windows Vista
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: mak, Assigned: standard8)

References

Details

Attachments

(3 files)

follow-up to bug 477739

We should fix initRoots to check if roots folders effectively exist, and take corrective actions in case any issue is detected on startup.
Priority: -- → P3
Blocks: 1428519
Current plan as discussed with Marco on IRC:

- In either the new database or the existing profile during InitSchema, we should call a function, say CheckRoots.
- The function checks for the existence of the built-in roots, and creates them if missing.
- For a new profile we can create them with predefined IDs, so we don't need to read them again.
- The existing EnsureRoots function goes away as a result.

The slight downside is that in the normal case, we'll be reading the ids slightly earlier when the database is read, but we're likely to be reading them within a few seconds anyway.
Assignee: nobody → standard8
Status: NEW → ASSIGNED
Priority: P3 → P1
Comment on attachment 8969229 [details]
Bug 478035 - Move the code for obtaining the places built-in folder ids from nsNavBookmarks to Database.

https://reviewboard.mozilla.org/r/237962/#review243768

::: toolkit/components/places/Database.cpp:1396
(Diff revision 1)
> +  int64_t id;
> +  while (NS_SUCCEEDED(stmt->ExecuteStep(&hasResult)) && hasResult) {
> +    rv = stmt->GetUTF8String(0, guid);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    rv = stmt->GetInt64(1, &id);

You could probably just do:
mRootId = stmt->AsInt64(1)
and so on for the other roots.

::: toolkit/components/places/nsNavBookmarks.h:286
(Diff revision 1)
>  
>    nsMaybeWeakPtrArray<nsINavBookmarkObserver> mObservers;
>  
>    int64_t TagsRootId() {
> -    nsresult rv = EnsureRoots();
> -    NS_ENSURE_SUCCESS(rv, -1);
> +    int64_t id = mDB->GetTagsFolderId();
> +    NS_ENSURE_TRUE(id > 0, -1);

I suppose we could init them to -1 (since they are signed integers) and just return id here and in the other database methods
Attachment #8969229 - Flags: review?(mak77) → review+
Comment on attachment 8969230 [details]
Bug 478035 - Re-create the built-in root folders if they are missing in the places database.

https://reviewboard.mozilla.org/r/237964/#review243778

::: commit-message-3c1b6:1
(Diff revision 1)
> +Bug 478035 - Re-create the built-in folders if they are missing in the places database. r?mak

build-in root folders

::: toolkit/components/places/Database.h:309
(Diff revision 1)
>    nsresult CheckRoots();
>  
>    /**
>     * Creates bookmark roots in a new DB.
>     */
> -  nsresult CreateBookmarkRoots();
> +  nsresult EnsureBookmarkRoots(const int32_t existingRoots);

looks like you renamed it to existingRootsCount elsewhere

::: toolkit/components/places/Database.cpp:13
(Diff revision 1)
>  #include "mozilla/ScopeExit.h"
>  
>  #include "Database.h"
>  
>  #include "nsIAnnotationService.h"
> -#include "nsINavBookmarksService.h"
> +#include "nsNavBookmarks.h"

nit: move it down with other concrete objects includes, since no more an interface include

::: toolkit/components/places/Database.cpp:1373
(Diff revision 1)
>  Database::CheckRoots()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    nsCOMPtr<mozIStorageStatement> stmt;
>    nsresult rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(

can we bypass this querying when we know that the database has just been created?

::: toolkit/components/places/Database.cpp:1444
(Diff revision 1)
> +                    0, mRootId);
> +
> -  if (NS_FAILED(rv)) return rv;
> +    if (NS_FAILED(rv)) return rv;
> +  }
> +
> +  int32_t position = existingRootCount;

this can be wrong, for example let's suppose the root at position 2 is missing, we have 0,1,3,4 and the count is 4. We'll create the first root at position 4, while it should be at 5.
maybe createRoot should just get the position from the db using a MAX subselect... or you should select all the current positions and store the max one when you loop through existing roots.

Regardless, there can be holes, but PlacesDBUtils can fix those later.

::: toolkit/components/places/tests/unit/test_missing_builtin_folders.js:26
(Diff revision 1)
> +  PlacesUtils.bookmarks.tagsGuid,
> +  PlacesUtils.bookmarks.unfiledGuid,
> +];
> +
> +add_task(async function setup() {
> +  // This file has the toolbar and

and...?

::: toolkit/components/places/tests/unit/test_missing_builtin_folders.js:74
(Diff revision 1)
> +
> +    Assert.equal(root.guid, guid, "GUIDs should match");
> +    Assert.equal(root.parentGuid, PlacesUtils.bookmarks.rootGuid,
> +      "Should have the correct parent GUID");
> +    Assert.equal(root.type, PlacesUtils.bookmarks.TYPE_FOLDER,
> +      "Should have the correct type");

Can we also add some checks for promiseItemGuid and promiseItemId around these? to check they return meaningful values

::: toolkit/components/places/tests/unit/test_missing_root_folder.js:20
(Diff revision 1)
> +  PlacesUtils.bookmarks.tagsGuid,
> +  PlacesUtils.bookmarks.mobileGuid,
> +];
> +
> +add_task(async function setup() {
> +  // This file has the toolbar and

and... :)

::: toolkit/components/places/tests/unit/test_missing_root_folder.js:57
(Diff revision 1)
> +  Assert.greaterOrEqual(rows[0].getResultByName("id"), 1,
> +    "Should have a valid root Id");
> +  Assert.equal(rows[0].getResultByName("parent"), 0,
> +    "Should have a parent of id 0");
> +  Assert.equal(rows[0].getResultByName("type"), PlacesUtils.bookmarks.TYPE_FOLDER,
> +    "Should have a type of folder");

ditto for promiseItemGuid/Id
Attachment #8969230 - Flags: review?(mak77)
Comment on attachment 8969231 [details]
Bug 478035 - Remove the now unnecessary code that attempts to fix a missing places root.

https://reviewboard.mozilla.org/r/237966/#review243790
Attachment #8969231 - Flags: review?(mak77) → review+
Comment on attachment 8969230 [details]
Bug 478035 - Re-create the built-in root folders if they are missing in the places database.

https://reviewboard.mozilla.org/r/237964/#review244214

::: toolkit/components/places/Database.cpp:1481
(Diff revision 2)
> -  if (NS_FAILED(rv)) return rv;
> +    if (NS_FAILED(rv)) return rv;
> +    position++;
> +  }
>  
> +  if (mMobileRootId < 1) {
> -  int64_t mobileRootId = CreateMobileRoot();
> +    int64_t mobileRootId = CreateMobileRoot();

the position in CreateMobileRoot looks broken, it just counts, we could probably pass position to it
Attachment #8969230 - Flags: review?(mak77) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 6a5016ade63765fa5731bfbffc01a6f534181f11 -d a10d637e1678: rebasing 459535:6a5016ade637 "Bug 478035 - Move the code for obtaining the places built-in folder ids from nsNavBookmarks to Database. r=mak"
rebasing 459536:357f58473b55 "Bug 478035 - Re-create the built-in root folders if they are missing in the places database. r=mak"
merging toolkit/components/places/tests/head_common.js
rebasing 459537:11b2f3c9e8bc "Bug 478035 - Remove the now unnecessary code that attempts to fix a missing places root. r=mak" (tip)
merging toolkit/components/places/PlacesDBUtils.jsm
merging toolkit/components/places/tests/unit/test_preventive_maintenance.js
warning: conflicts while merging toolkit/components/places/tests/unit/test_preventive_maintenance.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db3d3eb4b46c
Move the code for obtaining the places built-in folder ids from nsNavBookmarks to Database. r=mak
https://hg.mozilla.org/integration/autoland/rev/fe5121c793e4
Re-create the built-in root folders if they are missing in the places database. r=mak
https://hg.mozilla.org/integration/autoland/rev/e726894fdd13
Remove the now unnecessary code that attempts to fix a missing places root. r=mak
https://hg.mozilla.org/mozilla-central/rev/db3d3eb4b46c
https://hg.mozilla.org/mozilla-central/rev/fe5121c793e4
https://hg.mozilla.org/mozilla-central/rev/e726894fdd13
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.