Closed Bug 1378711 Opened 3 years ago Closed 3 years ago

Bookmarks Properties / Places Transactions are susceptible to failing to undo correctly

Categories

(Toolkit :: Places, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Discovered whilst working on the test for bug 1376925:

It is possible for a batched transaction to be marked completed whilst a transact is happening, and this causes the undo tree to be incorrect - not everything gets undone.

The specific case uncovered in bug 1376925 is likely to affect only tests, but it might lead to other errors. Here's the case:

- The test opens the bookmarks properties dialog with "add a bookmark".
- This starts a batched transaction the first item adds a new bookmark.
- The test then simulates pressing the new folder button.
- The new folder is created, and there's then an additional move for the bookmark that happens.
- The test sees the new folder being created on the UI, and so proceeds to cancel the dialog
- Cancelling the dialog causes the batch to be completed and then undo to be called

https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/browser/components/places/content/bookmarkProperties.js#446

- However, the move transaction is in progress when the batch is flagged as completed.

- The move completes:

https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/toolkit/components/places/PlacesTransactions.jsm#546

and transact() then sees that we're no longer in batch mode, so records the transaction as a forced new entry, which removes the existing partial undo queue.

- The undo then fails to run correctly, and the test causes failures in other tests as it hasn't cleaned itself up.

Bug 1376925 has added a workaround to browser_bookmarksProperties.js for the intermittent failure this was causing (in browser_library_commands.js), by ensuring the notifications for the async transactions have happened, however we need something better here.
I think I have a simple fix for this that I'll post once bug 1376925 is reviewed.
Assignee: nobody → standard8
Comment on attachment 8885632 [details]
Bug 1378711 - Ensure that current places transactions have completed before clearing batch processing mode to avoid messing up the undo queue.

https://reviewboard.mozilla.org/r/156484/#review162098

I have a lot of doubts regarding this, it sounds like it may be wallpapering more bugs, and cause future headaches.

My assumption here is that the problem is _batchBlockingDeferred in browser-places.js, that is basically what controls when await task() here is resolved, afaict. and that deferred is resolved too early, when operations are not complete yet.

I actually think we should figure out a way to fix the consumers, rather than risking to group an unrelated operation into a batch.
The problematic consumers are browser-places and bookmarkProperties. We likely need a promise that tells they are done with their changes, rather than the _batchBlockingDeferred trick.

If in the meanwhile we want a workaround, we should probably apply it to _batchBlockingDeferred, that is already a bad hack.

::: toolkit/components/places/PlacesTransactions.jsm:566
(Diff revision 1)
>      return this._mainEnqueuer.enqueue(async () => {
>        this._batching = true;
>        this._createdBatchEntry = false;
>        let rv;
>        try {
>          // We should return here, but bug 958949 makes that impossible.

looks like this bug has been fixed in the meanwhile... plus this is async/await and no more generator.
Maybe while here you could fix this? Or just remove the comment if now we need to await.

::: toolkit/components/places/PlacesTransactions.jsm:573
(Diff revision 1)
>        } finally {
> +        // We must enqueue clearing batching mode to ensure that any existing
> +        // transactions have completed before we clear the batching mode.
> +        this._mainEnqueuer.enqueue(async () => {
> -        this._batching = false;
> +          this._batching = false;
> -        this._createdBatchEntry = false;
> +          this._createdBatchEntry = false;

my doubt is whether one may inadvertently nest a batch, and then the first one will set _batching = false for the second one.

I mean something like:
await transactions.batch();
await transactions.batch();

Additionally, the batch is what is inside "task", this risks to group into the batch stuff that doesn't pertain to it.
await transactions.batch();
await transactions.somethingElse();
Attachment #8885632 - Flags: review?(mak77)
On a second thought, the cases I was scared about are not going to happen, because you enqueue before returning from the batch() call. It's better than I thought.

The only negative aspect left is that this is only a partial solution, if the dialog is awaiting on something that is not a transaction (maybe even just a promiseItemId) before actually starting a transaction, this won't catch it.

As previously discussed, I fear the only real solutions to the problem are:
1. have in editBookmarkOverlay an array of all the pending promises, and in both bookmarkProperties and browser-places wait on that and on eventual local promises. It would basically be a Promise.all([_batchBlockingDeferred, pendingPromises]). It may require quite some refactoring. We'd collect the array of promises only when batching (we could pass an array to initPanel to push promises to) and clear it when the batch is done.
2. stop having these dialogs instant apply, and instead just collect transactions in an array and pass that to batch, that is the proper way to use it. Unfortunately this may be even more complex, and I wonder how we'd handle things like creating and visualizing a new folder in the folders tree.

Both may require far more work, so maybe after all this workaround would allow to start testing things sooner.
What do you think? I'm now prone to take this, and have a separate bug to do the promises thing.
Comment on attachment 8885632 [details]
Bug 1378711 - Ensure that current places transactions have completed before clearing batch processing mode to avoid messing up the undo queue.

https://reviewboard.mozilla.org/r/156484/#review162464

If this is the last issue preventing us from enabling the thing in Nightly, let's land it, and file a separate bug blocking bug 979280 to see if we can fix batches properly. I may have a look at it in the next weeks.
Attachment #8885632 - Flags: review+
Comment on attachment 8885632 [details]
Bug 1378711 - Ensure that current places transactions have completed before clearing batch processing mode to avoid messing up the undo queue.

https://reviewboard.mozilla.org/r/156484/#review162098

> looks like this bug has been fixed in the meanwhile... plus this is async/await and no more generator.
> Maybe while here you could fix this? Or just remove the comment if now we need to await.

I think we now want to await, so I'll drop the comment.
(In reply to Marco Bonardo [::mak] from comment #4)
> Both may require far more work, so maybe after all this workaround would
> allow to start testing things sooner.
> What do you think? I'm now prone to take this, and have a separate bug to do
> the promises thing.

I agree I think it is worth landing and getting testing started. I've filed bug 1380948 as the follow-up to this.
Blocks: 1380948
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/03f50585d8b7
Ensure that current places transactions have completed before clearing batch processing mode to avoid messing up the undo queue. r=mak
https://hg.mozilla.org/mozilla-central/rev/03f50585d8b7
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.