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

RESOLVED FIXED in Firefox 60

Status

()

P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: pauly, Assigned: pauly)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Assignee: nobody → paul.silaghi
Attachment #8951574 - Flags: review?(standard8)
(Assignee)

Comment 2

a year ago
Attachment #8951574 - Attachment is obsolete: true
Attachment #8951574 - Flags: review?(standard8)
Attachment #8951617 - Flags: review?(standard8)
Priority: -- → P2
Whiteboard: [fxsearch]
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.
Attachment #8951617 - Flags: review?(standard8)
(Assignee)

Comment 4

a year ago
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"
(Assignee)

Comment 5

a year ago
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?
Flags: needinfo?(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.
Flags: needinfo?(standard8)
(Assignee)

Comment 7

a year ago
(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...
Flags: needinfo?(standard8)
(Assignee)

Comment 8

a year ago
After some tweaks, it finally works.
Thanks for the suggestions.
Attachment #8951617 - Attachment is obsolete: true
Flags: needinfo?(standard8)
Attachment #8953366 - Flags: review?(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.
Attachment #8953366 - Attachment is obsolete: true
Attachment #8953366 - Flags: review?(standard8)

Comment 12

a year ago
mozreview-review
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
Attachment #8954796 - Flags: review?(standard8) → review+

Comment 14

a year ago
mozreview-review
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
Attachment #8954795 - Flags: review?(mak77) → review+

Comment 15

a year ago
Pushed by mbanner@mozilla.com:
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
Keywords: checkin-needed

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/06a197051c96
https://hg.mozilla.org/mozilla-central/rev/b0990ecdb2d6
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.