Closed Bug 1412143 Opened 7 years ago Closed 7 years ago

Add `mozIAsyncLivemarks.invalidateCachedLivemarks`

Categories

(Toolkit :: Places, enhancement, P3)

Product:
Toolkit
Component:
Places
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

Attachments

(1 file)

Bug 1412143 - Add `mozIAsyncLivemarks.invalidateCachedLivemarks`.
59 bytes, text/x-review-board-request
mak
: review+
Details 
The Sync bookmark buffer in bug 1305563 updates the database in a single transaction. We can't use `mozIAsyncLivemarks.{add, remove}Livemark` from within the transaction, because these methods start their own transactions.

To simplify merging, the buffer sets the livemark feed and site URL annos in the transaction, then calls `invalidateCachedLivemarks` to clear the entire livemarks cache. The next `{add, remove, get}Livemark` call will repopulate the cache, with the new livemarks.
We could also have `insertTree` use this, instead of inserting livemarks separately.
(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #1)
> We could also have `insertTree` use this, instead of inserting livemarks
> separately.

That's an interesting suggestion for the future. In practice it's likely most users don't have any livemark, I don't think we have telemetry about that.
Comment on attachment 8922573 [details]
Bug 1412143 - Add `mozIAsyncLivemarks.invalidateCachedLivemarks`.

https://reviewboard.mozilla.org/r/193676/#review199058

I have a few questions.

::: toolkit/components/places/nsLivemarkService.js:118
(Diff revision 1)
> +          this._livemarksMap.set(livemark.guid, livemark);
> +        }
> +      }
> +      return func(this._livemarksMap);
> +    });
> +    this._promiseLivemarksMapReady = promise.then(() => {}, Cu.reportError);

sounds like a .catch(Cu.reportError)?

::: toolkit/components/places/nsLivemarkService.js:300
(Diff revision 1)
> +  invalidateCachedLivemarks() {
> +    if (this._reloadTimer) {
> +      this._reloading = false;
> +      this._reloadTimer.cancel();
> +      delete this._reloadTimer;
> +    }

Why is this in the invalidate call instead of just on shutdown?
What happens if something called reloadLivemark, and then something else invokes invalidateCachedLivemarks? Looks like the timer will never fire.

::: toolkit/components/places/nsLivemarkService.js:301
(Diff revision 1)
> +    if (this._reloadTimer) {
> +      this._reloading = false;
> +      this._reloadTimer.cancel();
> +      delete this._reloadTimer;
> +    }
> +    let promise = this._promiseLivemarksMapReady.then(() => {

Looks like something may end up working with broken data between the time we effectively change the data and the time we invalidate it here.
Though, I can't think of a simple solution.

::: toolkit/components/places/nsLivemarkService.js:307
(Diff revision 1)
> +      let livemarksMap = this._livemarksMap;
> +      this._livemarksMap = null;
> +      if (livemarksMap) {
> +        // Stop any ongoing network fetch.
> +        for (let livemark of livemarksMap.values()) {
> +          livemark.terminate();

pretty much same question as above, what happens if a fetch was ongoing and something invokes invalidate?
Maybe this should check _reloading and restart the timer (wouldn't solve the specific .terminate() call though), and shutdown should just be left alone?

::: toolkit/components/places/tests/unit/test_async_transactions.js:1000
(Diff revision 1)
>    }
>    async function redo() {
>      ensureUndoState([[removeTxn], [createLivemarkTxn]], 2);
>      await PT.redo();
>      ensureUndoState([[removeTxn], [createLivemarkTxn]], 1);
> -    await ensureBookmarksTreeRestoredCorrectly(originalInfo);
> +    await ensureBookmarksTreeRestoredCorrectlyExceptDates(originalInfo);

why these changes? is it because of the isAnno optimization?

::: toolkit/components/places/tests/unit/test_mozIAsyncLivemarks.js:192
(Diff revision 1)
>    do_check_eq(livemark.parentId, PlacesUtils.unfiledBookmarksFolderId);
>    do_check_eq(livemark.parentGuid, PlacesUtils.bookmarks.unfiledGuid);
>    do_check_true(livemark.feedURI.equals(FEED_URI));
>    do_check_eq(livemark.siteURI, null);
>    do_check_true(livemark.lastModified > 0);
> -  do_check_true(is_time_ordered(livemark.dateAdded, livemark.lastModified));
> +  do_check_eq(livemark.dateAdded, livemark.lastModified);

why this change? is_time_ordered is used to avoid the timing resolution problem on Windows where PRTime and Date.now() may differ up to 16ms.
Attachment #8922573 - Flags: review?(mak77)
Priority: -- → P3
Comment on attachment 8922573 [details]
Bug 1412143 - Add `mozIAsyncLivemarks.invalidateCachedLivemarks`.

https://reviewboard.mozilla.org/r/193676/#review199058

> Why is this in the invalidate call instead of just on shutdown?
> What happens if something called reloadLivemark, and then something else invokes invalidateCachedLivemarks? Looks like the timer will never fire.

Oops, you're right. I was worried that the reload timer might be working with an out-of-date livemarks map, but, since we pass the map to `_startReloadTimer`, that's already possible. I wonder if a better fix here is to keep the timer, but have the timer callback fetch the livemarks map. WDYT?

> why these changes? is it because of the isAnno optimization?

Yes. Removing the `isAnno` check has an interesting consequence, though, related to your question below.

> why this change? is_time_ordered is used to avoid the timing resolution problem on Windows where PRTime and Date.now() may differ up to 16ms.

Since we serialize access to the livemarks map now, we'll get an `onItemChanged` observer notification when we set the site and feed URI annos, from https://searchfox.org/mozilla-central/rev/40e8eb46609dcb8780764774ec550afff1eed3a5/toolkit/components/places/nsLivemarkService.js#220-223.

The `isAnno` change that you mentioned above ignores this, so we'll always use the `dateAdded` as the `lastModified` time, instead of adjusting it in https://searchfox.org/mozilla-central/rev/40e8eb46609dcb8780764774ec550afff1eed3a5/toolkit/components/places/nsLivemarkService.js#324.
Depends on: 1417101
is the dependency on bug 1417101 a strict one, or just a connection? Should I proceed reviewing this?
Flags: needinfo?(kit)
(In reply to Marco Bonardo [::mak] from comment #7)
> is the dependency on bug 1417101 a strict one, or just a connection? Should
> I proceed reviewing this?

Just a connection; all the tests still pass without bug 1417101. I do pass `aDontUpdateLastModified` when we write the livemark annos now, but the `onItemChanged` observer still works correctly, and just passes the livemark folder's current modified date.
Flags: needinfo?(kit)
No longer depends on: 1417101
See Also: → 1417101
Comment on attachment 8922573 [details]
Bug 1412143 - Add `mozIAsyncLivemarks.invalidateCachedLivemarks`.

https://reviewboard.mozilla.org/r/193676/#review205974
Attachment #8922573 - Flags: review?(mak77) → review+
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f92cfb21a2e
Add `mozIAsyncLivemarks.invalidateCachedLivemarks`. r=mak
https://hg.mozilla.org/mozilla-central/rev/4f92cfb21a2e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: