1865990 - Opening the customize mode breaks the bookmarks/history sidebar bits (tree view gets confused)

Status: RESOLVED DUPLICATE of bug 1839304

(Firefox :: Bookmarks & History, defect)

Description

[Sören Hentzschel]

STR:

1. Open the bookmarks or history sidebar
2. Open the customize mode
3. Close the customize mode or switch the tab
4. Try to expand a row in the sidebar

Actual:

Nothing happens until you close and reopen the sidebar.

Expected:

Sidebar still works.

Comment 1

[Gregory Pappas [:gregp]]

Hmm, I can't reproduce this issue. Are you able to provide a screen recording of the issue happening in a fresh profile? Thanks

Comment 2

[Gregory Pappas [:gregp]]

Nevermind, I am able to reproduce the issue.

Comment 3

[Gregory Pappas [:gregp]]

My current understanding:

This is happening because entering/exiting customize mode is the only case where the sidebar is destroyed and then recreated without load being called
https://searchfox.org/mozilla-central/rev/d7f837add602d270f2b2958b3ab5206dc85965c0/browser/components/places/content/places-tree.js#449-474

So, the places result gets garbage collected(?) upon sidebar destruction without being replaced by a new result when the sidebar is recreated, which is not an expected case, causing everything to break.

Commenting out this._result.removeObserver(this) fixes the bug locally, but is obviously not a solution...

:Gijs, do you have any thoughts?

Comment 4

[:Gijs (he/him)]

(In reply to Gregory Pappas [:gregp] from comment #3)

My current understanding:

This is happening because entering/exiting customize mode is the only case where the sidebar is destroyed and then recreated without load being called
https://searchfox.org/mozilla-central/rev/d7f837add602d270f2b2958b3ab5206dc85965c0/browser/components/places/content/places-tree.js#449-474

So, the places result gets garbage collected(?) upon sidebar destruction without being replaced by a new result when the sidebar is recreated, which is not an expected case, causing everything to break.

Yeah, this sounds odd.

Commenting out this._result.removeObserver(this) fixes the bug locally, but is obviously not a solution...

:Gijs, do you have any thoughts?

I do not, off-hand - maybe Marco does?

Comment 5

[Marco Bonardo [:mak]]

(In reply to Gregory Pappas [:gregp] from comment #3)

My current understanding:

This is happening because entering/exiting customize mode is the only case where the sidebar is destroyed and then recreated without load being called
https://searchfox.org/mozilla-central/rev/d7f837add602d270f2b2958b3ab5206dc85965c0/browser/components/places/content/places-tree.js#449-474

You mean we destroy the sidebar (call unload) but then historySidebar.xhtml onload handler is not invoked?
That indeed sounds like a bug due to Customization handling, as I don't think Places is doing anything special here: it's just using onload and onunload of the sidebar document.
I don't know where we close the sidebar when customization is opened, though this code looks very suspicious:
https://searchfox.org/mozilla-central/rev/606cdd34860e42611fceba025314864cd293a43b/browser/base/content/browser-sidebar.js#467-472

Sorry for bouncing this back, but something's wrong when we toggle() the sidebar back in customize mode and I can't find where/how we do that.

Comment 6

[Gregory Pappas [:gregp]]

You mean we destroy the sidebar (call unload) but then historySidebar.xhtml onload handler is not invoked?

I mean view.setTree(null) is called when entering customize mode, resulting in this._result.removeObserver(this) being called. Then, when we exit customize mode, view.setTree(<tree id="historyTree" class="sidebar-placesTree" is="places-tree" ...) is called, but MozPlacesTree.load isn't, so we're left in this weird state where we have a tree, but our nsINavHistoryResult is broken and trying to do anything with it results in NS_ERROR_UNEXPECTED.

Comment 7

[Gregory Pappas [:gregp]]

Also, you don't really need to enter customize mode to reproduce the problem. Running a snippet like this in the browser toolbox console results in the same issue

window.requestAnimationFrame(() => {
  document.querySelector("#sidebar").style.display = "none";
  window.requestAnimationFrame(
    () => (document.querySelector("#sidebar").style.display = "")
  );
});

Comment 8

[:Gijs (he/him)]

I think this is an artifact of the responsibility for this tree widget living in 3 places:

1. treeView.js
2. places-tree.js
3. surprise, the XUl <tree> code. Which can unassign the view if the frame / display bits go away. Cf. https://searchfox.org/mozilla-central/rev/606cdd34860e42611fceba025314864cd293a43b/layout/xul/tree/nsITreeView.idl#133 ; e.g. https://searchfox.org/mozilla-central/rev/606cdd34860e42611fceba025314864cd293a43b/layout/xul/tree/nsTreeBodyFrame.cpp#324 (haven't verified we're hitting that, don't have a compiled debuggable build to hand atm)

ISTM that the observer being added/removed should be done in 1 place. Given that at this point we're all loath to touch the C++ tree view code because it needs to be burned with fire, I think that place should be one of the two JS things. Because the C++ code calls treeView.js to set/delete the tree ref from the view, it should be there - or, if there needs to be dependency injection for this it should be explicit (define an overridable property or use an event of some kind to communicate with the other party from treeView.js to the places-tree.js of the world). Right now the dependency is implicit and this is going to spell trouble whenever anything would destroy the tree's view (be it customize mode or anything else that messes with the display bits and destroys the tree's frame).

(I mean, either that or leaving this be and then using one of the new reusable components virtual list components or the Thunderbird equivalent to reimplement this. That's A Big Job but would probably fix this because it removes the C++ bits from the equation and stops things depending on the frame tree, which is ultimately what's at issue here. Oh, if we could but make our XBL design decisions differently at this point...)

Going to move this over to bookmarks/history because I think the synced tabs and webextension sidebars are unaffected as they don't use the places treeview, so this seems like a places problem - certainly it's not really possible for Customize Mode code to "fix" this for the places sidebar.

Comment 9

[Marco Bonardo [:mak]]

Sounds good. Dao is investigating the possibility to use the Thunderbird html tree implementation.
Sounds like this is a dupe of bug 1839304.