334104 - WAY_TOO_MUCH_GC crash opening Firefox bookmarks menu, doing GC inside XPC_WN_Shared_ToString

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

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

Opening Firefox's bookmarks menu under WAY_TOO_MUCH_GC, I crash here:

#0  0x00002ba7e90a1dd1 in XPCNativeSet::IsMarked (this=0x6)
    at /home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpcprivate.h:1410
#1  0x00002ba7e90a2130 in XPCNativeSet::Mark (this=0x6)
    at /home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpcinlines.h:545
#2  0x00002ba7e90c4f60 in XPCJSRuntime::GCCallback (cx=0xa6fed0,
    status=JSGC_FINALIZE_END)
    at /home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpcjsruntime.cpp:392
#3  0x00002aaaad254966 in DOMGCCallback (cx=Variable "cx" is not available.
)
    at /home/dbaron/builds/trunk/mozilla/dom/src/base/nsJSEnvironment.cpp:2204
#4  0x00002ba7e4bea502 in js_GC (cx=0xa6fed0, gcflags=5)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:2274
#5  0x00002ba7e4beafa3 in js_NewGCThing (cx=0xa6fed0, flags=1, nbytes=16)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:698
#6  0x00002ba7e4c33f02 in js_NewString (cx=0xa6fed0, chars=0x13e23a0,
    length=67, gcflag=Variable "gcflag" is not available.
)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsstr.c:2520
#7  0x00002ba7e4bbff2e in JS_NewString (cx=0xa6fed0,
    bytes=0x13e1760 "[xpconnect wrapped nsIRDFResource @ 0x13dcd60 (native @ 0x13dcd00)]", nbytes=67) at /home/dbaron/builds/trunk/mozilla/js/src/jsapi.c:4255
#8  0x00002ba7e90e04f7 in ToStringGuts (ccx=@0x7fffffabe950)
    at /home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:108
#9  0x00002ba7e90e05d6 in XPC_WN_Shared_ToString (cx=Variable "cx" is not available.
)
    at /home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:128


It looks like the XPCCallContext created in frame #9 never had its mSet initialized, but CanGetSet returns true because SetArgsAndResultPtr advanced mState to HAVE_ARGS (past HAVE_NAME).  It seems like:
 * SetArgsAndResultPtr should CHECK_STATE(HAVE_NAME)
 * something should SetName in this process, or do something else that would cause mSet not to be uninitialized when CanGetSet returns true.

At least I think that's what's happening.

Comment 1

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

(Note that this is not actually something being prematurely garbage collection, merely that we crash when running garbage collection inside XPC_WN_Shared_ToString.)

Comment 2

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

Attachment: patch

This fixes it; I used the same name that the other caller of ToStringGuts uses.

Changing the assertion at the start of XPCCallContext::SetArgsAndResultPtr probably requires a lot of work; it might even be easier to flip the order of the two states, based on looking at the callers.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 218513 [details] [diff] [review]
patch

sr=jst

Comment 4

[David Bradley]

Comment on attachment 218513 [details] [diff] [review]
patch

r=dbradley

Looks good to me. I wonder if SetName should just be moved into ToStringGuts, but I'm fine with doing it outside.

Comment 5

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

Fix checked in to trunk; filed bug 334744 to follow up on the XPCCallContext issues that allowed this to happen.

Comment 6

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

Fix checked in to MOZILLA_1_8_BRANCH.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 218513 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 8

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

Fix checked in to MOZILLA_1_8_0_BRANCH.

Comment 9

[Dave Liebreich [:davel]]

Please provide guidance on how to verify this fix on the 180 branch

Comment 10

[Bob Clary [:bc] (inactive)]

Bookmark menu opens ok in Linux opt/debug 1.5.0.4 and Windows debug 1.5.0.4 builds WAY_TOO_MUCH_GC. Window opt can't be tested to due an unrelated startup crash. See Bug 307560 comment 16.

verified fixed 1.5.0.4.