Enforce that all places items must have non-null GUIDs

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
Places
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: markh, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Places goes to great lengths to ensure all items have GUIDs, and has a "unique" index on the column. It has DEBUG-only code in Database::Shutdown() that asserts if it finds any NULL items, but best I can tell, this isn't actually enforced.

Doing this would allow Sync to remove some complexity around ensuring items have GUIDs when best I can tell, that's already true, just not enforced.

ISTM this might be as easy as changing the GUID column to be "NOT NULL"?

Mak, is there something I'm missing here?
(Reporter)

Updated

a year ago
Duplicate of this bug: 1294290
(In reply to Mark Hammond [:markh] from comment #0)
> ISTM this might be as easy as changing the GUID column to be "NOT NULL"?

The problem is that we can't do that, since Sqlite doesn't allow to alter a column to add not null, and you can't add a new not null column with a null value. Even if we had specified a default value and NOT NULL when we added the column, we could still end up with bogus entries having the default value, as today we can end up with NULL.

That said, I think today the only way to end up with a NULL guid is if a user/add-on runs a custom query on the database and doesn't set it, that while is bad, I'm not sure we should care about it, since he can cause far worse issues that way. The debug check is there just as an additional check so we can't break db creation.
I sort of suspect the Sync guids management comes from when guids were annotations, and then it was far more likely for them to be missing.
(In reply to Marco Bonardo [::mak] from comment #3)
> I sort of suspect the Sync guids management comes from when guids were
> annotations, and then it was far more likely for them to be missing.

Yes - and thanks for the DB schema explanation. It sounds like we should (generally) be free to throw in that case - we could easily do more damage by ignoring that state, adding a GUID, then carrying on like nothing happened.
ack - to clarify - we should change the scope of this bug to be that PlacesSyncUtils should throw when it sees an item without a GUID. The other option is a simple "wontfix". Kit/Mak, what do you think?
yeah, I think "discarding" guid-missing entries should be fine. Whatever discarding means for Sync.
Kit, we have a comment "This method can be removed and replaced with `PlacesUtils.promiseItemGuid` once bug 1294291 lands" and I think it's reasonable to just make that change and allow any exception caused by a null GUID to percolate up the stack - bug 1297941 notwithstanding obviously ;) WDYT?
Flags: needinfo?(kcambridge)
SGTM!
Depends on: 1297941
Flags: needinfo?(kcambridge)
Comment hidden (mozreview-request)
(Reporter)

Comment 10

a year ago
mozreview-review
Comment on attachment 8784903 [details]
Bug 1294291 - Remove missing GUID handling code from Sync and Places.

https://reviewboard.mozilla.org/r/74218/#review72264

Thanks - less is more! :)
Attachment #8784903 - Flags: review?(markh) → review+
Comment hidden (mozreview-request)

Comment 12

a year ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d090231db437
Remove missing GUID handling code from Sync and Places. r=markh

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d090231db437
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.