1389717 - Ensure `HistoryStore#changeItemID` waits for `fetchURLInfoForGuid`

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Lina Butler (ex-Mozilla)]

I caught this looking over our `PlacesSyncUtils` calls. Property access binds tighter than `await`, so `await PlacesSyncUtils.history.fetchURLInfoForGuid(oldID).url` won't wait for `fetchURLInfoForGuid`.

Comment 1

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1389717 - Ensure `HistoryStore#changeItemID` waits for `fetchURLInfoForGuid`.

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

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8896529 [details]
Bug 1389717 - Ensure `HistoryStore#changeItemID` waits for `fetchURLInfoForGuid`.

https://reviewboard.mozilla.org/r/167794/#review172996

I don't think this function is called, but we should fix this either way. LGTM!

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8896529 [details]
Bug 1389717 - Ensure `HistoryStore#changeItemID` waits for `fetchURLInfoForGuid`.

https://reviewboard.mozilla.org/r/167794/#review173008

::: services/sync/modules/engines/history.js:163
(Diff revision 1)
>    },
>  
>    async changeItemID(oldID, newID) {
> -    this.setGUID(await PlacesSyncUtils.history.fetchURLInfoForGuid(oldID).url, newID);
> +    let info = await PlacesSyncUtils.history.fetchURLInfoForGuid(oldID);
> +    if (info) {
> +      this.setGUID(info.url, newID);

I don't really mind, but I wonder if it might be better to have the code implicitly throw when info is falsey rather than silently do nothing?

Comment 4

[Lina Butler (ex-Mozilla)]

Comment on attachment 8896529 [details]
Bug 1389717 - Ensure `HistoryStore#changeItemID` waits for `fetchURLInfoForGuid`.

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d419fafe04f
Ensure `HistoryStore#changeItemID` waits for `fetchURLInfoForGuid`. r=markh,tcsc

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9d419fafe04f

Comment 7

[Lina Butler (ex-Mozilla)]

WONTFIXing for 56, since this function isn't ever called.