Closed Bug 1318414 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox51 --- unaffected
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

Attachments

(1 file)

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
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+
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 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
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)
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)
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
https://hg.mozilla.org/mozilla-central/rev/cac405d88a11
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
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+
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.