824260 - Hang when closing a window with thousands of xul buttons

Status: RESOLVED INACTIVE

(Core :: XBL, defect, P1)

Description

[Alice0775 White]

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.

Comment 1

[Alice0775 White]

sample plases.sqlite (7zip file) 
http://www.upload.ee/files/2924475/places.7z.html

Comment 2

[Alice0775 White]

Attachment: sample plases.sqlite (7zip file)

Comment 3

[Marco Bonardo [:mak]]

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)?

Comment 4

[Alice0775 White]

>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.

Comment 5

[Marco Bonardo [:mak]]

(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.

Comment 7

[Marco Bonardo [:mak]]

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?

Comment 8

[Alice0775 White]

(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

Comment 9

[Marco Bonardo [:mak]]

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.

Comment 10

[Marco Bonardo [:mak]]

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.

Comment 11

[Alice0775 White]

(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.

Comment 12

[Marco Bonardo [:mak]]

Ah, this profile contains 5405 downloads... this is definitely not something we can handle :(

Comment 13

[Alice0775 White]

(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.

Comment 14

[Marco Bonardo [:mak]]

(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?

Comment 15

[Alice0775 White]

(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.

Comment 16

[Marco Bonardo [:mak]]

(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.

Comment 17

[Alice0775 White]

Attachment: mem.log, (enabled useNewDownloadsView)

mem.log when I close Library after choose Downloads tree.

Comment 18

[Alice0775 White]

It seems doing GC/CC(and it seems to take too long time)  when close  Library window.

Comment 19

[Alex Keybl [:akeybl]]

(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)?

Comment 20

[Marco Bonardo [:mak]]

(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

Comment 21

[Marco Bonardo [:mak]]

Alex, for any tracking need you can look at bug 747422, any bug blocking the feature release is tracked there.

Comment 22

[Asaf Romano (gone)]

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.

Comment 23

[Asaf Romano (gone)]

I just verified that removing all the rules from download.css and allDownloadsViewOverlay.css (content & theme) does not matter. It lags just as much.

Comment 24

[Asaf Romano (gone)]

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.

Comment 25

[Asaf Romano (gone)]

*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);

Comment 26

[Asaf Romano (gone)]

Attachment: replacing the xul button with xul image cuts most of the on-close hang.

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.

Comment 27

[Asaf Romano (gone)]

Asking Boris and Neil for any hints on the xbl/layout/gc issue we see here. Please see comment 25 and comment 26.

Comment 28

[Boris Zbarsky [: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...

Comment 29

[Asaf Romano (gone)]

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.

Comment 30

[Boris Zbarsky [:bzbarsky]]

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...

Comment 31

[Asaf Romano (gone)]

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.

Comment 32

[Boris Zbarsky [:bzbarsky]]

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"?

Comment 33

[Neil Deakin]

(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.

Comment 34

[Asaf Romano (gone)]

Quite huge :)

It's trunk material though, isn't it? Unfortunately the download view is already in Aurora.

Comment 35

[Marco Bonardo [:mak]]

(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.

Comment 36

[Boris Zbarsky [:bzbarsky]]

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.

Comment 37

[Asaf Romano (gone)]

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.

Comment 38

[Marco Bonardo [:mak]]

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.

Comment 39

[Neil Deakin]

Attachment: Something like this?

bz, is something like this what you had in mind?
 
This is notably faster on window close (3 seconds instead of 20).

Comment 40

[Boris Zbarsky [: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.

Comment 41

[Neil Deakin]

Attachment: Patch

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.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Hmm.  Do we not leak the other things that are keyed off bound nodes in that situation?  :(

Comment 43

[Neil Deakin]

(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.

Comment 44

[Boris Zbarsky [:bzbarsky]]

No, what I meant was I was wondering why we don't leak via all the other hashtables the binding manager already maintains....

Comment 45

[Lukas Blakk [:lsblakk] use ?needinfo]

We're a week away from putting Firefox 20 onto the Beta channel, any new information or leads on the work here?

Comment 46

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 47

[Asaf Romano (gone)]

This doesn't affect the downloads view anymore, so it does not block.