Closed
Bug 1437476
Opened 6 years ago
Closed 6 years ago
Add automated test for "A bookmark can be loaded inside the Bookmarks Sidebar"
Categories
(Firefox :: Bookmarks & History, enhancement, P2)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: pauly, Assigned: pauly)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fxsearch])
Attachments
(2 files, 3 obsolete files)
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•6 years ago
|
||
Assignee: nobody → paul.silaghi
Attachment #8951574 -
Flags: review?(standard8)
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8951574 -
Attachment is obsolete: true
Attachment #8951574 -
Flags: review?(standard8)
Attachment #8951617 -
Flags: review?(standard8)
Updated•6 years ago
|
Priority: -- → P2
Whiteboard: [fxsearch]
Comment 3•6 years ago
|
||
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•6 years 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•6 years 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)
Comment 6•6 years ago
|
||
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•6 years 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•6 years 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 9•6 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•6 years 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+
Assignee | ||
Comment 13•6 years ago
|
||
Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd9316f0f9fd126998bf88cd8c82a5569f5a283d
Keywords: checkin-needed
Comment 14•6 years 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•6 years 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/06a197051c96 https://hg.mozilla.org/mozilla-central/rev/b0990ecdb2d6
Status: NEW → RESOLVED
Closed: 6 years 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.
Description
•