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

Assignee

Description

•

8 years ago

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

Richard Newman [:rnewman]

Updated

•

8 years ago

Blocks: 1317096

Lina Butler [:lina]

Assignee

Updated

•

8 years ago

Blocks: 1302288

status-firefox51: --- → unaffected

status-firefox52: --- → affected

status-firefox53: --- → affected

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/93770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93770/

Thom Chiovoloni [:tcsc] (ex-moco)

Comment 2

•

8 years 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+

Lina Butler [:lina]

Assignee

Comment 3

•

8 years 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).

Richard Newman [:rnewman]

Comment 4

•

8 years ago

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 on attachment 8811852 [details]
Bug 1318414 - Default to empty strings for titles and parent titles if not set.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93770/diff/1-2/

Richard Newman [:rnewman]

Comment 6

•

8 years 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+

Lina Butler [:lina]

Assignee

Updated

•

8 years ago

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 on attachment 8811852 [details]
Bug 1318414 - Default to empty strings for titles and parent titles if not set.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93770/diff/2-3/

Pulsebot

Comment 8

•

8 years 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

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 9

•

8 years ago

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)

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93770/diff/3-4/

Lina Butler [:lina]

Assignee

Comment 11

•

8 years ago

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)

Pulsebot

Comment 12

•

8 years 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

Carsten Book [:Tomcat]

Comment 13

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/cac405d88a11

Status: ASSIGNED → RESOLVED

Closed: 8 years ago

status-firefox53: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 53

Lina Butler [:lina]

Assignee

Comment 14

•

8 years ago

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?

Julien Cristau [:jcristau]

Comment 15

•

8 years ago

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+

Carsten Book [:Tomcat]

Comment 16

•

8 years ago

bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/0532e792808c

status-firefox52: affected → fixed

Richard Newman [:rnewman]

Comment 17

•

8 years ago

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.