Closed Bug 520659 Opened 10 years ago Closed 10 years ago

Lazily build places trees when possible

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.7a2

People

(Reporter: mano, Assigned: mano)

References

(Depends on 1 open bug)

Details

(Keywords: perf)

Attachments

(2 files, 16 obsolete files)

25.45 KB, image/JPEG
Details
75.86 KB, patch
mano
: review+
Details | Diff | Splinter Review
See bug 498130 for background.

We can now avoid almost all of the work that is done in buildVisibleSection.
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.
Keywords: perf
OS: Mac OS X → All
Hardware: x86 → All
Attached patch working (obsolete) — Splinter Review
Marco, you're welcome to measure!
7ms (with 2ms freeze)->3ms (with no freezes) on dietrich's box.
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.
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.
Summary: Improve places' treeview perfomance by paging → Lazily build places trees when possible
PS: the above times are in a debug build, in opt builds we are talking about ms.
No longer depends on: 520746
Duplicate of this bug: 520746
No longer depends on: 524445
Duplicate of this bug: 524445
Attached patch ready for first pass (obsolete) — Splinter Review
Reintroducing selection-restoration was hard. Finding out that some file-browsers don't even bother implementing that was harder.

Marco, time for initial reveiw.
Attachment #408987 - Attachment is obsolete: true
Attachment #410247 - Flags: review?(mak77)
Attached file first-pass review (obsolete) —
Attachment #410247 - Flags: review?(mak77)
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.
Attached patch take 3 (obsolete) — Splinter Review
Attachment #410247 - Attachment is obsolete: true
Attachment #410644 - Attachment is obsolete: true
Attachment #412640 - Flags: review?(mak77)
i will dedicate monday to reviews, i spent time on expiration this week.
Attached file take 3 review (obsolete) —
we're near, a bunch of typos and a couple minor things, next iteration should be good.
Attachment #412640 - Flags: review?(mak77)
> should be an assertion instead?

No.  Most of the time we should actually convert NS_ASSERTs back to js exceptions.
> any reason to not use lastIndexOf? it should work with undefined.

I need *non*-undefined rows there.
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Attached patch take 4 (obsolete) — Splinter Review
Attachment #412640 - Attachment is obsolete: true
Attachment #414048 - Attachment is obsolete: true
Attachment #415123 - Flags: review?(mak77)
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
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.
Attached file review4 (obsolete) —
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.
Attachment #415123 - Flags: review?(mak77)
Attached patch patch (obsolete) — Splinter Review
Attachment #415123 - Attachment is obsolete: true
Attachment #415632 - Attachment is obsolete: true
Attachment #416417 - Flags: review?(mak77)
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
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]
Attached file review5 (obsolete) —
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.
Attachment #416417 - Flags: review?(mak77)
Attached patch patch (obsolete) — Splinter Review
Attachment #416417 - Attachment is obsolete: true
Attachment #416674 - Attachment is obsolete: true
Attachment #418148 - Flags: review?(mak77)
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...
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.
Attached image 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.
Attachment #418440 - Attachment is patch: false
Attachment #418440 - Attachment mime type: text/plain → image/JPEG
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
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.
Attachment #418148 - Flags: review?(mak77) → review-
Attached patch patch (fix drag issue) (obsolete) — Splinter Review
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!
Attachment #418148 - Attachment is obsolete: true
Attachment #418544 - Flags: review?(mak77)
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).
Yes, STR are more than welcome.
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 on attachment 418544 [details] [diff] [review]
patch (fix drag issue)

can't review since d&d is still bogus.
Attachment #418544 - Flags: review?(mak77)
Attached patch fix the drag issue #2 (obsolete) — Splinter Review
Had to fix some other minor bugs while I was there.
Attachment #418544 - Attachment is obsolete: true
Attachment #424559 - Flags: review?(mak77)
Attached patch without debug code (obsolete) — Splinter Review
Attachment #424559 - Attachment is obsolete: true
Attachment #424560 - Flags: review?(mak77)
Attachment #424559 - Flags: review?(mak77)
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.
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.
(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.
filed  Bug 543720 -  Dragging an open container through different parents in the treeview closes the container
Attachment #424560 - Flags: review?(mak77) → review-
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.
>-      this._tree.invalidateRow(aParentNode._viewIndex);
>+      this.invalidateContainer(aNode);

shouldn't you invalidate aParentNode?

No, it's now done on invalidation.
Attached patch now building (obsolete) — Splinter Review
Attachment #424560 - Attachment is obsolete: true
(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);
No, it invalidates aNode if it's a container...
Comment on attachment 428216 [details] [diff] [review]
now building

sounds good, thank you for following my crazy comments for so long time :)
Attachment #428216 - Flags: review?(mak77) → review+
Thanks for the in-death review.

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

I filed bug 547815 for SeaMonkey.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 547815
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 ;(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
i have found another bug due to a typo, i have an updated patch, will attach it soon.
Attached patch corrects some orangeness (obsolete) — Splinter Review
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
Attachment #428216 - Attachment is obsolete: true
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.
Attachment #428438 - Attachment is obsolete: true
Attachment #428439 - Flags: review?(mano)
to simplify changes are:
- added braces to case constructs to avoid var redefs
- corrected PlaceUtils -> PlacesUtils typo
- added NS_IF_ADDREF to FindNodeByDetails
i landed bug 544047, so, if that sticks, this will probably need a small unbitrot (a couple methods are in common).
unbitrotted since i had it in my mq.
Attachment #428439 - Attachment is obsolete: true
Attachment #428450 - Flags: review?(mano)
Attachment #428439 - Flags: review?(mano)
tryserver got a green.
Comment on attachment 428450 [details] [diff] [review]
unbitrot, add missing addref, fix oranges.

Thanks Much.
Attachment #428450 - Flags: review?(mano) → review+
http://hg.mozilla.org/mozilla-central/rev/f739190773f8
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a2
Version: unspecified → Trunk
No longer blocks: 549192
Depends on: 549192
Depends on: 549491
Blocks: 528723
Blocks: 555293
Depends on: 555651
Depends on: 561848
Blocks: 566613
Depends on: 608501
Blocks: 558127
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 :-(
Depends on: 628258
Depends on: 741064
Depends on: 811772
You need to log in before you can comment on or make changes to this bug.