Default to empty strings for titles and parent titles if not set

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Sync
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kitcambridge, Assigned: kitcambridge)

Tracking

unspecified
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 unaffected, firefox52 fixed, firefox53 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

From IRC:

10:20 <•rnewman> kitcambridge, markh, tcsc: also, did we recently stop uploading parentName for the roots?
10:21 because all records are supposed to have a parentName, and iOS is refusing them…
10:22 <•rnewman> my guess is that a Places item has an undefined parentTitle, so when we JSON-serialize our record, that field is omitted.
10:25 <•rnewman> should be the empty string if the parent is the places root
Blocks: 1317096
Blocks: 1302288
status-firefox51: --- → unaffected
status-firefox52: --- → affected
status-firefox53: --- → affected
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
Comment on attachment 8811852 [details]
Bug 1318414 - Default to empty strings for titles and parent titles if not set.

https://reviewboard.mozilla.org/r/93770/#review93916

r=me if the answer to my question below is yes.

::: toolkit/components/places/PlacesSyncUtils.jsm:313
(Diff revision 1)
>        default:
>          throw new Error(`Unknown bookmark kind: ${kind}`);
>      }
>  
>      // Sync uses the parent title for de-duping.
>      if (bookmarkItem.parentGuid) {

Is this always set when we would want parentTitle to be present?
Attachment #8811852 - Flags: review?(tchiovoloni) → review+
(Assignee)

Comment 3

a year ago
mozreview-review-reply
Comment on attachment 8811852 [details]
Bug 1318414 - Default to empty strings for titles and parent titles if not set.

https://reviewboard.mozilla.org/r/93770/#review93916

> Is this always set when we would want parentTitle to be present?

Yes. The Places root won't have a `parentTitle`, but that's handled specially in https://github.com/mozilla-mobile/firefox-ios/blob/903c01e603b5d88ae6ed753bcc947af68a2704ed/Sync/BookmarkPayload.swift#L499-L502. Richard, is my understanding correct? (We also shouldn't be uploading Places roots anymore).
Clients do tend to handle the places root specially, both outgoing and incoming. It's allowed to not have a parentName, and iOS won't complain.

I'm not sure this change is correct.

let x = {"foo": null, "bar": undefined, "baz": "", "noo": "noo"};

function tt(param) {
  return param in x ? x[param] : "";
}

console.log(tt("foo"));
=> null
console.log(tt("bar"));
=> undefined
console.log(tt("baz"));
=> ""
console.log(tt("noo"));
=> "noo"

I think you actually want:

  item.parentTitle = parent["title"] || "";


I also think you might need to change walkBookmarksRoots, and you should consider whether this change belongs in PlacesSyncUtils or in fromSyncBookmark in bookmarks.js.
Comment hidden (mozreview-request)

Comment 6

a year ago
mozreview-review
Comment on attachment 8811852 [details]
Bug 1318414 - Default to empty strings for titles and parent titles if not set.

https://reviewboard.mozilla.org/r/93770/#review94218

Belt and braces!
Attachment #8811852 - Flags: review?(rnewman) → review+
Summary: `PlacesSyncUtils.bookmarks.fetch` should return an empty string for the parent title if not set → Default to empty strings for titles and parent titles if not set
Comment hidden (mozreview-request)

Comment 8

a year ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c644b01be649
Default to empty strings for titles and parent titles if not set. r=rnewman,tcsc
Backed out for failing xpcshell test test_bookmark_store.js:

https://hg.mozilla.org/integration/autoland/rev/74d7cd944ce0d5bc723d4e3a8ed1865ca819a7f8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c644b01be6496b3903d47f2842ba018188b08777
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=6893280&repo=autoland

19:03:14     INFO -  PROCESS | 21121 | console.debug: PushDB:
19:03:14     INFO -  PROCESS | 21121 |   put: Request successful. Updated record
19:03:14     INFO -  PROCESS | 21121 |   5d90f8b1-c176-486f-8e2f-17f7a8d248f0
19:03:14     INFO -  TEST-PASS | dom/push/test/xpcshell/test_updateRecordNoEncryptionKeys_ws.js | test_with_data_enabled - [test_with_data_enabled : 75] Should generate public keys for new records - {} == true
19:03:14     INFO -  PROCESS | 21121 | console.debug: PushDB:
19:03:14     INFO -  PROCESS | 21121 |   getByKeyID()
19:03:14     INFO -  PROCESS | 21121 | console.debug: PushDB:
19:03:14     INFO -  PROCESS | 21121 |   update: Update successful
19:03:14     INFO -  PROCESS | 21121 |   0d8886b9-8da1-4778-8f5d-1cf93a877ed6
19:03:14     INFO -  PROCESS | 21121 | Object
19:03:14     INFO -  PROCESS | 21121 |     - pushEndpoint = https://example.org/push/key
19:03:14     INFO -  PROCESS | 21121 |     - scope = https://example.com/page/2
19:03:14     INFO -  PROCESS | 21121 |     - originAttributes =
19:03:14     INFO -  PROCESS | 21121 |     - pushCount = 0
19:03:14     INFO -  PROCESS | 21121 |     - lastPush = 0
19:03:14     INFO -  PROCESS | 21121 |     - p256dhPublicKey = ArrayBuffer {}
19:03:14     INFO -  PROCESS | 21121 |     - p256dhPrivateKey = {"crv":"P-256","d":"0ZvmjySdd3DQHYkqPpk-AJVnotW-YGpggnx92ztn7KI","ext":true,"key_ops":["deriveBits"],"kty":"EC","x":"sJVWMktkgVZ2WGpwxwM753HQu6IcKsebqgb3Oihv3dQ","y":"BXc0RkdQRSHxa6mIUC3ZSjhcLTq6jYzezGtc3sLytD0"}
19:03:14     INFO -  PROCESS | 21121 |     - authenticationSecret = Uint8Array {"0":174,"1":75,"2":107,"3":143,"4":146,"5":89,"6":186,"7":7,"8":4,"9":160,"10":46,"11":3,"12":3,"13":108,"14":250,"15":244}
19:03:14     INFO -  PROCESS | 21121 |     - systemRecord = false
19:03:14     INFO -  PROCESS | 21121 |     - appServerKey = undefined
19:03:14     INFO -  PROCESS | 21121 |     - recentMessageIDs = undefined
19:03:14     INFO -  PROCESS | 21121 |     - quota = Infinity
19:03:14     INFO -  PROCESS | 21121 |     - ctime = 0
19:03:14     INFO -  PROCESS | 21121 |     - channelID = 0d8886b9-8da1-4778-8f5d-1cf93a877ed6
19:03:14     INFO -  PROCESS | 21121 |     - version = undefined
19:03:14     INFO -  PROCESS | 21121 | console.debug: PushDB:
19:03:14     INFO -  PROCESS | 21121 |   getByKeyID: Got record
19:03:14     INFO -  PROCESS | 21121 | Object
19:03:14     INFO -  PROCESS | 21121 |     - pushEndpoint = https://example.org/push/no-key/1
19:03:14     INFO -  PROCESS | 21121 |     - scope = https://example.com/page/1
19:03:14     INFO -  PROCESS | 21121 |     - originAttributes =
19:03:14     INFO -  PROCESS | 21121 |     - pushCount = 0
19:03:14     INFO -  PROCESS | 21121 |     - lastPush = 0
19:03:14     INFO -  PROCESS | 21121 |     - p256dhPublicKey = undefined
19:03:14     INFO -  PROCESS | 21121 |     - p256dhPrivateKey = undefined
19:03:14     INFO -  PROCESS | 21121 |     - authenticationSecret = undefined
19:03:14     INFO -  PROCESS | 21121 |     - systemRecord = false
19:03:14     INFO -  PROCESS | 21121 |     - appServerKey = undefined
19:03:14     INFO -  PROCESS | 21121 |     - recentMessageIDs = undefined
19:03:14     INFO -  PROCESS | 21121 |     - quota = Infinity
19:03:14     INFO -  PROCESS | 21121 |     - ctime = 0
19:03:14     INFO -  PROCESS | 21121 |     - channelID = eb18f12a-cc42-4f14-accb-3bfc1227f1aa
19:03:14     INFO -  PROCESS | 21121 |     - version = undefined
19:03:14  WARNING -  TEST-UNEXPECTED-FAIL | dom/push/test/xpcshell/test_updateRecordNoEncryptionKeys_ws.js | test_with_data_enabled - [test_with_data_enabled : 78] Should add public key to partial record - "undefined" == true
19:03:14     INFO -      /builds/slave/test/build/tests/xpcshell/tests/dom/push/test/xpcshell/test_updateRecordNoEncryptionKeys_ws.js:test_with_data_enabled:78
19:03:14     INFO -      _run_next_test@/builds/slave/test/build/tests/xpcshell/head.js:1566:9
19:03:14     INFO -      do_execute_soon/<.run@/builds/slave/test/build/tests/xpcshell/head.js:713:9
19:03:14     INFO -      _do_main@/builds/slave/test/build/tests/xpcshell/head.js:210:5
19:03:14     INFO -      _execute_test@/builds/slave/test/build/tests/xpcshell/head.js:545:5
19:03:14     INFO -      @-e:1:1
Flags: needinfo?(kcambridge)
Comment hidden (mozreview-request)
Sorry about that; I think that test didn't run in https://treeherder.mozilla.org/#/jobs?repo=try&revision=27f82cac4fdb5f6129d8fec78c9f55b43b114332.

Fixed failure and requested xpcshell tests for all platforms in https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2ac122ec91b1069109a7895c0a26b5162a3bcc4.
Flags: needinfo?(kcambridge)

Comment 12

a year ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cac405d88a11
Default to empty strings for titles and parent titles if not set. r=rnewman,tcsc

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cac405d88a11
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Comment on attachment 8811852 [details]
Bug 1318414 - Default to empty strings for titles and parent titles if not set.

Approval Request Comment
[Feature/regressing bug #]: Bug 1302288, bug 1317096.
[User impact if declined]: Older versions of Firefox iOS won't sync Desktop bookmarks correctly.
[Describe test coverage new/current, TreeHerder]: Includes modified tests, https://treeherder.mozilla.org/#/jobs?repo=try&revision=827f54f307a5.
[Risks and why]: Low risk; this is an additive change that adds an explicitly empty string property instead of omitting it.
[String/UUID change made/needed]: None.
Attachment #8811852 - Flags: approval-mozilla-aurora?
Comment on attachment 8811852 [details]
Bug 1318414 - Default to empty strings for titles and parent titles if not set.

take this fix for bookmark sync with ios in aurora52
Attachment #8811852 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 16

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0532e792808c
status-firefox52: affected → fixed
I saw this happen for a non-root bookmark in Bug 1323461 Comment 1. The bookmark was created on October 18th. I assume it's fixed by this bug.

Still figuring out if this causes a catastrophic failure, and thus we need to weaken constraints on iOS to work around this.
You need to log in before you can comment on or make changes to this bug.