1556427 - bookmarks.onRemoved event arguments have incorrect values

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[nero120]

Attachment: Screenshot of Bookmarks Library prior to deleting all

This is especially noticable when deleting multiple bookmarks simultaneously. You can recreate this by:

• Open the Bookmarks Library, delete all current bookmarks
• Create a new bookmark in Other Bookmarks named "Bookmark 1" with URL "https://www.mozilla.org/en-GB/about/"
• Copy the bookmark and rename it to "Bookmark 2"
• Repeat this process until you have ten bookmarks, so named
• Select all ten bookmarks (ctrl-a) and press the delete key to remove them all

If you log the triggered onRemoved events arguments they will appear as follows:

{"0":"-B2M2PhUxDDK","1":{"parentId":"unfiled_____","index":0,"node":{"id":"-B2M2PhUxDDK","parentId":"unfiled_____","index":0,"type":"bookmark","url":"https://www.mozilla.org/en-GB/about/"}}}
{"0":"4i-26x3DI4Uv","1":{"parentId":"unfiled_____","index":1,"node":{"id":"4i-26x3DI4Uv","parentId":"unfiled_____","index":1,"type":"bookmark","url":"https://www.mozilla.org/en-GB/about/"}}}
{"0":"cluw6-H0-O5T","1":{"parentId":"unfiled_____","index":2,"node":{"id":"cluw6-H0-O5T","parentId":"unfiled_____","index":2,"type":"bookmark","url":"https://www.mozilla.org/en-GB/about/"}}}
{"0":"2oyWvM9m-aYq","1":{"parentId":"unfiled_____","index":3,"node":{"id":"2oyWvM9m-aYq","parentId":"unfiled_____","index":3,"type":"bookmark","url":"https://www.mozilla.org/en-GB/about/"}}}
{"0":"xympA5dpNM_7","1":{"parentId":"unfiled_____","index":4,"node":{"id":"xympA5dpNM_7","parentId":"unfiled_____","index":4,"type":"bookmark","url":"https://www.mozilla.org/en-GB/about/"}}}
{"0":"shjNngfAcY6r","1":{"parentId":"unfiled_____","index":5,"node":{"id":"shjNngfAcY6r","parentId":"unfiled_____","index":5,"type":"bookmark","url":"https://www.mozilla.org/en-GB/about/"}}}
{"0":"WakuxlnqVSyJ","1":{"parentId":"unfiled_____","index":6,"node":{"id":"WakuxlnqVSyJ","parentId":"unfiled_____","index":6,"type":"bookmark","url":"https://www.mozilla.org/en-GB/about/"}}}
{"0":"BLPLxb-nPO7i","1":{"parentId":"unfiled_____","index":7,"node":{"id":"BLPLxb-nPO7i","parentId":"unfiled_____","index":7,"type":"bookmark","url":"https://www.mozilla.org/en-GB/about/"}}}
{"0":"DviWRnAvp0BU","1":{"parentId":"unfiled_____","index":8,"node":{"id":"DviWRnAvp0BU","parentId":"unfiled_____","index":8,"type":"bookmark","url":"https://www.mozilla.org/en-GB/about/"}}}
{"0":"zdq80AyvYJHQ","1":{"parentId":"unfiled_____","index":9,"node":{"id":"zdq80AyvYJHQ","parentId":"unfiled_____","index":9,"type":"bookmark","url":"https://www.mozilla.org/en-GB/about/"}}}

Note that:

• The removeInfo.node object is missing title
• The index values do not take into account previous events. i.e. the second event has an index of 1, however this is actually 0 due to the first event having already changed the indexes of the proceeding deletions.

Compare this to the events as logged in Chrome:

{"0":"77355","1":{"index":0,"node":{"dateAdded":1559568479065,"id":"77355","title":"Bookmark 1","url":"https://www.mozilla.org/en-GB/about/"},"parentId":"2"}}
{"0":"77356","1":{"index":0,"node":{"dateAdded":1559568479068,"id":"77356","title":"Bookmark 2","url":"https://www.mozilla.org/en-GB/about/"},"parentId":"2"}}
{"0":"77357","1":{"index":0,"node":{"dateAdded":1559568479070,"id":"77357","title":"Bookmark 3","url":"https://www.mozilla.org/en-GB/about/"},"parentId":"2"}}
{"0":"77358","1":{"index":0,"node":{"dateAdded":1559568479072,"id":"77358","title":"Bookmark 4","url":"https://www.mozilla.org/en-GB/about/"},"parentId":"2"}}
{"0":"77359","1":{"index":0,"node":{"dateAdded":1559568479074,"id":"77359","title":"Bookmark 5","url":"https://www.mozilla.org/en-GB/about/"},"parentId":"2"}}
{"0":"77360","1":{"index":0,"node":{"dateAdded":1559568479076,"id":"77360","title":"Bookmark 6","url":"https://www.mozilla.org/en-GB/about/"},"parentId":"2"}}
{"0":"77361","1":{"index":0,"node":{"dateAdded":1559568479079,"id":"77361","title":"Bookmark 7","url":"https://www.mozilla.org/en-GB/about/"},"parentId":"2"}}
{"0":"77362","1":{"index":0,"node":{"dateAdded":1559568479081,"id":"77362","title":"Bookmark 8","url":"https://www.mozilla.org/en-GB/about/"},"parentId":"2"}}
{"0":"77363","1":{"index":0,"node":{"dateAdded":1559568479085,"id":"77363","title":"Bookmark 9","url":"https://www.mozilla.org/en-GB/about/"},"parentId":"2"}}
{"0":"77364","1":{"index":0,"node":{"dateAdded":1559568479087,"id":"77364","title":"Bookmark 10","url":"https://www.mozilla.org/en-GB/about/"},"parentId":"2"}}

The title values for the BookmarkTreeNode objects are present and the indexes properly reflect the current index of the bookmarks when actually deleted.

Comment 1

[Alex Cornestean]

Hi,
Can you please provide the method used to log the onRemoved events arguments as mentioned in the STR of the bug? Thanks !

Comment 2

[nero120]

Hi Alex,

I was running the tests from my app's code which I've not yet converted to ES6 yet, you can get the same log format as above using:

function onRemovedHandler() {
    console.log(JSON.stringify(arguments));
};
browser.bookmarks.onRemoved.addListener(onRemovedHandler);

Comment 3

[Alex Cornestean]

Attachment: Nightly console log.png

Comment 4

[Alex Cornestean]

Hello,

The issue can be reproduced on the latest Release version (67.0.2 / 20190607204818) and Beta (68.0b9 / 20190610153228) by following the above STR under Windows 10 Pro 64-bit and macOS High Sierra 10.13.6.

On the Release and Beta versions of Firefox, the results are identical to the ones mentioned above, confirming the issue.

However, on the Nightly version (69.0a1 / 20190611213517), all events have an index value of 0 (for further details, please see the attached screenshot). I do not know exactly if this is the correct behavior and I would like you to confirm or deny the correctness of this, before modifying flags for Fx69. Thanks !

Nontheless, this still confirms that the issue can be reproduced on FX67 and FX68.

Comment 5

[nero120]

Hi Alex, I downloaded the latest nightly and confirmed that the events are now triggered in the correct order and the bookmark indexes reflect the new positions according to the event order (i.e. always 0), so that's fantastic!

The only minor difference remaining between Firefox and Chrome is that in Firefox the removeInfo.node object is still missing title, it would be good for all extension developers to have consistency between platforms.

Comment 6

[nero120]

Hi Alex, after further testing I've discovered the issue does affect Nightly 69.0a1 / 20190703094734. If you attempt to move multiple bookmarks at once, the oldIndex value in the onMoved events arguments is incorrect for all events bar the first. For example if you have the ten bookmarks as specified in the original post, then select bookmarks 1-5 and drag them all beneath Bookmark 10, the registered onMoved events appear as:

{"0":"22cLcbsNjgFF","1":{"parentId":"unfiled_____","index":9,"oldParentId":"unfiled_____","oldIndex":0}}
{"0":"O9ZiFs37M8Wv","1":{"parentId":"unfiled_____","index":9,"oldParentId":"unfiled_____","oldIndex":1}}
{"0":"-PdU_73wbHiM","1":{"parentId":"unfiled_____","index":9,"oldParentId":"unfiled_____","oldIndex":2}}
{"0":"NwbGffiElGTB","1":{"parentId":"unfiled_____","index":9,"oldParentId":"unfiled_____","oldIndex":3}}
{"0":"gMdM_TNaIOls","1":{"parentId":"unfiled_____","index":9,"oldParentId":"unfiled_____","oldIndex":4}}

This is incorrect as all oldIndex values should be 0 to be consistent with a new index value of 9 for each (the event values assumes the preceeding bookmark has finished it's move and is therefore no longer above it when the event begins). This behaviour is confirmed by checking Chrome:

{"0":"104475","1":{"index":9,"oldIndex":0,"oldParentId":"2","parentId":"2"}}
{"0":"104476","1":{"index":9,"oldIndex":0,"oldParentId":"2","parentId":"2"}}
{"0":"104477","1":{"index":9,"oldIndex":0,"oldParentId":"2","parentId":"2"}}
{"0":"104478","1":{"index":9,"oldIndex":0,"oldParentId":"2","parentId":"2"}}
{"0":"104479","1":{"index":9,"oldIndex":0,"oldParentId":"2","parentId":"2"}}

You can replicate these logs using the following code:

function onMovedHandler() {
    console.log(JSON.stringify(arguments));
};
browser.bookmarks.onMoved.addListener(onMovedHandler);

Comment 7

[Alex Cornestean]

Hello,

With the latest Nightly (69.0a1 / 20190704214457), under Windows 10 Pro 64-bit, for some reason, I am unable to reproduce the issue for which you have provided the new STR.

As a matter of fact, even when attempting to reproduce the issue using the old provided STR, I am no longer able to obtain the old (correct) behavior on Nightly. Nothing will be displayed in the browser console or otherwise when either deleting or moving bookmarks in the Bookmark Library.

I tried on two different machines, with new profiles, with no success.

The previously documented results are still properly obtainable in the latest Beta (68.0 / 20190701181138) and Release (67.0.4 20190619235627).

Comment 8

[nero120]

Hi Alex, I updated my Nightly to build 20190704214457 and re-ran the test, which worked as described above. I'm also running under under Windows 10 Pro 64-bit. Can you confirm the onMoved events are triggering? I'll attach a screen shot of my test below.

Comment 9

[nero120]

Attachment: Test when moving multiple bookmarks at once

Comment 10

[Alex Cornestean]

Hello,

Based on the screenshot you attached, I have managed to reproduce the issue concerning the ‘onMoved’ event on the latest Nightly (69.0a1 / 20190707211726) under Windows 10 Pro 64-bit and macOS High Sierra 10.13.6.

Will update the tracking flags appropriately. Thanks !

Comment 11

[nero120]

Great stuff! Thanks very much Alex. :)

Comment 12

[nero120]

Sooo, because I finally got fed up of people giving my extension 1 star reviews because "it's broken", I thought I'd try and work around this bug since it's been a year and no one has touched it. When I'm finally getting somewhere, I find another "bonus Mozilla feature" (TM) - what's wrong with this picture:

So bookmark indexes are also wrong which means I have to work around that as well, great. Is this part of the same bug or is it a different bug? I left wondering if this code has been tested at all...?!

Comment 13

[nero120]

Attachment: Bookmark Index Bug.png

Comment 14

[Rob Wu [:robwu]]

The index appears to reflect the index at the time of deletion, which isn't necessarily wrong when the whole folder is deleted.

The title property should be present, however. Someone filed a bug report at https://github.com/mdn/content/issues/13052, which shows that the code in the documentation does not behave as expected.

And for the record, the behavior works in Chrome and is confirmed to be working here:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/data/extensions/api_test/bookmarks/test.js;l=414;drc=6e6a9ab9cc16cedcfe189fe466b969dc48724d06

P.S. If comment 12 / comment 13 is still reproducible, please file a new bug with reproduction steps (test case).

Comment 15

[Rob Wu [:robwu]]

Attachment: Bug 1556427 - Add node.title to bookmarks.onRemoved event

The bookmarks.onRemoved event should have a title attribute
reflecting the title of the removed bookmark. This behavior is
documented in an example at:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/bookmarks/onRemoved

That documented example was reportedly broken: https://github.com/mdn/content/issues/13052

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6619dbd6ed33
Add node.title to bookmarks.onRemoved event r=Standard8,willdurand,markh

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/6619dbd6ed33