Closed
Bug 478035
Opened 15 years ago
Closed 6 years ago
change initRoots to check for roots validity
Categories
(Toolkit :: Places, defect, P1)
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.
Reporter | ||
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•6 years ago
|
||
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 | ||
Updated•6 years ago
|
Assignee: nobody → standard8
Status: NEW → ASSIGNED
Priority: P3 → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 5•6 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 6•6 years ago
|
||
mozreview-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)
Reporter | ||
Comment 7•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 11•6 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•6 years ago
|
||
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
Comment 20•6 years ago
|
||
bugherder |
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: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•