1437476 - Add automated test for "A bookmark can be loaded inside the Bookmarks Sidebar"

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement, P2)

Description

[Paul Silaghi, QA [:pauly]]

TestRail link: https://testrail.stage.mozaws.net/index.php?/cases/view/4162

Steps:
1. Launch Firefox.
2. Enable the Bookmarks Sidebar.
3. Right-click a bookmark from the sidebar and select Properties.
4. Check the Load this bookmark in the sidebar option and click Save.
5. Open that bookmark.

Expected results:
1. Firefox is successfully launched.
2. The Bookmarks Sidebar is enabled and displayed at the left side.
3. A dialog box is brought up showing the properties of the selected bookmark.
4. The properties of the bookmark are successfully updated and no errors are thrown for this action.
5. The bookmark is successfully opened inside the Bookmarks Sidebar. 
The header of the sidebar displays the title of the page and a horizontal scrollbar is available for the sidebar itself.

Comment 1

[Paul Silaghi, QA [:pauly]]

Attachment: browser_bookmark_load_in_sidebar

Comment 2

[Paul Silaghi, QA [:pauly]]

Attachment: browser_bookmark_load_in_sidebar

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8951617 [details] [diff] [review]
browser_bookmark_load_in_sidebar

Review of attachment 8951617 [details] [diff] [review]:
-----------------------------------------------------------------

For some reason, this is leaking when I test it locally on a debug build with leak output:

TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_bookmark_load_in_sidebar.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/bookmarks/bookmarksPanel.xul]
TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_bookmark_load_in_sidebar.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/places/bookmarkProperties.xul]
TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_bookmark_load_in_sidebar.js | leaked 1 window(s) until shutdown [url = about:blank]

I'm not quite sure what is going on here. It might be worth looking at other places where we use withSidebarTree -> withBookmarksDialog and seeing if there's anything different.

Comment 4

[Paul Silaghi, QA [:pauly]]

That's strange, I'm not sure either what's going on.
I'm seeing some failures for some other recently added tests after running this test on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=71caf7356c5d04c5dd9999f5885179ad4d934ab7
"A promise chain failed to handle a rejection: sidebarBrowser is null - stack: null Rejection date: Mon Feb 19 2018 06:19:23 GMT-0800 (PST) - false == true"

Comment 5

[Paul Silaghi, QA [:pauly]]

The way I'm waiting for the sidebarBrowser to load after clicking on the sidebar bookmark is somehow wrong. Do you have any idea how to wait for the sidebarBrowser content in a different way?

Comment 6

[Mark Banner (:standard8)]

Ok, my suspicion is the test is somehow not waiting fully for the load, which is trying to kick in later during other tests.

I'd suggest trying something like:


--------
let sidebar = document.getElementById("sidebar");
let sidebarBrowser = sidebar.contentDocument.getElementById("web-panels-browser");

tree.selectItems([bm.guid]);
tree.controller.doCommand("placesCmd_open");

await BrowserTestUtils.browserLoaded(sidebarBrowser, false, TEST_URL);
--------

I think that would then work, but I haven't tested it.

Comment 7

[Paul Silaghi, QA [:pauly]]

(In reply to Mark Banner (:standard8) from comment #6)
> await BrowserTestUtils.browserLoaded(sidebarBrowser, false, TEST_URL);

browser/components/places/tests/browser/browser_bookmark_load_in_sidebar.js | Uncaught exception - at resource://testing-common/BrowserTestUtils.jsm:272 - TypeError: browser is null

I guess sidebarBrowser is still NULL at this point...

Comment 8

[Paul Silaghi, QA [:pauly]]

Attachment: browser_bookmark_load_in_sidebar

After some tweaks, it finally works.
Thanks for the suggestions.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8953366 [details] [diff] [review]
browser_bookmark_load_in_sidebar

Ok, I figured out the leaks. It turns out that toggling the checkbox causes an annotation to be written to the bookmark.

For the annotation, we're passing an array of objects - PlacesTransaction clones the array but not the objects. I suspect it then leaks as the new array of objects is passed to the c++ backend across the xpcom interface.

I have a patch to fix this, I'll get it up in a moment, and I'll re-attach this one at the same time.

Comment 10

[Mark Banner (:standard8)]

Attachment: Bug 1437476 - Clone annotation objects before passing to back-end services to avoid leaks.

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

Comment 11

[Mark Banner (:standard8)]

Attachment: Bug 1437476 - Add a test for checking that a bookmark can be loaded inside the Bookmarks sidebar.

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

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8954796 [details]
Bug 1437476 - Add a test for checking that a bookmark can be loaded inside the Bookmarks sidebar.

https://reviewboard.mozilla.org/r/223922/#review229918

Comment 13

[Paul Silaghi, QA [:pauly]]

Try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd9316f0f9fd126998bf88cd8c82a5569f5a283d

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8954795 [details]
Bug 1437476 - Clone annotation objects before passing to back-end services to avoid leaks.

https://reviewboard.mozilla.org/r/223920/#review230202

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06a197051c96
Clone annotation objects before passing to back-end services to avoid leaks. r=mak
https://hg.mozilla.org/integration/autoland/rev/b0990ecdb2d6
Add a test for checking that a bookmark can be loaded inside the Bookmarks sidebar. r=standard8

Comment 16

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/06a197051c96
https://hg.mozilla.org/mozilla-central/rev/b0990ecdb2d6