browser_page_action_menu.js fails with async places transactions turned on

RESOLVED FIXED in Firefox 56

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
Assignee: nobody → standard8
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
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.
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.
Depends on: 1371677
(Assignee)

Updated

2 years ago
Assignee: standard8 → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 3

2 years ago
Now that bug 1371677 has landed, it looks like this is down to a timing issue waiting for a bookmark to be removed.
Assignee: nobody → standard8
Status: NEW → ASSIGNED

Comment 5

2 years ago
mozreview-review
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.
Attachment #8885658 - Flags: review?(mak77)
Comment hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
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?
Attachment #8885658 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)

Comment 9

2 years ago
Pushed by mbanner@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0d7414321dbe
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.