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?

(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?

SGTM!

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! :)

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