Closed Bug 1295410 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file)

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 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+
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
Blocks: 1295510
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: nobody → markh
https://hg.mozilla.org/mozilla-central/rev/714f26001d3e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: