Sync sometimes passes "new String()" holding a GUID instead of a native string, causing PlacesUtils to reject it.

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Sync
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

unspecified
Firefox 51
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)

(Assignee)

Description

2 years ago
Items in the bookmark's guidMap() are created via |new String()| so that additional properties can be added to the object. Now that PlacesSyncUtils exists, we validate GUIDs via PlacesUtils.isValidGuid() which rejects such objects.

Best I can tell, this breaks our duplicate handling. Sadly there are no tests for this in the tree currently, but I'm working on some in bug 1276823.
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8781346 [details]
Bug 1295410 - ensure we don't pass string objects for GUIDs to PlacesSyncUtils.

https://reviewboard.mozilla.org/r/71762/#review69352

Ah, good catch! I fixed another occurence of this in http://searchfox.org/mozilla-central/rev/9ec085584d7491ddbaf6574d3732c08511709172/services/sync/modules/engines/bookmarks.js#789, but not this one.
Attachment #8781346 - Flags: review?(kcambridge) → review+

Comment 3

2 years ago
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/714f26001d3e
ensure we don't pass string objects for GUIDs to PlacesSyncUtils. r=kitcambridge
Also, this seems familiar; I think it's come up before for Thom, too. It'd be great to use POJOs instead of relying on this JS quirk. Filed bug 1295510.
(Assignee)

Updated

2 years ago
Assignee: nobody → markh

Comment 5

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