387203 - getFolderContents() can crash [@nsNavHistoryContainerResultNode::CloseContainer] when JS GC happens

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[(not reading, please use [e-mail] instead)]

getFolderContents() can crash when JS GC happens

with the patch in #378558 I am crashing when I attempt to copy the "root" bookmark in the left hand pane of the bookmark organizer.

What I think is going is that JS GC is happening, and we are garbage collecting "result" [see getFolderContents()]

  getFolderContents:
  function PU_getFolderContents(aFolderId, aExcludeItems, aExpandQueries) {
    var query = this.history.getNewQuery();
    query.setFolders([aFolderId], 1);
    var options = this.history.getNewQueryOptions();
    options.setGroupingMode([Ci.nsINavHistoryQueryOptions.GROUP_BY_FOLDER], 1);
    options.excludeItems = aExcludeItems;
    options.expandQueries = aExpandQueries;

    var result = this.history.executeQuery(query, options);
    result.root.containerOpen = true;
    return asContainer(result.root);
  },

See attachment #1 [details] [diff] [review] for the stack to when we GC.

Later, I'll crash in nsNavHistoryContainerResultNode::CloseContainer() because result points to freed memory.  See attachment #2 [details] [diff] [review] for the stack

Not, this is obviously not 100% reproducable, depending on when GC runs.
(I think cyen didn't see it with a new profile, but did with an old one.)

To fix this, in nsNavHistoryContainerResultNode, make mResult be a nsCOMPtr instead of a regular nsNavHistoryResult *.  This way, mResult won't get collected until the node is no longer referenced.

I'll attach a patch.

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: stack to GC

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: stack to crash

Comment 3

[(not reading, please use [e-mail] instead)]

Attachment: patch to fix the crash

Comment 4

[(not reading, please use [e-mail] instead)]

this blocks cyen's bug, but crashes like this could be lurking in our code.  (I'll check breakpad / bugzilla.)

Comment 5

[(not reading, please use [e-mail] instead)]

Comment on attachment 271318 [details] [diff] [review]
patch to fix the crash

I think now I'm leaking nsNavHistoryResult and nsNavHistorQueryNode objects, as they own each other strongly.

working on it...

Comment 6

[Asaf Romano (gone)]

So we've a couple of issues here:
  1) Nodes cannot live without their containing result being strong-ref'ed somewhere else (in the caller).
  2) getFolderContents returns the root result node instead of the result itself.

To fix 1) you'd need to keep a strong reference to the result in the root node of it, implement cycle-collection, ...and cross your fingers.

We should fix 2) either way, I think.

Comment 7

[(not reading, please use [e-mail] instead)]

working on both #1 and #2.

FWIW, Jonas agrees about #1.

Comment 8

[(not reading, please use [e-mail] instead)]

Attachment: patch, v1

Comment 9

[(not reading, please use [e-mail] instead)]

Attachment: fix getFolderContents(), per mano

Comment 10

[Asaf Romano (gone)]

Comment on attachment 271686 [details] [diff] [review]
fix getFolderContents(), per mano

r=mano

Comment 11

[(not reading, please use [e-mail] instead)]

getFolderContents() cleanup landed:

Checking in places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  control
ler.js
new revision: 1.166; previous revision: 1.165
done
Checking in places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.93; previous revision: 1.92
done
Checking in places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.48; previous revision: 1.47
done
Checking in preferences/selectBookmark.js;
/cvsroot/mozilla/browser/components/preferences/selectBookmark.js,v  <--  select
Bookmark.js
new revision: 1.7; previous revision: 1.6
done

Comment 12

[(not reading, please use [e-mail] instead)]

Running with my patch, there are scenarious where we traverse the cycles, unlink, and release the objects during cycle collection.

But there are also some scenarios where we are leaking nsNavHistoryResult and nsNavHistoryContainerResultNode objects.

I'm not sure if we've always been leaking those objects or not, but I'll investigate.

Comment 13

[(not reading, please use [e-mail] instead)]

Attachment: updated patch

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 272069 [details] [diff] [review]
updated patch

seeking review for the crash fix.

will spin off the leak issue into another bug.

Comment 15

[(not reading, please use [e-mail] instead)]

Comment on attachment 272069 [details] [diff] [review]
updated patch

seeking additional review from Jonas.

Comment 16

[(not reading, please use [e-mail] instead)]

> will spin off the leak issue into another bug.

I have not checked in yet, due to the leak issue.

On simple startup / shutdown, there are new leaks are coming from the places toolbar and from exporting to bookmarks.html.  (I'm not opening the history or bookmark menu) 

before:

     |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
                                              Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
   0 TOTAL                                          29      201   256931        6 ( 1201.82 +/-  1707.26)   830208        5 ( 1191.39 +/-  1900.05)
  62 StringAdopt                                     1        1     2559        1 (   76.31 +/-    25.32)        0        0 (    0.00 +/-     0.00)
 443 nsLocalFile                                    88      176     2492        2 (  270.45 +/-   152.13)    14008        2 (  317.19 +/-   184.98)
 567 nsStringBuffer                                  8       24    32624        3 ( 4693.88 +/-  2283.80)    59778        3 ( 5255.39 +/-  2489.45)

after:

     |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
                                              Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
   0 TOTAL                                          29    93233   275197     2490 ( 1210.31 +/-  1682.38)   897915     2491 ( 1267.67 +/-  1965.81)
 465 nsNavHistoryContainerResultNode               144     5472       41       38 (   22.23 +/-    12.43)      400       38 (   32.13 +/-    12.46)
 466 nsNavHistoryFolderResultNode                  160     6080       39       38 (   20.45 +/-    11.61)      374       38 (   30.07 +/-    12.46)
 467 nsNavHistoryQuery                             112      224       11        2 (    5.40 +/-     2.33)       66        2 (    7.81 +/-     3.27)
 468 nsNavHistoryQueryOptions                       56      224       11        4 (    6.17 +/-     2.62)      163       40 (   23.40 +/-    14.89)
 470 nsNavHistoryResult                            144      288        5        2 (    3.00 +/-     1.31)      167       38 (   26.65 +/-    13.70)
 471 nsNavHistoryResultNode                        104    66976      647      644 (  325.48 +/-   187.75)     4066      644 (  461.51 +/-   209.92)
 569 nsStringBuffer                                  8    13512    33858     1689 ( 4763.13 +/-  2227.65)    63377     1689 ( 5234.77 +/-  2453.19)
 585 nsTArray_base                                   4      120     6412       30 (  706.45 +/-   234.27)        0        0 (    0.00 +/-     0.00)
 631 nsVoidArray                                     4      160     9927       40 ( 2107.78 +/-   981.31)        0        0 (    0.00 +/-     0.00)

I'm investigating.

Comment 17

[(not reading, please use [e-mail] instead)]

> I'm investigating.

Sliding out of M7 to M8.  Not a UI blocker, and I won't have time during M7 to track down this leak.

Comment 18

[Asaf Romano (gone)]

Comment on attachment 272069 [details] [diff] [review]
updated patch

let's see if we can figure out the leak by M8, then make the trade.

Comment 19

[(not reading, please use [e-mail] instead)]

this crash has showed up:

http://crash-stats.mozilla.com/report/index/b843f930-3d26-11dc-a7c2-001a4bd43e5c

Comment 20

[(not reading, please use [e-mail] instead)]

the leak might be fixed by bug #397221. 

I'll try again, as we need this.

Comment 21

[(not reading, please use [e-mail] instead)]

Attachment: updated to the trunk, but this still leaks

using XPCOM_MEM_LEAK_LOG, make-tree.pl, find-leakers.pl, and fix-macosx-stack.pl (thanks dbaron!) to figure out why.

Comment 22

[(not reading, please use [e-mail] instead)]

Attachment: updated to trunk (for myk), still no progress on the leak

Comment 24

[Peter Van der Beken [:peterv]]

Comment on attachment 284996 [details] [diff] [review]
updated to trunk (for myk), still no progress on the leak

>Index: src/nsNavHistoryResult.cpp
>===================================================================

>+NS_IMPL_CYCLE_COLLECTION_0(nsNavHistoryResultNode)

If you think these objects are part of a cycle then they must be holding a strong reference to something. This says they hold on to nothing, so they can't be part of a cycle.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Unless it's only used as baseclass for objects that do hold on to things.

Comment 26

[Jeff Walden [:Waldo]]

Do you need to traverse mRootNode in nsNavHistoryResultNode?

Comment 27

[(not reading, please use [e-mail] instead)]

> Do you need to traverse mRootNode in nsNavHistoryResultNode?

that's not a member of nsNavHistoryResultNode, but of nsNavHistoryResult, and I'm already traversing it.

Comment 28

[(not reading, please use [e-mail] instead)]

Attachment: output on console after setting XPCOM_MEM_LEAK_LOG environment variable

up to date leak info, for peterv and others

Comment 29

[(not reading, please use [e-mail] instead)]

I think I have it!  

I need to traverse and unlink mChildren (in addition to mResult), in the cycle collector macros for nsNavHistoryContainerResultNode.

triple checking now.

Comment 30

[Peter Van der Beken [:peterv]]

Heh, I'll just mid-aired trying to post this:

Shouldn't you traverse mChildren in nsNavHistoryContainerResultNode? Note that
the cycle collector won't collect a cycle unless it's aware of all the edges
that hold nodes in that cycle.

Comment 31

[(not reading, please use [e-mail] instead)]

Attachment: patch that also traverses and unlinks mChildren

Comment 32

[(not reading, please use [e-mail] instead)]

Attachment: patch for review

Comment 33

[(not reading, please use [e-mail] instead)]

Attachment: patch for review

Comment 34

[(not reading, please use [e-mail] instead)]

jonas, the changes since your last review can be viewed here:

https://bugzilla.mozilla.org/attachment.cgi?oldid=284996&action=interdiff&newid=285154&headers=1

Comment 35

[Jeff Walden [:Waldo]]

Comment on attachment 285154 [details] [diff] [review]
patch for review

>-NS_INTERFACE_MAP_BEGIN(nsNavHistoryResult)
>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsNavHistoryResult)
>   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsINavHistoryResult)
>   NS_INTERFACE_MAP_STATIC_AMBIGUOUS(nsNavHistoryResult)
>   NS_INTERFACE_MAP_ENTRY(nsINavHistoryResult)
>   NS_INTERFACE_MAP_ENTRY(nsINavBookmarkObserver)
>   NS_INTERFACE_MAP_ENTRY(nsINavHistoryObserver)
>   NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
>+  NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(nsNavHistoryResult)
> NS_INTERFACE_MAP_END

180 #define NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(_class) \
181   NS_INTERFACE_MAP_BEGIN(_class)                        \
182     NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(_class)   \

...suggests the second change here is unnecessary.

Comment 36

[(not reading, please use [e-mail] instead)]

Attachment: updated patch, per comment from waldo

thanks waldo.

Comment 37

[(not reading, please use [e-mail] instead)]

fixed

Checking in nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavB
ookmarks.cpp
new revision: 1.123; previous revision: 1.122
done
Checking in nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHis
tory.cpp
new revision: 1.178; previous revision: 1.177
done
Checking in nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  ns
NavHistoryResult.cpp
new revision: 1.119; previous revision: 1.118
done
Checking in nsNavHistoryResult.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.h,v  <--  nsNa
vHistoryResult.h
new revision: 1.48; previous revision: 1.47
done

Comment 38

[(not reading, please use [e-mail] instead)]

Attachment: attempt at a test case (but it doesn't fail without my patch, so not checking it in)

Comment 40

[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