1377598 - browser_page_action_menu.js fails with async places transactions turned on

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Mark Banner (:standard8)]

This test fails when browser.places.useAsyncTransactions is set to true:

23:04:44 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/urlbar/browser_page_action_menu.js | "Edit This Bookmark" == "Bookmark This Page" - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_page_action_menu.js :: bookmark/< :: line 67
2597 23:04:44 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/urlbar/browser_page_action_menu.js | false == true - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_page_action_menu.js :: bookmark/< :: line 68

Comment 1

[Mark Banner (:standard8)]

An initial look at this reveals that on the onItemAdded listener isn't being triggered for the star UI the first time it is used. The listener seems to be added, so I'm not quite sure what's happening yet.

I'll dig into this more tomorrow.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

hm, I don't know if it's related to the problem, but IIRC the star uses addLazyBookmarkObserver. That means when onItemAdded is fired, it is not really observing, instead it's PlacesCategoriesObserver that catches the notification and registers a real observer. At this point I suspect it doesn't work as expected, because the NEW bookmarking API caches the observer when it invokes getObservers() so it won't see the newly registered one.

To sum up:
OLD API: onItemAdded is seen by PlacesCategoriesObserver that adds the star as an observer, since we are walking the array of observers, we'll see the new one and invoke it.
NEW API: onItemAdded is seen by PlacesCategoriesObserver that adds the star as an observer, since we already fetched the observers through getObservers, we won't see the new one.

Note that I'm removing the concept of lazy observers in bug 1371677. Partially a luck since it may fix this, partially a problem because it's a complex patch to review.

Comment 3

[Mark Banner (:standard8)]

Now that bug 1371677 has landed, it looks like this is down to a timing issue waiting for a bookmark to be removed.

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1377598 - When testing the page action menu, wait for the bookmark to be removed before continuing to avoid issues when enabling async transactions.

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

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8885658 [details]
Bug 1377598 - When testing the page action menu, wait for the bookmark to be removed before continuing to avoid issues when enabling async transactions.

https://reviewboard.mozilla.org/r/156508/#review161598

::: browser/base/content/test/urlbar/browser_page_action_menu.js:65
(Diff revision 1)
>  
> +    // Wait for the bookmark to be removed before continuing.
> +    await BrowserTestUtils.waitForCondition(async () => {
> +      return !(await PlacesUtils.bookmarks.fetch({url}));
> +    }, "The bookmark for the page should have been removed");
> +

Can we use PlacesTestUtils.waitForNotification with "onItemRemoved" instead? it should be more efficient.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8885658 [details]
Bug 1377598 - When testing the page action menu, wait for the bookmark to be removed before continuing to avoid issues when enabling async transactions.

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

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8885658 [details]
Bug 1377598 - When testing the page action menu, wait for the bookmark to be removed before continuing to avoid issues when enabling async transactions.

https://reviewboard.mozilla.org/r/156508/#review161612

::: browser/base/content/test/urlbar/browser_page_action_menu.js:64
(Diff revision 2)
>      StarUI._element("editBookmarkPanelRemoveButton").click();
>  
> +    // Wait for the bookmark to be removed before continuing.
> +    await PlacesTestUtils.waitForNotification("onItemRemoved",
> +      (id, parentId, index, type, itemUrl) => url == itemUrl.spec);
> +

if this uses old transactions (synchronous behavior) don't we risk that click() removes the bookmark before we observe?
Should be store the promise before, and await for it after?

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8885658 [details]
Bug 1377598 - When testing the page action menu, wait for the bookmark to be removed before continuing to avoid issues when enabling async transactions.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d7414321dbe
When testing the page action menu, wait for the bookmark to be removed before continuing to avoid issues when enabling async transactions. r=mak

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0d7414321dbe