383651 - Re-entrancy into nsCycleCollector::Collect can result in a crash

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew Miller]

Watching the size of mBuf to catch the point where re-entrancy occurs gives the below stack trace:

   1.
      Old value = 5059
   2.
      New value = 0
   3.
      nsDeque::Empty (this=0x80c4e98) at /home/amil082/mozilla_trunk/mozilla/xpcom/ds/nsDeque.cpp:148
   4.
      148       mOrigin=0;
   5.
      (gdb) bt
   6.
      #0  nsDeque::Empty (this=0x80c4e98) at /home/amil082/mozilla_trunk/mozilla/xpcom/ds/nsDeque.cpp:148
   7.
      #1  0xb7da962e in nsCycleCollector::Collect (this=0x80c4e60, aTryCollections=1) at /home/amil082/mozilla_trunk/mozilla/xpcom/base/nsCycleCollector.cpp:1922
   8.
      #2  0xb7da97d8 in nsCycleCollector_collect () at /home/amil082/mozilla_trunk/mozilla/xpcom/base/nsCycleCollector.cpp:2190
   9.
      #3  0xb4af1769 in nsJSContext::FireGCTimer (this=0xb0409af8, aLoadInProgress=0) at /home/amil082/mozilla_trunk/mozilla/dom/src/base/nsJSEnvironment.cpp:3270
  10.
      #4  0xb4af18c9 in nsJSContext::GC (this=0xb0409af8) at /home/amil082/mozilla_trunk/mozilla/dom/src/base/nsJSEnvironment.cpp:3082
  11.
      #5  0xb4b1cef9 in nsGlobalWindow::SetDocShell (this=0xb0409870, aDocShell=0x0) at /home/amil082/mozilla_trunk/mozilla/dom/src/base/nsGlobalWindow.cpp:1785
  12.
      #6  0xb43abfba in nsDocShell::Destroy (this=0xb0409200) at /home/amil082/mozilla_trunk/mozilla/docshell/base/nsDocShell.cpp:3501
  13.
      #7  0xb48c22e3 in nsFrameLoader::Destroy (this=0xb04090d8) at /home/amil082/mozilla_trunk/mozilla/content/base/src/nsFrameLoader.cpp:282
  14.
      #8  0xb4959585 in nsGenericHTMLFrameElement::UnbindFromTree (this=0xb0409030, aDeep=1, aNullParent=0)
  15.
          at /home/amil082/mozilla_trunk/mozilla/content/html/content/src/nsGenericHTMLElement.cpp:3059
  16.
      #9  0xb48d4a59 in nsGenericElement::UnbindFromTree (this=0xb0408ae8, aDeep=1, aNullParent=0)
  17.
          at /home/amil082/mozilla_trunk/mozilla/content/base/src/nsGenericElement.cpp:2078
  18.
      #10 0xb48d4a59 in nsGenericElement::UnbindFromTree (this=0xb0408870, aDeep=1, aNullParent=1)
  19.
          at /home/amil082/mozilla_trunk/mozilla/content/base/src/nsGenericElement.cpp:2078
  20.
      #11 0xb48d1b4d in nsGenericElement::cycleCollection::Unlink (this=0xb4f15764, p=0xb06ee030)
  21.
          at /home/amil082/mozilla_trunk/mozilla/content/base/src/nsGenericElement.cpp:3266
  22.
      #12 0xb7da8e5f in nsCycleCollector::CollectWhite (this=0x80c4e60, graph=@0xbf8d2f50)
  23.
          at /home/amil082/mozilla_trunk/mozilla/xpcom/base/nsCycleCollector.cpp:1360
  24.
      #13 0xb7da9726 in nsCycleCollector::Collect (this=0x80c4e60, aTryCollections=5) at /home/amil082/mozilla_trunk/mozilla/xpcom/base/nsCycleCollector.cpp:1996
  25.
      #14 0xb7da980f in nsCycleCollector::Shutdown (this=0x80c4e60) at /home/amil082/mozilla_trunk/mozilla/xpcom/base/nsCycleCollector.cpp:2040
  26.
      #15 0xb7da9846 in nsCycleCollector_shutdown () at /home/amil082/mozilla_trunk/mozilla/xpcom/base/nsCycleCollector.cpp:2206
  27.
      #16 0xb7d2383a in NS_ShutdownXPCOM_P (servMgr=0x0) at /home/amil082/mozilla_trunk/mozilla/xpcom/build/nsXPComInit.cpp:778
  28.
      #17 0xb7f497ab in ~ScopedXPCOMStartup (this=0xbf8d32a4) at /home/amil082/mozilla_trunk/mozilla/toolkit/xre/nsAppRunner.cpp:793
  29.
      #18 0xb7f4e6bd in XRE_main (argc=1, argv=0xbf8d3938, aAppData=0x8065840) at /home/amil082/mozilla_trunk/mozilla/toolkit/xre/nsAppRunner.cpp:2861
  30.
      #19 0x0804af9a in main (argc=1, argv=0xbf8d3938) at /home/amil082/mozilla_trunk/mozilla/xulrunner/app/nsXULRunnerApp.cpp:408

Comment 1

[Andrew Miller]

Attachment: Block re-entrancy to nsCycleCollector::Collect

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 267645 [details] [diff] [review]
Block re-entrancy to nsCycleCollector::Collect

Since you've got an early return, it would be better to put it outside rather than inside COLLECT_TIME_DEBUG.

This looks fine to me, so r+sr=dbaron, but I'd like to give graydon and peterv a chance to chime in if they see any reason why it should stay the way it was.

Comment 3

[Andrew Miller]

Attachment: As above, but with the check before the timer

This patch moves up the check to before COLLECT_TIME_DEBUG, as requested by dbaron in his previous review.

Once this patch is reviewed by graydon and peterv, please commit or set the status white-board, as I don't have cvs write access.

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 267657 [details] [diff] [review]
As above, but with the check before the timer

Marking sr by dbaron.

Comment 5

[Peter Van der Beken [:peterv]]

I checked this in, thanks for the patch!