Closed Bug 824260 Opened 7 years ago Closed 2 years ago

Hang when closing a window with thousands of xul buttons

Categories

(Core :: XBL, defect, P1)

defect

Tracking

()

RESOLVED INACTIVE

People

(Reporter: alice0775, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: regression, reproducible)

Attachments

(5 files)

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/c965fa0804cf
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121221 Firefox/20.0 ID:20121221030826

browser.library.useNewDownloadsView = true ---- browser hangs up
browser.library.useNewDownloadsView = false --- download list appears in right side pane within 1 second.

Steps to reproduce:
1. Create clean new profile
2. Copy places.sqlite to the profile folder
3. Set browser.library.useNewDownloadsView = true and restart browser
4. Open Library
5, Select dowbloads folder in left side pane

Actual results:
Browser hang up

Expected results:
Show list in right side pane within 1 second.
sample plases.sqlite (7zip file) 
http://www.upload.ee/files/2924475/places.7z.html
Blocks: 822343
24 Dec build will contain some improvements for this, let me know if you notice any improvement. More to come in the next days. I think this can just be duped to Bug 822343 unless you have some specific issue to point out.

Question though, wich antivirus do you have? does disabling temporary real-time protection in the AV helps (even just by some seconds)?
>24 Dec build will contain some improvements for this, let me know if you notice any improvement.

http://hg.mozilla.org/mozilla-central/rev/dc2abccc2adb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121225 Firefox/20.0 ID:20121225030757

[1]browser.library.useNewDownloadsView = false
When select downloads folder browser does not freeze. The download folder opened immediately(almost 0sec)
After exit Firefox, the process remains is removed immediately(within 1sec)

[2]browser.library.useNewDownloadsView = true
When select downloads folder browser freezes for about 10sec. And also unable to switch to another application by taskbar .
After exit Firefox, the process remains about 15sec with 100% CPU usage

Umm, it is still very slow.
UI become unresponsive :(



>Question though, wich antivirus do you have?

avast!7

>does disabling temporary real-time protection in the AV helps (even just by some seconds)?

It seems nothing changed.
When select downloads folder browser freezes for about 10sec. And also unable to switch another application by taskbar .
After exit Firefox, the process remains about 15 sec with 100% CPU usage
NOTE:The downloaded file is already deleted and does not exist in the file system.
(In reply to Alice0775 White from comment #4)
> Umm, it is still very slow.
> UI become unresponsive :(

ok, this part is just bug 822343

> After exit Firefox, the process remains about 15 sec with 100% CPU usage

This looks strange, we should check why there's anything happening on shutdown, nothing should be there. I suppose the Library was still open when you closed Firefox? I wonder if for any reason in this case we refresh the view...
Since the first part is already tracked I'm transforming this bug to cover the shutdown problem.
Summary: Browser hang up when select downloads folder in Library if browser.library.useNewDownloadsView=true → Shutdown hang after opening the Library on the new Downloads view
Assignee: nobody → mano
Status: NEW → ASSIGNED
Duplicate of this bug: 824488
Alice, provided this bug needs fixing (likely we are doing something dumb like re-initializing the view on Library shutdown_, today's nightly for me is quite faster (the hang lasts less than 1s), could you provide any feedback in this regard?
Priority: -- → P1
(In reply to Marco Bonardo [:mak] (intermittently avail. until 3 Jan) from comment #7)
> Alice, provided this bug needs fixing (likely we are doing something dumb
> like re-initializing the view on Library shutdown_, today's nightly for me
> is quite faster (the hang lasts less than 1s), could you provide any
> feedback in this regard?

12-27 Nightly is as same result of 12-25( comment #4) .
There seems to be no improvement.

http://hg.mozilla.org/mozilla-central/rev/d29b182e169e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121227 Firefox/20.0 ID:20121227030822
Does that happen in safe mode? my view was much slower with the profiler add-on for example.
Fwiw, I have MSE antivirus and don't notice anything bad with it.
Btw, I can definitely reproduce an abnormal long hang with the attached places.sqlite. investigating that apart from the shutdown bug for now, likely 2 issues cohexist.
(In reply to Marco Bonardo [:mak] (intermittently avail. until 3 Jan) from comment #9)
> Does that happen in safe mode? my view was much slower with the profiler
> add-on for example.
> Fwiw, I have MSE antivirus and don't notice anything bad with it.

Same results in safe-mode.
Ah, this profile contains 5405 downloads... this is definitely not something we can handle :(
(In reply to Marco Bonardo [:mak] (intermittently avail. until 3 Jan) from comment #12)
> Ah, this profile contains 5405 downloads... this is definitely not something
> we can handle :(

however, no noticeable lag exists if browser.library.useNewDownloadsView = false.
(In reply to Alice0775 White from comment #13)
> however, no noticeable lag exists if browser.library.useNewDownloadsView =
> false.

yes that's cause the treeview is much better than the richlistbox, perf-wise. The richlistbox has no lazy behavior, so it lods everything at once.

The average of the profile is about 500 downloads per month.
Do you actually need all of this downloads history or did it just pile up due to being mixed up with common history?
(In reply to Marco Bonardo [:mak] (intermittently avail. until 3 Jan) from comment #14)
> (In reply to Alice0775 White from comment #13)
> Do you actually need all of this downloads history or did it just pile up
> due to being mixed up with common history?

I do not need download history actually.
But I need history, because I want to know which zip file already downloaded or not by the link color.
(In reply to Alice0775 White from comment #15)
> I do not need download history actually.
> But I need history, because I want to know which zip file already downloaded
> or not by the link color.

The problem is that download history and history are the same thing in this new implementation, so if we limit one we limit both. That means if you need those 4500 downloaded links to be marked visited we have a big problem to solve, since it's unlikely the richlistbox can handle them, and if we hide them we have a privacy hit.
mem.log when I close Library after choose Downloads tree.
It seems doing GC/CC(and it seems to take too long time)  when close  Library window.
(In reply to Marco Bonardo [:mak] from comment #12)
> Ah, this profile contains 5405 downloads... this is definitely not something
> we can handle :(

We'll need to prove that this is extremely uncommon or resolve for FF20 if we're planning on shipping the new download panel. Can we maintain the history but only view X downloads in the panel itself (if it's the view that's the issue)?
(In reply to Alex Keybl [:akeybl] from comment #19)
> We'll need to prove that this is extremely uncommon or resolve for FF20 if
> we're planning on shipping the new download panel.

yes, bug 822343 blocks our feature release bug

> Can we maintain the
> history but only view X downloads in the panel itself (if it's the view
> that's the issue)?

The issue is not the panel, it's the Library view that acts as the download manager.
I'm indeed working on a similar solution in bug 825846
Blocks: ReleaseDownloadsPane
No longer blocks: 822343
Depends on: 822343
Alex, for any tracking need you can look at bug 747422, any bug blocking the feature release is tracked there.
Oh my.

Unfortunately the issue we see here has nothing to do with downloads, places, i/o and the like. It rather seems that we're facing a strange layout/xbl performance issue.

In a chrome scratchpad, run the following code. It creates a richlistbox with 5000 richlistitems on which the "download" binding is applied. It'll take a while for the window to show up (and that's another performance issue, but somewhat more expected than what you're about to see):

let windowUrl = "data:application/vnd.mozilla.xul+xml," +
"<?xml version='1.0'?><?xml-stylesheet href='chrome://global/skin/'?>" +
"<?xml-stylesheet href='chrome://browser/content/downloads/allDownloadsViewOverlay.css'?>" +
"<?xml-stylesheet href='chrome://browser/skin/downloads/allDownloadsViewOverlay.css'?>" +
    "<window xmlns='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul'" +
    " title='test for bug 687573 - vertical scroll' width='300' height='500'>" +
    "<richlistbox flex='1' id='theList'/></window>";
let windowFeatures = "chrome,titlebar,height=500,width=500,centerscenn";

let win = Services.ww.openWindow(null, windowUrl, "_blank", windowFeatures, null);
win.addEventListener("load", function() {
  let theList = win.document.getElementById("theList");
  for (let i = 0; i < 5000; i++) {
    let elt = win.document.createElement("richlistitem");
    elt.setAttribute("class", "download download-state");
    elt.setAttribute("type", "download");
    theList.appendChild(elt);
  }
}, true);

Now close the window. Notice how the browser freezes for quite a bit.

Notes:
1) The download binding is a "styling" binding. It has no "implementation" part (though it inherits richlistitem).
2) This issue doesn't seem to reproduce if I add regular richlistitems rather than download-binding richlistitems.

I should also note that the current style sheet used by this binding seems to be very careless about performance, and that could justify, in part, the delay we see when the window opens. However, no matter how complicated an inefficient the stylesheet is, I wouldn't expect it to matter when the window is closed.

CCing Boris and Neil for input. Thanks in advance.
I just verified that removing all the rules from download.css and allDownloadsViewOverlay.css (content & theme) does not matter. It lags just as much.
Interesting... removing the three buttons from the binding reduces the lag-on-close by an order of magnitude (It doesn't seem to affect the lag-on-load though). What's strange is that the buttons are hidden (display:none, no frame) regardless when the window is closed. This looks more and more like an XBL issue.
Summary: Shutdown hang after opening the Library on the new Downloads view → Hang when closing the Places Library if the downloads view is loaded with thousands of items
*Much* reduced testcase. 15000 hidden (display: none) xul buttons are enough to cause this hang.

Try this in a chrome scratchpad:

Cu.import("resource://gre/modules/Services.jsm");

let windowUrl = "data:application/vnd.mozilla.xul+xml," +
"<?xml version='1.0'?><?xml-stylesheet href='chrome://global/skin/'?>" +
    "<window xmlns='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul'" +
    "  width='300' height='500'>" +
    "</window>";
let windowFeatures = "chrome,titlebar,height=500,width=500,centerscenn";

let win = Services.ww.openWindow(null,windowUrl, "_blank", windowFeatures, null);
win.addEventListener("load", function() {
  let theList = win.document.getElementById("theList");
  for (let i = 0; i < 15000; i++) {
    let button = win.document.createElement("button");
    button.setAttribute("style", "display: none;");
    win.document.documentElement.appendChild(button);
  }
}, true);
This workaround probably won't be acceptable due to accessibility considerations, but I'm putting it here anyway just to demonstrate the improvement of replacing the xul buttons with xul images.
Depends on: 827245
No longer depends on: 822343
Asking Boris and Neil for any hints on the xbl/layout/gc issue we see here. Please see comment 25 and comment 26.
Flags: needinfo?(bzbarsky)
So I assume the steps to reproduce from comment 25 are to run that code then close the resulting window?

If so, the time seems to be largely spent in two places:

1)  Tearing down the binding for an element (via SetBinding(null) ends up calling nsBindingManager::RemoveInsertionParent() on that node.  This does an enumeration over the insertion parent hashtable with RemoveInsertionParentCB as the callback, which compares the given node to the GetValue() of every single entry in the table and removes the ones that match.  So basically, this is unhooking all the cases in which the element is marked as the insertion parent of something.

2)  Then we do it _again_ from the RemovedFromDocumentInternal call that the RemoveFromBindingManagerRunnable does.

The size of the table is O(N) in number of elements that are inside insertion points, so the whole thing ends up O(N^2) in number of insertion points. 

I don't know to what extent Neil's stuff in bug 653881 fixes this...
Flags: needinfo?(bzbarsky)
Boris, what I find strange here is that this is only reproducible with very certain bindings (buttons in particular), and it doesn't seem to be correlated with anything intuitive.

Another issue I'm worried about is that all of this happens even when the buttons are hidden (as they are in the given testcase). I'd expect the xbl binding not to be attached in this case.
XBL bindings are attached to the roots of display:none subtrees (but not necessarily to nodes further down in them).

I would expect this to reproduce for any binding with populated insertion points; you just needs lots of the element in question...
I see. I guess I'm out of luck then. Thanks for the input.

It's plan B time, I guess, which is to replace most of the xul elements in this UI with html counterparts.
Well, or we could fix XBL to suck less here, maybe.  What's the timeframe on this?  Are we talking like "Firefox 19" or like "Firefox 21"?
(In reply to Boris Zbarsky (:bz) from comment #28)

> I don't know to what extent Neil's stuff in bug 653881 fixes this...

Closing the window takes about 20 seconds currently, but an older build from August containing the patches from bug 653881 takes only 2 seconds, so that's a huge improvement here. Window open time is about 12-15 seconds for both.
Flags: needinfo?(enndeakin)
Quite huge :)

It's trunk material though, isn't it? Unfortunately the download view is already in Aurora.
(In reply to Boris Zbarsky (:bz) from comment #32)
> Well, or we could fix XBL to suck less here, maybe.  What's the timeframe on
> this?  Are we talking like "Firefox 19" or like "Firefox 21"?

Firefox 20.
OK.  For 20, it seems like we could try to do something...

Neil, what do you think of adding another hashtable that maps the insertion parent to its set of insertion kids?  Then instead of enumerating the insertion point hashtable we'd just do a single lookup and remove the relevant entries.
With the change coming in bug 827405, this issue will become less severe for Firefox 20: The download binding (which contains the buttons and so on) isn't applied until the richlistitem for becomes visible.

So, unless the user had scrolled the list enough to show thousands of downloads, there won't be any hang when the window is closed. Given that, and given that the hang lasts for very few seconds, this should not block the release of the downloads panel feature. However, it'd be nice to have this fixed for Firefox 20 regardless, assuming the suggested XBL change is not risky.
Assignee: mano → nobody
No longer blocks: ReleaseDownloadsPane
Status: ASSIGNED → NEW
Component: Bookmarks & History → XBL
No longer depends on: 827245
OS: Windows 7 → All
Product: Firefox → Core
Hardware: x86 → All
Summary: Hang when closing the Places Library if the downloads view is loaded with thousands of items → Hang when closing a window with thousands of xul buttons
I think Neil said yesterday he was evaluating low risk changes for Aurora, so assigning to him based on that, but feel free to correct if I'm wrong.
Assignee: nobody → enndeakin
bz, is something like this what you had in mind?
 
This is notably faster on window close (3 seconds instead of 20).
Attachment #699821 - Flags: feedback?(bzbarsky)
Comment on attachment 699821 [details] [diff] [review]
Something like this?

Generally, speaking, yeah.  RemoveInsertionParentCB is unused after this, and SetInsertionParent probably needs to remove the child from the relevant list if !aParent, right?

But other than that, looks good.
Attachment #699821 - Flags: feedback?(bzbarsky) → feedback+
Blocks: 825102
Attached patch PatchSplinter Review
So this is the patch I have right now. However, it has one issue that there is a leak when the following is used:

var button = document.createElement("button");
button.cloneNode(true);

I think this is because SetInsertionParent(node, nullptr) is never called on the cloned node as it isn't in the document, so the nsTArray holding the children in the hash is never deleted.
Hmm.  Do we not leak the other things that are keyed off bound nodes in that situation?  :(
(In reply to Boris Zbarsky (:bz) from comment #42)
> Hmm.  Do we not leak the other things that are keyed off bound nodes in that
> situation?  :(

Yes, lots of things are leaked (nodes, windows, etc). I didn't check but I assume its the nsIContent keys in the hash that are being held onto.
No, what I meant was I was wondering why we don't leak via all the other hashtables the binding manager already maintains....
We're a week away from putting Firefox 20 onto the Beta channel, any new information or leads on the work here?
Marco/Mano: is this still affecting the downloads panel on beta?

I don't think we're going to be able to take Neil's patch on beta at this point. Do we need to look into alternative workarounds?
This doesn't affect the downloads view anymore, so it does not block.
Assignee: enndeakin → nobody
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.