Closed Bug 1389717 Opened 6 years ago Closed 6 years ago

Ensure `HistoryStore#changeItemID` waits for `fetchURLInfoForGuid`

Categories

(Firefox :: Sync, enhancement, P1)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox55 --- unaffected
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

Attachments

(1 file)

Bug 1389717 - Ensure `HistoryStore#changeItemID` waits for `fetchURLInfoForGuid`.
59 bytes, text/x-review-board-request
tcsc
: review+
Details 
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`.
Summary: Fix `await` operator precedence in `History#changeItemID` → Ensure `HistoryStore#changeItemID` waits for `fetchURLInfoForGuid`
Attachment #8896529 - Flags: review?(markh) → review?(tchiovoloni)
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!
Attachment #8896529 - Flags: review?(tchiovoloni) → review+
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?
Attachment #8896529 - Flags: review+
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9d419fafe04f
Ensure `HistoryStore#changeItemID` waits for `fetchURLInfoForGuid`. r=markh,tcsc
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/9d419fafe04f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
WONTFIXing for 56, since this function isn't ever called.
status-firefox55: unaffected → wontfix
You need to log in before you can comment on or make changes to this bug.