Bugzilla

Robert O'Callahan (:roc) (email my personal email if necessary)

Assignee

Comment 1

•

17 years ago

Attached patch v1 (obsolete) — Details — Splinter Review

Any chance for steps to reproduce or a testcase? Delaying the release of objects in released cycles (like this patch does) should fix it, but it'd be nice to verify that it does.

Assignee: pavlov → peterv

Status: NEW → ASSIGNED

Reporter

Comment 2

•

17 years ago

Sorry, no.

Jonas Sicking (:sicking) No longer reading bugmail consistently

Comment 3

•

17 years ago

Should we also try to stop the event from firing synchronously from the dtor? Objects going away seems like it could happen when it's not safe to run scripts

Boris Zbarsky [:bzbarsky]

Comment 4

•

17 years ago

The problem is that the image node is going away, so we have to tell the image request proxy about that.

Given that we have cycle collector now maybe a better approach is to have strong refs both ways (instead of the weak ref imagelib has now, which we introduced to fix leaks) and just have them all cycle collect each other...  That should work OK even with stuff like jpeg push, since necko has a ref to the imgRequest, not the proxy.

Then again, imgRequestProxy's destructor doesn't remove it from the loadgroup, so we could end up with a dead object in the loadgroup if we did things that way.  Or if _would_ remove itself, and we'd be back in the situation where we are now.  :(

Perhaps the loadgroup should fire onload async, but that's a pretty scary change.

Assignee

Comment 5

•

17 years ago

Attached patch v1.1 (obsolete) — Details — Splinter Review

I think we should do this regardless of whether we do comment 3, we don't know what other destructor code ends up in JS.
The patch splits off the unrooting of white objects from the rest of cycle collection. XPConnect then does that unrooting (through FinishCollection) when getting a callback for JSGC_END (at which point it should be safe to call into JS). This is similar to how XPConnect does deferred releases. I also made us do only one cycle collection per call to nsXPConnect::Collect, even when the JS GC restarts. Restarting doesn't call JSGC_END, so we'd have to accumulate all the white objects from the multiple cycle collections. It seemed to make things a bit too complicated. From testing it doesn't seem to change much to do only one cycle collection (we'll still repeatedly call nsXPConnect::Collect on shutdown). I also made XPCWrappedNative's Root/Unroot calls noops, we don't want to AddRef the XPCWrappedNative between JSGC_MARK_END and JSGC_END, otherwise XPConnect will root the XPCWrappedNative's JS objects and the XPCWrappedNative would be uncollectable.

Attachment #288196 - Attachment is obsolete: true

Attachment #289491 - Flags: superreview?(jst)

Attachment #289491 - Flags: review?(dbaron)

Assignee

Comment 6

•

17 years ago

Attached patch v1.2 (obsolete) — Details — Splinter Review

Need to keep the PtrInfo's alive until FinishCollection is called, so make the graph a member of nsCycleCollector.

Attachment #289491 - Attachment is obsolete: true

Attachment #289818 - Flags: superreview?(jst)

Attachment #289818 - Flags: review?(dbaron)

Attachment #289491 - Flags: superreview?(jst)

Attachment #289491 - Flags: review?(dbaron)

Robert O'Callahan (:roc) (email my personal email if necessary)

Assignee

Comment 7

•

17 years ago

Attached patch v1.3 (obsolete) — Details — Splinter Review

Need to return something in NS_IMETHOD.

Attachment #289818 - Attachment is obsolete: true

Attachment #290589 - Flags: superreview?(jst)

Attachment #290589 - Flags: review?(dbaron)

Attachment #289818 - Flags: superreview?(jst)

Attachment #289818 - Flags: review?(dbaron)

Reporter

Comment 8

•

17 years ago

I applied this patch but I'm still crashing with this assertion. I'll try to get a stack.

Robert O'Callahan (:roc) (email my personal email if necessary)

Reporter

Comment 9

•

17 years ago

Here's a stack:

#0  JS_Assert (s=0x11026d0 "!rt->gcRunning", file=0x1101e0c "/Users/roc/mozilla-checkin/mozilla/js/src/jsgc.c", ln=1336) at /Users/roc/mozilla-checkin/mozilla/js/src/jsutil.c:63
#1  0x0105d639 in js_NewGCThing (cx=0x30b75840, flags=7, nbytes=8) at /Users/roc/mozilla-checkin/mozilla/js/src/jsgc.c:1336
#2  0x01018e36 in JS_NewExternalString (cx=0x30b75840, chars=0x3f797f80, length=35, type=0) at /Users/roc/mozilla-checkin/mozilla/js/src/jsapi.c:2513
#3  0x12a244f7 in XPCConvert::NativeData2JS (ccx=@0xbfffcce0, d=0xbfffcdc4, s=0xbfffcfd4, type=@0xbfffce49, iid=0xbfffce18, scope=0x34abb060, pErr=0x0) at /Users/roc/mozilla-checkin/mozilla/js/src/xpconnect/src/xpcconvert.cpp:409
#4  0x12a3f68a in nsXPCWrappedJSClass::CallMethod (this=0x34b78510, wrapper=0x34b787f0, methodIndex=6, info=0x20e8838, nativeParams=0xbfffcfd4) at /Users/roc/mozilla-checkin/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1341
#5  0x12a3828f in nsXPCWrappedJS::CallMethod (this=0x34b787f0, methodIndex=6, info=0x20e8838, params=0xbfffcfd4) at /Users/roc/mozilla-checkin/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp:567
#6  0x01380c8c in PrepareAndDispatch (self=0x34b78830, methodIndex=6, args=0xbfffd0f4) at /Users/roc/mozilla-checkin/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_unixish_x86.cpp:93
#7  0x01380de0 in nsXPTCStubBase::Stub6 (this=0x34b78830) at ../../../../../../dist/include/xpcom/xptcstubsdef.inc:8
#8  0x2b1b0a19 in nsIOService::NewURI (this=0x2926510, aSpec=@0xbfffd22c, aCharset=0x44942be8 "UTF-8", aBaseURI=0x34752f40, result=0xbfffd384) at /Users/roc/mozilla-checkin/mozilla/netwerk/base/src/nsIOService.cpp:500
#9  0x002536b8 in NS_NewURI (result=0xbfffd384, spec=@0xbfffd22c, charset=0x44942be8 "UTF-8", baseURI=0x34752f40, ioService=0x2926510) at ../../../dist/include/xpcom/nsCOMPtr.h:127
#10 0x13e29903 in NS_NewURI (result=0xbfffd384, spec=@0xbfffd2ec, charset=0x44942be8 "UTF-8", baseURI=0x34752f40, ioService=0x2926510) at ../../dist/include/xpcom/nsServiceManagerUtils.h:138
#11 0x13929dd1 in nsContentUtils::NewURIWithDocumentCharset (aResult=0xbfffd384, aSpec=@0xbfffd2ec, aDocument=0x42567800, aBaseURI=0x34752f40) at /Users/roc/mozilla-checkin/mozilla/content/base/src/nsContentUtils.cpp:1829
#12 0x139eccbb in nsGenericHTMLElement::GetHrefURIForAnchors (this=0x35372250, aURI=0xbfffd384) at /Users/roc/mozilla-checkin/mozilla/content/html/content/src/nsGenericHTMLElement.cpp:1315
#13 0x139ecd56 in nsGenericHTMLElement::IsHTMLLink (this=0x35372250, aURI=0xbfffd384) at /Users/roc/mozilla-checkin/mozilla/content/html/content/src/nsGenericHTMLElement.cpp:1292
#14 0x139f591e in nsHTMLAnchorElement::IsLink (this=0x35372250, aURI=0xbfffd384) at /Users/roc/mozilla-checkin/mozilla/content/html/content/src/nsHTMLAnchorElement.cpp:294
#15 0x1395000d in nsDocument::ForgetLink (this=0x42567800, aContent=0x35372250) at /Users/roc/mozilla-checkin/mozilla/content/base/src/nsDocument.cpp:5904
#16 0x139f6667 in nsHTMLAnchorElement::UnbindFromTree (this=0x35372250, aDeep=1, aNullParent=1) at /Users/roc/mozilla-checkin/mozilla/content/html/content/src/nsHTMLAnchorElement.cpp:204
#17 0x1396dc17 in nsGenericElement::cycleCollection::Unlink (this=0x13ffc140, p=0x353721b0) at /Users/roc/mozilla-checkin/mozilla/content/base/src/nsGenericElement.cpp:3361
#18 0x0137f116 in nsCycleCollector::CollectWhite (this=0xa7000) at /Users/roc/mozilla-checkin/mozilla/xpcom/base/nsCycleCollector.cpp:1542
#19 0x0137fb93 in nsCycleCollector::BeginCollection (this=0xa7000) at /Users/roc/mozilla-checkin/mozilla/xpcom/base/nsCycleCollector.cpp:2252
#20 0x0137fbd2 in nsCycleCollector_beginCollection () at /Users/roc/mozilla-checkin/mozilla/xpcom/base/nsCycleCollector.cpp:2694
#21 0x12a0ad3e in XPCCycleCollectGCCallback (cx=0x30b75840, status=JSGC_MARK_END) at /Users/roc/mozilla-checkin/mozilla/js/src/xpconnect/src/nsXPConnect.cpp:428
#22 0x010607d0 in js_GC (cx=0x30b75840, gckind=GC_NORMAL) at /Users/roc/mozilla-checkin/mozilla/js/src/jsgc.c:2526
#23 0x01018c1f in JS_GC (cx=0x30b75840) at /Users/roc/mozilla-checkin/mozilla/js/src/jsapi.c:2383
#24 0x12a07476 in nsXPConnect::Collect (this=0x292d9f0) at /Users/roc/mozilla-checkin/mozilla/js/src/xpconnect/src/nsXPConnect.cpp:518
#25 0x0137fcb1 in nsCycleCollector::Collect (this=0xa7000, aTryCollections=1) at /Users/roc/mozilla-checkin/mozilla/xpcom/base/nsCycleCollector.cpp:2132
#26 0x0137fd54 in nsCycleCollector_collect () at /Users/roc/mozilla-checkin/mozilla/xpcom/base/nsCycleCollector.cpp:2688
#27 0x13b266c2 in nsJSContext::CC () at /Users/roc/mozilla-checkin/mozilla/dom/src/base/nsJSEnvironment.cpp:3313
#28 0x13b26783 in nsJSContext::MaybeCC (aHigherProbability=0) at /Users/roc/mozilla-checkin/mozilla/dom/src/base/nsJSEnvironment.cpp:3335
#29 0x13b293b1 in nsUserActivityObserver::Observe (this=0x30b75180, aSubject=0x0, aTopic=0x13dc8c1c "user-interaction-active", aData=0x0) at /Users/roc/mozilla-checkin/mozilla/dom/src/base/nsJSEnvironment.cpp:279
#30 0x01325e2f in nsObserverList::NotifyObservers (this=0x226893c, aSubject=0x0, aTopic=0x13dc8c1c "user-interaction-active", someData=0x0) at /Users/roc/mozilla-checkin/mozilla/xpcom/ds/nsObserverList.cpp:128
#31 0x013264d9 in nsObserverService::NotifyObservers (this=0x2921bd0, aSubject=0x0, aTopic=0x13dc8c1c "user-interaction-active", someData=0x0) at /Users/roc/mozilla-checkin/mozilla/xpcom/ds/nsObserverService.cpp:181
#32 0x139c3ec2 in nsUITimerCallback::Notify (this=0x30ba4f20, aTimer=0x30ba1650) at /Users/roc/mozilla-checkin/mozilla/content/events/src/nsEventStateManager.cpp:208
#33 0x01372c4b in nsTimerImpl::Fire (this=0x30ba1650) at /Users/roc/mozilla-checkin/mozilla/xpcom/threads/nsTimerImpl.cpp:403
#34 0x01372e4f in nsTimerEvent::Run (this=0x34918080) at /Users/roc/mozilla-checkin/mozilla/xpcom/threads/nsTimerImpl.cpp:487
#35 0x0136ec37 in nsThread::ProcessNextEvent (this=0x2912840, mayWait=0, result=0xbfffdad4) at /Users/roc/mozilla-checkin/mozilla/xpcom/threads/nsThread.cpp:490

Robert O'Callahan (:roc) (email my personal email if necessary)

Reporter

Comment 10

•

17 years ago

I guess that's really a different bug, by the look of it.

Robert O'Callahan (:roc) (email my personal email if necessary)

Reporter

Comment 11

•

17 years ago

For this one, maybe nsHTMLAnchorElement should cache the link URI for IsLink and nsDocument::ForgetLink should call a version of IsLink that only uses the cached value if there is one? After all there should be no need to forget the link if IsLink has never returned one to any callers.

Robert O'Callahan (:roc) (email my personal email if necessary)

Reporter

Comment 12

•

17 years ago

Or just the PRUint32 hash should be cached, otherwise there'd be major memory bloat.

Boris Zbarsky [:bzbarsky]

Comment 13

•

17 years ago

Given that we're unlinking the element, and the element IsInDoc(), that means that the document is being unlinked too, right?

It might make sense to skip the unbind/remove here and have the document handle teardown of the whole DOM in that case.  Then the document could set a flag indicating that it's in such teardown and nsHTMLAnchorElement could check it before doing the ForgetLink thing.

In fact, that might be a good idea in other cases where calling ForgetLink is pointless (places where the doc clears the hashtable anyway).

Robert O'Callahan (:roc) (email my personal email if necessary)

Reporter

Comment 14

•

17 years ago

I just hit this stack again. The URL is "irc://irc.mozilla.org/%23airmozilla". Which suggests that it's Chatzilla triggering the problem (although the real problem is in our code of course).

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

Assignee

Comment 15

•

17 years ago

Could we move that to a different bug? It sort of is a separate issue, and we can't really defer unlinking like my patch does for release.

Comment 16

•

17 years ago

(In reply to comment #5)
> I also made XPCWrappedNative's Root/Unroot calls noops,

How do you guarantee that this is safe?  What if it and other things get Unlink calls?  Particularly if it wasn't kept alive through JS anymore?


Also, was the change from PR_TRUE to PR_FALSE in NS_CycleCollectorForget intentional?  If so, why?

jag (Peter Annema)

Comment 17

•

17 years ago

For comment 15, the new bug seems to be 407034.

Updated

•

17 years ago

Keywords: crash, dogfood, regression

Comment 18

•

17 years ago

Removing dogfood keyword, since this bug is older than the regression I'm hitting.  I guess I'm hitting bug 410323.

Keywords: dogfood, regression

Ryan VanderMeulen [:RyanVM]

Comment 19

•

17 years ago

I got easy steps to reproduce, at least on SeaMonkey:

 - Open Chatzilla
 - attach to a server
 - join a channel
 - leave the channel

For instance, load:
irc://freenode/#asdkadsj

and press control+w.

It always crash after 3 or 5 seconds.

Trunk CZ on Firefox should do the same.

Updated

•

17 years ago

Version: unspecified → Trunk

Comment 20

•

17 years ago

I'm still seeing the "gcrunning" assertion now that bug 410323 is fixed.  My stack looks like the one here: nsHTMLImageElement::~nsHTMLImageElement during GC leads to onload trying to fire during GC.

"WINNT 5.2 fxdbug-win32-tb Dep Debug + Leak Test" is still hitting it occasionally too, but I don't know what its stack trace looks like.

Keywords: dogfood

Comment 21

•

17 years ago

Jesse, I've crashed very few times with the stack and the "GetNewOrUsed" assertion from bug 410323, mainly on pages with intensive JS usage.

I crash much often with this stack here and I crashed with the same steps I used on comment 19 with a build with the fix from bug 410323 applied.

This CZ regression I posted is newer than this bug (I yet couldn't track the exact day), but the stack is almost the same.

Updated

•

17 years ago

OS: Mac OS X → All

Updated

•

17 years ago

Severity: normal → blocker

Reed Loden [:reed]

Comment 22

•

17 years ago

Upping to P1 since this is a dogfood blocker that is hurting both developers and tinderboxen.

Priority: P3 → P1

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

Assignee

Comment 23

•

17 years ago

(In reply to comment #16)
> > I also made XPCWrappedNative's Root/Unroot calls noops,
> 
> How do you guarantee that this is safe?  What if it and other things get Unlink
> calls?  Particularly if it wasn't kept alive through JS anymore?

XPCWrappedNative's Unlink doesn't do anything, so this should be fine.

> Also, was the change from PR_TRUE to PR_FALSE in NS_CycleCollectorForget
> intentional?  If so, why?

I think it makes more sense to return PR_TRUE, if the cycle collector has been shut down we already forgot about all purple nodes anyway. IIRC the change isn't needed for this bug, so I could undo it if you prefer.

Comment 24

•

17 years ago

Comment on attachment 290589 [details] [diff] [review]
v1.3

Yeah, I suppose the NS_CycleCollectorForget change does make sense.

nsCycleCollector.cpp has two "Didn't call Empty()?" that should, I
think, refer to Clear() rather than Empty().

Given how rare users of NS_DECL_CYCLE_COLLECTION_CLASS_NO_UNLINK are
likely to be, it's probably not worth putting the empty Root and Unroot
methods in a common base class (inheriting
nsXPCOMCycleCollectionParticipant), but it might be worth leaving a
comment with the idea in case it's used more in the future.

Should ClearGraph also set mRootCount to 0?  Given that it frees the
nodes, I think that's probably a good idea.

Seems like you could eliminate sCurrGraph and use sCollector->mGraph
instead (and replace the null-checks of sCurrGraph with a check of
mRootCount, maybe, or some other non-emptiness check?).

Maybe rename CollectWhite to RootAndUnlinkWhite or just UnlinkWhite?

Maybe assert...
+        if(!gDidCollection)
+        {
+            gInCollection = nsCycleCollector_beginCollection();
...right here...
+            gDidCollection = PR_TRUE;
+        }
...that gDidCollection is still false, to ensure that there wasn't
recursion into the same point?

And maybe assert...
+        if(gInCollection)
+        {
... right here ...
+            gCollected = nsCycleCollector_finishCollection();
+            gInCollection = PR_FALSE;
+        }
that gCollected is false, for the same reason.


CollectWhite starts with an mBuf.Empty().  You need to make sure
BeginCollection does this mBuf.Empty() even if CollectWhite isn't
called (in the case where it returns true), since FinishCollection will
need to know that that buf was cleared.  Probably the best way to do
this is to move the clearing of mBuf into BeginCollection.

r+sr=dbaron with that (unless you really wanted both jst and me to
review)

Sorry for the really long delay here.

Attachment #290589 - Flags: superreview?(jst)

Attachment #290589 - Flags: superreview+

Attachment #290589 - Flags: review?(dbaron)

Attachment #290589 - Flags: review+

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

Comment 25

•

17 years ago

One other thing -- what prevents the finishCollection call from recurring into cycle collection?  It seems like that could be a problem since there's only a single |mBuf|.  (It might be better to make it be ok rather than preventing it from happening.)

Assignee

Comment 26

•

17 years ago

(In reply to comment #24)
> CollectWhite starts with an mBuf.Empty().  You need to make sure
> BeginCollection does this mBuf.Empty() even if CollectWhite isn't
> called (in the case where it returns true), since FinishCollection will
> need to know that that buf was cleared.  Probably the best way to do
> this is to move the clearing of mBuf into BeginCollection.

I think this is fine as is, if BeginCollection doesn't call CollectWhite then mBuf.GetSize() == 0.

Took care of all the other comments, still need to look at finishCollection recursion.

Assignee

Comment 27

•

17 years ago

I'm going to disable recursion through finishCollection, I don't think we should try to collect again while we haven't yet released all the white nodes from the previous collection.

Assignee

Comment 28

•

17 years ago

Attached patch v1.4 — Details — Splinter Review

With dbaron's comments taken care of.

Attachment #290589 - Attachment is obsolete: true

Attachment #296316 - Flags: superreview+

Attachment #296316 - Flags: review+

Igor Bukanov

Updated

•

17 years ago

Blocks: 411531

Assignee

Updated

•

17 years ago

No longer blocks: 411531

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

Assignee

Updated

•

17 years ago

Blocks: 407034

Status: ASSIGNED → RESOLVED

Closed: 17 years ago

Resolution: --- → FIXED

Target Milestone: --- → mozilla1.9 M11

Comment 29

•

17 years ago

Attached patch fix DEBUG_CC bustage — Details — Splinter Review

This trivial patch fixes DEBUG_CC compilation errors from the previous patch.  I'll check it in shortly.

Comment 30

•

17 years ago

Attached file Still crashing here (obsolete) — Details

Comment 31

•

17 years ago

Attached file crashing here (obsolete) — Details

This is the stack that I got for the steps on comment #19.

Comment 32

•

17 years ago

Attached file and here... (obsolete) — Details

On self built trunk, with clean tree, updated 23:00h GMT.

Comment 33

•

17 years ago

(In reply to comment #31)
> Created an attachment (id=296657) [details]
> crashing here
> 
> This is the stack that I got for the steps on comment #19.

I got this one while closing a tab outside CZ too.

And sorry for the incomplete stack on the last attachment =/.

Assignee

Comment 34

•

17 years ago

Nope, all of those stacks show bug 407034.

Comment 35

•

17 years ago

Oh, ok.

Updated

•

17 years ago

Attachment #296655 - Attachment is obsolete: true

Updated

•

17 years ago

Attachment #296657 - Attachment is obsolete: true

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

Updated

•

17 years ago

Attachment #296659 - Attachment is obsolete: true

Comment 36

•

16 years ago

Attached patch fix DEBUG_CC bustage on Windows — Details — Splinter Review

Attachment #307617 - Flags: superreview?(peterv)

Attachment #307617 - Flags: review?(peterv)