520659 - Lazily build places trees when possible

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Asaf Romano (gone)]

See bug 498130 for background.

We can now avoid almost all of the work that is done in buildVisibleSection.

Comment 1

[Marco Bonardo [:mak]]

as a reference for drivers: buildVisibleSection is the point where we spend more time after we get the results from the backend, so it is worth to spend time on making it faster.

Comment 2

[Asaf Romano (gone)]

Attachment: working

Marco, you're welcome to measure!

Comment 3

[Asaf Romano (gone)]

7ms (with 2ms freeze)->3ms (with no freezes) on dietrich's box.

Comment 4

[Marco Bonardo [:mak]]

does this patch include all the other "requirements"?

btw, i'm going to test and report percentage of improvement.

Please clarify to me the patches dependencies, and ask reviews accordingly.

Comment 5

[Asaf Romano (gone)]

Yes (no dependencies).

Comment 6

[Marco Bonardo [:mak]]

OMG! this is what i call an improvement!
query time 5275
Full sidebar time 5310
before this patch full sidebar time was 10500 (the previously reported 40s were on a VM, while these are on a real machine).

This is making the treeview time completely transparent to the query time, and that's DAMN good!
Query time includes both the time to query and the time to fill result with nodes, that is the major pain. Query time is probably 1s, once we will be able to async fill the result it will be perfect.

Comment 7

[Marco Bonardo [:mak]]

PS: the above times are in a debug build, in opt builds we are talking about ms.

Comment 10

[Asaf Romano (gone)]

Attachment: ready for first pass

Reintroducing selection-restoration was hard. Finding out that some file-browsers don't even bother implementing that was harder.

Marco, time for initial reveiw.

Comment 11

[Marco Bonardo [:mak]]

Attachment: first-pass review

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 410247 [details] [diff] [review]
ready for first pass

first-pass done, attached above.
Changes are a lot, so i'll need more passes to get acquainted to the new algo.

Comment 13

[Asaf Romano (gone)]

Attachment: take 3

Comment 14

[Asaf Romano (gone)]

Marco: any update?

Comment 15

[Marco Bonardo [:mak]]

i will dedicate monday to reviews, i spent time on expiration this week.

Comment 16

[Marco Bonardo [:mak]]

Attachment: take 3 review

we're near, a bunch of typos and a couple minor things, next iteration should be good.

Comment 17

[Asaf Romano (gone)]

> should be an assertion instead?

No.  Most of the time we should actually convert NS_ASSERTs back to js exceptions.

Comment 18

[Asaf Romano (gone)]

> any reason to not use lastIndexOf? it should work with undefined.

I need *non*-undefined rows there.

Comment 19

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 20

[Asaf Romano (gone)]

Attachment: take 4

Comment 21

[Marco Bonardo [:mak]]

while i review, i have something to investigate for you, if i open the star panel, open the folder selector and click an expander for a non-selected root, it does not open, as soon as i select the container it opens.
so for example:
1. open star panel
2. open folder selector (bookmarks menu is selected)
3. click once the expander near bookmarks toolbar, nothing happens
4. select bookmarks toolbar, the container opens

Comment 22

[Marco Bonardo [:mak]]

also D&D in the library right pane seem to have some issue (view doesn't update?)
and i reached a situation where selecting in the left pane was not updating the right pane.

i can't tell offhand if this could be due to my local build, eventually we could produce a trybuild for testing.

Comment 23

[Marco Bonardo [:mak]]

Attachment: review4

code-wise things are fine, i think you should look at problems i reported in my last comments, then produce a trybuild that we can use for some deep testing, and we should be done.

Comment 24

[Asaf Romano (gone)]

Attachment: patch

Comment 25

[Marco Bonardo [:mak]]

looks like i still see some problem, i created a new profile, any tree is empty and pointing to an "invalid item" exception in tree.xml

Comment 26

[Marco Bonardo [:mak]]

actually:
Error: uncaught exception: [Exception... "'Illegal value' when calling method: [nsINavHistoryResultTreeViewer::nodeForTreeIndex]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/places/tree.xml :: get_selectedNode :: line 377"  data: no]

Comment 27

[Marco Bonardo [:mak]]

Attachment: review5

very few things, the only thing that concerns me is the above exception, that prevents me from doing further testing.
Please do some basic Library test on next revision.

Comment 28

[Asaf Romano (gone)]

Attachment: patch

Comment 29

[Marco Bonardo [:mak]]

this works much better.
I'm doing manual testing, for now i've found that if you drag in the library a node to the empty space below the last node you get:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavHistoryContainerResultNode.getChild]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/places/treeView.js :: PTV__getNodeForRow :: line 245"  data: no]

I think we had a similar problem even before this patch (different error clearly) so this doesn't block, but in case you know a simple fix...

Comment 30

[Marco Bonardo [:mak]]

in bookmarks sidebar, open a folder with some children in it, then try to drag the folder inside itself. Or open bookmarks sidebar, expand bookmarks menu, expand Mozilla Firefox folder, drag this folder above all other elements, usually on Bookmarks Menu it will start sending a bunch of exc:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsINavHistoryContainerResultNode.getChildIndex]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: chrome://browser/content/places/treeView.js :: PTV__getInsertionPoint :: line 1212"  data: no]

Looks like happening on dragover of a node (mostly container) on itself or an ancestor. when i reach this point just selecting a node inside the folder throws on getChildIndex till i dragmove the folder, then everything works again.

Comment 31

[Marco Bonardo [:mak]]

Attachment: screen, drag issue

dragging in bookmarks sidebar (actually dragging a child of Mozilla Firefox folder inside bookmarks menu) i ended up like this... As you can see the folder contents are duped inside the dragged bookmarks, as if it was a container.
So globally looks like there are issues with d&d in the bookmarks sidebar.

Comment 32

[Marco Bonardo [:mak]]

diff --git a/browser/components/places/content/treeView.js b/browser/components/places/content/treeView.js

+  _getRowForNode:
+  function PTV__getRowForNode(aNode, aForceBuild, aParentRow, aNodeIndex) {

+    if (row == -1 && aForceBuild === true) {
+      let parentRow = typeof(aParentRow) == "number" ? aParentRow
+                      : this._getRowForNode(parent);

fix indent please

Comment 33

[Marco Bonardo [:mak]]

code looks fine afaict, functionality is good excluding the d&d issues, so, once those are fixed we should be fine to check this in.

Comment 34

[Asaf Romano (gone)]

Attachment: patch (fix drag issue)

I'm almost sure the that the bug described in comment 29 is an old one.  Thus I didn't fix it.

The other d&d bug is a nice catch in my algorithm, thanks much!

Comment 35

[Marco Bonardo [:mak]]

actually, this works better, but i'm still able to make the bookmarks sidebar go in a bad state just dragging an open folder on all the other elements, then releasing it inside itself, and then dragging an element of that folder out of it... sorry i don't have perfect steps, but after some move everything goes crazy.
I can try to build better steps if you want.

And another minor issue: when dragging an open folder in another point, it closes (it should stay open).

Comment 36

[Asaf Romano (gone)]

Yes, STR are more than welcome.

Comment 37

[Marco Bonardo [:mak]]

So, new profile:
1. open bookmarks sidebar
2. open Mozilla Firefox folder
3. drag over the open folder above all the other entries in the sidebar going up (be sure to stop for a bit on Mozilla firefox, Bookmarks Menu, Bookmarks toolbar) then go down, when you'll be over Mozilla Firefox folder's contents you will start seeing exceptions.
4. drop it inside itself (so no action)
5. drag a bookmark child of Mozilla Firefox out of it, above inside Bookmarks Menu
6. get this assertion:

ASSERT: empty container
Stack Trace: 
0:(null)
1:_getInsertionPoint(10,-1)
2:get_insertionPoint()
3:PC__canInsert(true)
4:PC_isCommandEnabled(cmd_paste)
5:isCommandEnabled(cmd_paste)
6:goUpdateCommand(cmd_paste)
7:goUpdateGlobalEditMenuItems()
8:oncommandupdate([object Event @ 0x99e86a8 (native @ 0x9b92030)])
9:updateCommands(focus)
10:onxblfocus([object Event @ 0x99e8028 (native @ 0x9b91fd0)])

Comment 38

[Marco Bonardo [:mak]]

Comment on attachment 418544 [details] [diff] [review]
patch (fix drag issue)

can't review since d&d is still bogus.

Comment 39

[Asaf Romano (gone)]

Attachment: fix the drag issue #2

Had to fix some other minor bugs while I was there.

Comment 40

[Asaf Romano (gone)]

Attachment: without debug code

Comment 41

[Marco Bonardo [:mak]]

Some first comment, while i review:

d&d is working better, i can't see assertions.

When moving an expanded folder inside the same parent its open status is preserved, but if i move an expanded folder to another parent the open status is lost. So in the bookmarks sidebar dragging the open "Mozilla Firefox" folder from Bookmarks Menu to Unsorted Bookmarks, ends up with a closed "Mozilla Firefox" folder in unsorted bookmarks. I'd not block on this though, so eventually can follow-up.

toolkit/components/places/utils.js is messed up, first there is a typo repeated many times (4 or 5 entities like these):
let [node, shouldClose) = convertNode(aNode);
the round parenthesis is clearly wrong, last minute change? This is pretty evident since all views are empty when opening the browser :)

Also since all of these let [node, shouldClose] are in a switch, unless you put the cases in braces, javascript will complain about variable redefinitions. The parser just can't know that you are returning from the switch cases.

Comment 42

[Asaf Romano (gone)]

Are you sure that's a new bug? Please check.  If so, I will (try to) fix it here - not in a follow up.

Yes, some last minute change as usual.

Comment 43

[Marco Bonardo [:mak]]

(In reply to comment #42)
> Are you sure that's a new bug? Please check.  If so, I will (try to) fix it
> here - not in a follow up.

ha, it's not a new bug! i can repro in current nightly, i'll file it apart and nevermind the request.

Comment 44

[Marco Bonardo [:mak]]

filed  Bug 543720 -  Dragging an open container through different parents in the treeview closes the container

Comment 45

[Marco Bonardo [:mak]]

Comment on attachment 424560 [details] [diff] [review]
without debug code

So, this is r- just for the above issues in utils.js, and there are still a couple typos below, but this can land as soon as those are fixed.

Before the final review rou still need to:
- collect what's changed and file a Seamonkey bug depending on this, i suppose Neil will want to update their treeviews
- push this to the tryserver and get a full green on all unit&mochi tests

Then ask me final review, and land!

>diff --git a/browser/components/places/content/treeView.js b/browser/components/places/content/treeView.js

>+  _isPlainContainer: function PTV__isPlainContainer(aContainer) {
>+    if (aContainer._plainContainer !== undefined)
>+      return aContainer._plainContainer;
>+
>+    // We don't know enough about non-query containers.
>+    if (!(aContainer instanceof Ci.nsINavHistoryQueryResultNode))
>+      return aContainer._plainContainer = false;
>+
>+    let resultType = aContainer.queryOptions.resultType;
>+    switch (resultType) {
>+      case Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_QUERY:
>+      case Ci.nsINavHistoryQueryOptions.RESULTS_AS_SITE_QUERY:
>+      case Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_SITE_QUERY:
>+      case Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY:
>+        return aContainer._plainContainer = false;
>+    }
>+
>+    // If it's a folder, it's not a plain container.
>+    let nodeType = aContainer.type;
>+    return aContainer._plainContainer =
>+           (nodeType != Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER &&
>+            nodeType != Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT);
>+  },

even if they are broken and going away in this form, please fix the dynamic containers case, or otherwise add a todo comment.

>+    // Non-plain containers are initially built with their contents.
>+    let parentIsPlain = this._isPlainContainer(parent);
>+    if (!parentIsPlain) {
>+      if (parent == this._rootNode) {
>+        // Root node case.

useless comment, the code is self explaining thanks to nice naming.

>+        return this._rows.indexOf(aNode);
>+      }
>+
>+      return this._rows.indexOf(aNode, aParentRow);
>+    }
>+
>+    let row = -1;
>+    let useNodeIndex = typeof(aNodeIndex) == "number";
>+    if (parent == this._rootNode) {
>+      // Root node case.

ditto

>+  /**
>+   * Given a row, returns the node and the row of its parent.

Given a row, finds and returns the parent details of the associated node.

>+   *
>+   * @param aChildRow
>+   *        Row number.
>+   */
>+  _getParentByChildRow: function PTV__getParentByChildRow(aChildRow) {

add a "@return [parentNode, parentRow]" please


>+   * Tries to find an equivalent node for a node which was removed.

please try to explain what is considered "equivalent", when this method comes to hand and so on, nice comments are welcome :)

>+  _getNewRowForRemovedNode:
>+  function PTV__getNewRowForRemovedNode(aUpdatedContainer, aOldNode) {
>+    let parent = aOldNode.parent;
>+    if (parent) {
>+      // If the node's parent is still set, the node is not obsolete
>+      // and we should just find out its new position.
>+      //
>+      // However, if the node's parent is closed, the node is invisible.

that newline in comment is awful, add a common newline or remove it.

>   nodeInserted: function PTV_nodeInserted(aParentNode, aNode, aNewIndex) {


>-      for (var i = aNewIndex + 1; i < aParentNode.childCount; i++) {
>-        var viewIndex = aParentNode.getChild(i)._viewIndex;
>-        if (viewIndex >= 0) {
>-          // the view indices of subsequent children have not been shifted so
>-          // the next item will have what should be our index
>-          newViewIndex = viewIndex;
>+      let cc = aParentNode.cc;

uh? what is aParentNode.cc? maybe .childCount?

>+      let separatorsAreHidden = PlacesUtils.nodeIsSeparator(aNode) &&
>+        this._result.sortingMode != Ci.nsINavHistoryQueryOptions.SORT_BY_NONE;
>+      for (let i = aNewIndex + 1; i < cc; i++) {
>+        let node = aParentNode.getChild(i);
>+        if (!separatorsAreHidden || PlacesUtils.nodeIsSeparator(node)) {
>+          // The children have not been shifted so the next item will have what
>+          // should be our index.
>+          row = this._getRowForNode(node, false, parentRow, i);
>           break;
>         }
>       }

>-      this._tree.invalidateRow(aParentNode._viewIndex);
>+      this.invalidateContainer(aNode);

shouldn't you invalidate aParentNode?

>   getCellText: function PTV_getCellText(aRow, aColumn) {
>       case this.COLUMN_TYPE_DATE:
>         let nodeTime = node.time;
>+
>         if (nodeTime == 0 || !PlacesUtils.nodeIsURI(node)) {

hrm, this newline does not look useful

>diff --git a/toolkit/components/places/public/nsINavHistoryService.idl b/toolkit/components/places/public/nsINavHistoryService.idl

>+   * Look for a node in the container by some of its details.  Does not search
>+   * closed containers.
>+   *
>+   * @param aURI
>+   *        the node's uri attribute value
>+   * @param aTime
>+   *        the node's time attribute value.
>+   * @param aItemId
>+   *        the node's itemId attribute value.
>+   * @param aRecursive
>+   *        whether or not to search recursively.
>+   *
>+   * @throws NS_ERROR_NOT_AVAILABLE if containerOpen is false.

specify if this refers only to this container, or also to subcontainers in case this is recursive, i can guess people would ask themselves "will this throw if i specify aRecursive and a subcontainer is closed?"
And thinking to that this method has a pretty restricted usage, i can use it only if i know a node is visible. btw, won't block on it.

>+   * @return a result node that matches the given details if any, null
>+   *         otherwise.
>+   */
>+  nsINavHistoryResultNode findNodeByDetails(in AUTF8String aURIString,
>+                                            in PRTime aTime,
>+                                            in long long aItemId,
>+                                            in boolean aRecursive);

You need an SR for these API changes. ask mconnor eventually.

>diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp

>+NS_IMETHODIMP
>+nsNavHistoryContainerResultNode::GetChildIndex(nsINavHistoryResultNode* aNode,
>+                                               PRUint32* _retval)
>+{
>+  if (!mExpanded)
>+    return NS_ERROR_NOT_AVAILABLE;
>+
>+  *_retval = FindChild(static_cast<nsNavHistoryResultNode *>(aNode));

no space between nsNavHistoryResultNode and *

>diff --git a/toolkit/components/places/src/utils.js b/toolkit/components/places/src/utils.js

>   wrapNode: function PU_wrapNode(aNode, aType, aOverrideURI, aForceCopy) {

>-        var node = convertNode(aNode);
>+
>+        let [node, shouldClose) = convertNode(aNode);

as said in previous comment, typo and vars redefinition

>-        var node = convertNode(aNode);
>-        var dataUrl = gatherDataUrl(node);
>-        // Convert node could pass an open container node.
>-        if (self.nodeIsContainer(node))
>+
>+        let [node, shouldClose) = convertNode(aNode);

ditto, and won't repeat later.


>     // case this.TYPE_UNICODE:

even if it is not wrong, i think this commented out case kills readability
i'd say to make it a "default: // TYPE_UNICODE" or just convert to a readable comment "By default we will return TYPE_UNICODE" or whatever.

Comment 46

[Asaf Romano (gone)]

>-      this._tree.invalidateRow(aParentNode._viewIndex);
>+      this.invalidateContainer(aNode);

shouldn't you invalidate aParentNode?

No, it's now done on invalidation.

Comment 47

[Asaf Romano (gone)]

Attachment: now building

Comment 48

[Asaf Romano (gone)]

Comment on attachment 428216 [details] [diff] [review]
now building

Green.

Comment 49

[Marco Bonardo [:mak]]

(In reply to comment #46)
> >-      this._tree.invalidateRow(aParentNode._viewIndex);
> >+      this.invalidateContainer(aNode);
> 
> shouldn't you invalidate aParentNode?
> 
> No, it's now done on invalidation.

er, my question was about the fact invalidateContainer expects a container, i suppose, here aNode is the node passed in to nodeInserted, that could be a container or not. thus i was asking if this code should not instead be
+      this.invalidateContainer(aParentNode);

Comment 50

[Asaf Romano (gone)]

No, it invalidates aNode if it's a container...

Comment 51

[Marco Bonardo [:mak]]

Comment on attachment 428216 [details] [diff] [review]
now building

sounds good, thank you for following my crazy comments for so long time :)

Comment 52

[Asaf Romano (gone)]

Thanks for the in-death review.

Checked in: http://hg.mozilla.org/mozilla-central/rev/2c60006b6c1f

I filed bug 547815 for SeaMonkey.

Comment 53

[Marco Bonardo [:mak]]

Zpao is going to backout, this fails due to variables redefinition in utils.js as pointed out in comment 45.
You should put braces into case structures to avoid local variables redefinition or change code so that it's not an issue.
I wanted to fix it on the fly, but i should have clobbered my local build and that would have required too much time ;(

Comment 54

[Marco Bonardo [:mak]]

a useful log:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1266874895.1266879811.8485.gz

Comment 55

[Marco Bonardo [:mak]]

(In reply to comment #53)
> Zpao is going to backout, this fails due to variables redefinition in utils.js
> as pointed out in comment 45.

well, final part of comment 41 actually, i should have made that clearer.

Comment 56

[Marco Bonardo [:mak]]

backout (thanks zpao):
http://hg.mozilla.org/mozilla-central/rev/f17dbc97dae0
http://hg.mozilla.org/mozilla-central/rev/29f9e4224533

Comment 57

[Marco Bonardo [:mak]]

i have found another bug due to a typo, i have an updated patch, will attach it soon.

Comment 58

[Marco Bonardo [:mak]]

Attachment: corrects some orangeness

So, this fixes the vars redefinition, a strict warning in browser.js and a typo that prevents inline editing from working.

Unfortunatly sounds like there is still something wrong with the cycle collector, with this patch, running b-c tests gives me a bunch of:

TEST-PASS | chrome://mochikit/content/browser/../browser/browser/components/places/tests/browser/browser_library_infoBox.js | Additional info field correctly hidden after toggle: #editBMPanel_keywordRow
...
###!!! ASSERTION: Fault in cycle collector: traversed refs exceed refcount (ptr: ab26948)
: 'Not Reached', file c:/mozilla/mozilla-central/obj-pymake-i686-pc-msys/browser/xpcom/base/../../../../xpcom/base/nsCycleCollector.cpp, line 1144
xul!Fault+0x0000000000000012 (c:\mozilla\mozilla-central\xpcom\base\nscyclecollector.cpp, line 1188)
xul!scanVisitor::VisitNode+0x0000000000000037 (c:\mozilla\mozilla-central\xpcom\base\nscyclecollector.cpp, line 1747)
xul!GraphWalker<scanVisitor>::DoWalk+0x000000000000006D (c:\mozilla\mozilla-central\xpcom\base\nscyclecollector.cpp, line 1316)
xul!GraphWalker<scanVisitor>::WalkFromRoots+0x000000000000005B (c:\mozilla\mozilla-central\xpcom\base\nscyclecollector.cpp, line 1255)
xul!nsCycleCollector::ScanRoots+0x000000000000003B (c:\mozilla\mozilla-central\xpcom\base\nscyclecollector.cpp, line 1787)
xul!nsCycleCollector::BeginCollection+0x00000000000000AA (c:\mozilla\mozilla-central\xpcom\base\nscyclecollector.cpp, line 2663)
xul!nsCycleCollector_beginCollection+0x0000000000000018 (c:\mozilla\mozilla-central\xpcom\base\nscyclecollector.cpp, line 3228)
xul!XPCCycleCollectGCCallback+0x000000000000007B (c:\mozilla\mozilla-central\js\src\xpconnect\src\nsxpconnect.cpp, line 392)
mozjs!js_GC+0x0000000000000817 (c:\mozilla\mozilla-central\js\src\jsgc.cpp, line 3196)
mozjs!JS_GC+0x000000000000005F (c:\mozilla\mozilla-central\js\src\jsapi.cpp, line 2464)
xul!nsXPConnect::Collect+0x00000000000000CF (c:\mozilla\mozilla-central\js\src\xpconnect\src\nsxpconnect.cpp, line 479)
xul!nsCycleCollector::Collect+0x00000000000000FD (c:\mozilla\mozilla-central\xpcom\base\nscyclecollector.cpp, line 2521)
xul!nsCycleCollector_collect+0x000000000000001A (c:\mozilla\mozilla-central\xpcom\base\nscyclecollector.cpp, line 3216)
xul!nsJSContext::CC+0x000000000000003A (c:\mozilla\mozilla-central\dom\base\nsjsenvironment.cpp, line 3549)
xul!nsJSContext::IntervalCC+0x0000000000000034 (c:\mozilla\mozilla-central\dom\base\nsjsenvironment.cpp, line 3638)
xul!nsJSContext::MaybeCC+0x00000000000000A8 (c:\mozilla\mozilla-central\dom\base\nsjsenvironment.cpp, line 3615)
xul!nsJSContext::CCIfUserInactive+0x0000000000000013 (c:\mozilla\mozilla-central\dom\base\nsjsenvironment.cpp, line 3625)
xul!GCTimerFired+0x000000000000004F (c:\mozilla\mozilla-central\dom\base\nsjsenvironment.cpp, line 3664)
xul!nsTimerImpl::Fire+0x000000000000028E (c:\mozilla\mozilla-central\xpcom\threads\nstimerimpl.cpp, line 427)
xul!nsTimerEvent::Run+0x00000000000000A1 (c:\mozilla\mozilla-central\xpcom\threads\nstimerimpl.cpp, line 521)
xul!nsThread::ProcessNextEvent+0x00000000000001FA (c:\mozilla\mozilla-central\xpcom\threads\nsthread.cpp, line 527)
xul!NS_ProcessNextEvent_P+0x0000000000000053 (c:\mozilla\mozilla-central\obj-pymake-i686-pc-msys\browser\xpcom\build\nsthreadutils.cpp, line 250)
xul!mozilla::ipc::MessagePump::Run+0x00000000000000FD (c:\mozilla\mozilla-central\ipc\glue\messagepump.cpp, line 118)
xul!MessageLoop::RunInternal+0x0000000000000056 (c:\mozilla\mozilla-central\ipc\chromium\src\base\message_loop.cc, line 217)
xul!MessageLoop::RunHandler+0x0000000000000082 (c:\mozilla\mozilla-central\ipc\chromium\src\base\message_loop.cc, line 200)
xul!MessageLoop::Run+0x0000000000000043 (c:\mozilla\mozilla-central\ipc\chromium\src\base\message_loop.cc, line 174)
xul!nsBaseAppShell::Run+0x0000000000000050 (c:\mozilla\mozilla-central\widget\src\xpwidgets\nsbaseappshell.cpp, line 180)
xul!nsAppShell::Run+0x0000000000000042 (c:\mozilla\mozilla-central\widget\src\windows\nsappshell.cpp, line 240)
xul!nsAppStartup::Run+0x000000000000006A (c:\mozilla\mozilla-central\toolkit\components\startup\src\nsappstartup.cpp, line 183)
xul!XRE_main+0x0000000000002BBC (c:\mozilla\mozilla-central\toolkit\xre\nsapprunner.cpp, line 3489)
firefox!NS_internal_main+0x00000000000002B2 (c:\mozilla\mozilla-central\browser\app\nsbrowserapp.cpp, line 158)
firefox!wmain+0x000000000000011E (c:\mozilla\mozilla-central\toolkit\xre\nswindowswmain.cpp, line 120)
firefox!__tmainCRTStartup+0x00000000000001A8 (f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c, line 583)
firefox!wmainCRTStartup+0x000000000000000F (f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c, line 403)
kernel32!BaseThreadInitThunk+0x0000000000000012
ntdll!RtlInitializeExceptionChain+0x0000000000000063
ntdll!RtlInitializeExceptionChain+0x0000000000000036

Comment 59

[Marco Bonardo [:mak]]

Attachment: add missing addref and fix orangeness

uh, sounds like FindNodeByDetails was not addrefing the node. Adding an NS_IF_ADDREF has solved the cycle collector failure.

Mano, do you think this is fine? i'll push this new version to the tryserver, but locally i got a green.

Comment 60

[Marco Bonardo [:mak]]

to simplify changes are:
- added braces to case constructs to avoid var redefs
- corrected PlaceUtils -> PlacesUtils typo
- added NS_IF_ADDREF to FindNodeByDetails

Comment 61

[Marco Bonardo [:mak]]

i landed bug 544047, so, if that sticks, this will probably need a small unbitrot (a couple methods are in common).

Comment 62

[Marco Bonardo [:mak]]

Attachment: unbitrot, add missing addref, fix oranges.

unbitrotted since i had it in my mq.

Comment 63

[Marco Bonardo [:mak]]

tryserver got a green.

Comment 64

[Asaf Romano (gone)]

Comment on attachment 428450 [details] [diff] [review]
unbitrot, add missing addref, fix oranges.

Thanks Much.

Comment 65

[Asaf Romano (gone)]

http://hg.mozilla.org/mozilla-central/rev/f739190773f8

Comment 66

[[e-mail]]

Comment on attachment 428450 [details] [diff] [review]
unbitrot, add missing addref, fix oranges.

>   _buildVisibleSection:
>-  function PTV__buildVisibleSection(aContainer, aVisible, aToOpen, aVisibleStartIndex)
>+  function PTV__buildVisibleSection(aContainer, aFirstChildRow, aToOpen)
>   {
...
>-          this._buildVisibleSection(curChild, aVisible, aToOpen, aVisibleStartIndex);
>+          rowsAddedCounter += this._buildVisibleSection(curChild, aToOpen,
>+                                                        row + 1);
Oops... should have been (curChild, row + 1, aToOpen);

And no, I didn't spot it when KaiRo ported it to SeaMonkey :-(