128258 - Recent Trunk builds crashing at [@ RemovePropertyTreeChild]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[greer]

There have been a spate of crashes at RemovePropertyTreChild in the Trunk builds
from 2/23 onward. This follows Brendan's update of the jsscope.c file on the
22nd. cc'ing Brendan. The checkin is pretty extensive and the comments not very
informative. I will add qawanted to see if someone can reproduce it.

RemovePropertyTreeChild   32 
Crash data range: 2002-02-23 to 2002-02-27
Build ID range: 2002022311 to 2002022709
 
Stack Trace: 

         RemovePropertyTreeChild       
[d:\builds\seamonkey\mozilla\js\src\jsscope.c  line 639]
         js_SweepScopeProperties       
[d:\builds\seamonkey\mozilla\js\src\jsscope.c  line 1434]
         js_GC  [d:\builds\seamonkey\mozilla\js\src\jsgc.c  line 1250]
         js_ForceGC     [d:\builds\seamonkey\mozilla\js\src\jsgc.c  line 975]
         js_DestroyContext      [d:\builds\seamonkey\mozilla\js\src\jscntxt.c 
line 228]
         JS_DestroyContext      [d:\builds\seamonkey\mozilla\js\src\jsapi.c 
line 893]
         nsJSContext::~nsJSContext
[d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp  line 450]
         nsJSContext::`scalar deleting destructor'
         nsJSContext::Release  
[d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp  line
474]
         nsCOMPtr_base::assign_with_AddRef
[d:\builds\seamonkey\mozilla\xpcom\glue\nsCOMPtr.cpp  line 74]
         nsXULPrototypeDocument::`scalar deleting destructor'
         _hashEnumerate [d:\builds\seamonkey\mozilla\xpcom\ds\nsHashtable.cpp 
line 199]
         PL_HashTableEnumerateEntries   [../../../lib/ds/plhash.c  line 430]
         nsHashtable::Enumerate
[d:\builds\seamonkey\mozilla\xpcom\ds\nsHashtable.cpp  line 362]
         nsSupportsHashtable::~nsSupportsHashtable
[d:\builds\seamonkey\mozilla\xpcom\ds\nsHashtable.cpp  line 866]
         nsXULPrototypeCache::~nsXULPrototypeCache
[d:\builds\seamonkey\mozilla\content\xul\document\src\nsXULPrototypeCache.cpp 
line 168]
         nsXULPrototypeCache::Release
[d:\builds\seamonkey\mozilla\content\xul\document\src\nsXULPrototypeCache.cpp 
line 168]
         nsServiceManager::ReleaseService
[d:\builds\seamonkey\mozilla\xpcom\components\nsServiceManagerObsolete.cpp  line
128]
         nsXBLService::~nsXBLService
[d:\builds\seamonkey\mozilla\content\xbl\src\nsXBLService.cpp  line 608]
         nsXBLService::`scalar deleting destructor'
         nsXBLDocumentInfo::Release
[d:\builds\seamonkey\mozilla\content\xbl\src\nsXBLDocumentInfo.cpp  line 316]
         nsCSSFrameConstructor::ReleaseGlobals 
[..\html\style\src\nsCSSFrameConstructor.h]
         Shutdown      
[d:\builds\seamonkey\mozilla\layout\build\nsLayoutModule.cpp  line 168]
         nsGenericModule::Shutdown
[d:\builds\seamonkey\mozilla\xpcom\components\nsGenericFactory.cpp  line 290]
         nsGenericModule::`scalar deleting destructor'
         nsObserverService::Release    
[d:\builds\seamonkey\mozilla\xpcom\ds\nsObserverService.cpp
line 72]
         nsDll::Shutdown       
[d:\builds\seamonkey\mozilla\xpcom\components\xcDll.cpp  line 489]
         nsFreeLibrary 
[d:\builds\seamonkey\mozilla\xpcom\components\nsNativeComponentLoader.cpp
line 384]
         nsFreeLibraryEnum
[d:\builds\seamonkey\mozilla\xpcom\components\nsNativeComponentLoader.cpp  line 433]
         _hashEnumerate [d:\builds\seamonkey\mozilla\xpcom\ds\nsHashtable.cpp 
line 199]
         PL_HashTableEnumerateEntries   [../../../lib/ds/plhash.c  line 430]
         nsHashtable::Enumerate
[d:\builds\seamonkey\mozilla\xpcom\ds\nsHashtable.cpp  line 362]
         nsNativeComponentLoader::UnloadAll
[d:\builds\seamonkey\mozilla\xpcom\components\nsNativeComponentLoader.cpp  line
1015]
         nsComponentManagerImpl::UnloadLibraries
[d:\builds\seamonkey\mozilla\xpcom\components\nsComponentManager.cpp  line 3046]
         nsComponentManagerImpl::Shutdown
[d:\builds\seamonkey\mozilla\xpcom\components\nsComponentManager.cpp  line 863]
         NS_ShutdownXPCOM      
[d:\builds\seamonkey\mozilla\xpcom\build\nsXPComInit.cpp  line 575]
         main   [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp 
line 1648]
         WinMain       
[d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp  line 1658]
         WinMainCRTStartup()
         kernel32.dll + 0x1eb69 (0x77e5eb69)
 
        Source File :
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsscope.c line :
639
     (3450451)  URL: had only been mozilla to view PHPost 1.07 from a local
apache server
     (3450451)  Comments: closed down Mozilla  crashed.
     (3445174)  Comments: Crash when sending link in Mail Compose
     (3436283)  Comments: Exiting Mail after a mail/browser usage session
     (3421804)  URL: http://buzilla.mozilla.org
     (3421804)  Comments: just did a bugzilla search
     (3418959)  Comments: Closing browser
     (3414351)  Comments: error happened when closing email program  then
closing the mail filters dialog.
     (3400388)  Comments: Simply closing the Mail window caused this crash. I
did not do anything out of the ordinary  besides close it :)
     (3366235)  Comments: quitting chatzilla
     (3358435)  Comments: I was typing in a textfield at freeride.com
     (3340193)  Comments: closing mozilla mail program
     (3336884)  URL: www.beneworld.com
     (3336884)  Comments: Exiting the email client
     (3304162)  URL: ANY news:// starts a wizard for A MAIL account..  :(
     (3289384)  URL: www.ontariosciencecenter.ca
     (3289384)  Comments: closed mozilla and then crashed

Comment 1

[Mike Shaver (:shaver emeritus)]

Do we have a reproducable test case for this?  I'd love to see it under the
debugger.

Comment 2

[Brendan Eich [:brendan]]

Argh.

/be

Comment 3

[Brendan Eich [:brendan]]

Ok, I see the bug, I even commented on the hard case at length
(http://lxr.mozilla.org/mozilla/source/js/src/jsscope.c#550).  The problem
arises during interleaved property-add/property-delete/GC cycles that reparent
kids to their grandparents.  You can end up with two nodes in the same ply of
the tree that match.  Both may have kids that many scopes point at (in addition
to many scopes pointing at both matching "duplicate children").

The code leaves the duplicate child out of the grandparent's kids (the ply where
it matches an existing node).  But it sets the duplicate child's parent to refer
to the grandparent.  Later, if the grandparent is GC'd, the duplicate child (or
children, there could be several due to repetition of this cycle) has   a
dangling parent member!

We can't set the duplicate child's parent member to null, because that would
mean it was adopted by the root of the tree, but it belongs to a scope where it
was added (modulo deletions, which caused it to be a duplicate child) after
other properties.  Enumeration would wrongly miss the true parent and other
nodes on the way up the tree to the root.  The data structures would not be
consistent, either: the ancestor line height would not be >= scope->entryCount
for scopes referring to nodes along the line.

The right answer seems to be to let the duplicates pile up.  This can happen in
much less likely circumstances due to multi-threaded races through
GetPropertyTreeChild, per the comment there and the big jsscope.h comment.  It
seems right to me, but it can make for more kids in a tree than we'd like.  If
anyone can reproduce this bug in a real-world Mozilla, I'd like to study the
frequency of duplicate child creation.  I can make a testcase to trigger it, but
that won't tell us about real-world likelihood.

Patch in a few minutes.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix

We need this for 0.9.9.  Prompt testing and reviewing appreciated.

/be

Comment 5

[Brendan Eich [:brendan]]

Asa, here's one regression my big footprint win from bug 62164's landing is
known to have caused.  Cc'ing cathleen for testing help, still need review, but
I hope to land this for 0.9.9 today.

/be

Comment 6

[Mike Shaver (:shaver emeritus)]

Comment on attachment 71975 [details] [diff] [review]
proposed fix

Some day, we're going to need to claim those dups!

r=shaver, thanks for the continuing education.

Comment 7

[Brendan Eich [:brendan]]

shaver, thanks -- your forwarding idea should help unify duplicate kids.  If we
forward live JSScopeProperty nodes always, and record the forwarding address in
the old node, we can also flag (with a bit included in SPROP_FLAGS_NOT_MATCHED,
see jsscope.c:370) the duplicate child nodes, and forward them to the matching,
unique-among-its-siblings node.

Want to do that in your GC bug?

/be

Comment 8

[John Bandhauer]

Comment on attachment 71975 [details] [diff] [review]
proposed fix

sr=jband. I'll buy that.

Comment 9

[Asa Dotzler [:asa]]

Comment on attachment 71975 [details] [diff] [review]
proposed fix

a=asa (on behalf of drivers) for checkin to 0.9.9 and the trunk

Comment 10

[Brendan Eich [:brendan]]

Fixed, trunk and 0.9.9 branch.

/be

Comment 11

[Phil Schwartau]

[e-mail]: could you mark this bug Verified if the crashes
you were seeing at RemovePropertyTreChild have gone away? Thanks; 
I was never able to reproduce this -

Comment 12

[Phil Schwartau]

typo: RemovePropertyTreeChild

Comment 13

[greer]

I don't see any crashes with this signature since the checkin on 3/1. Prior to
that there are daily crash incidents. VERIFIED.