Closed Bug 280234 Opened 20 years ago Closed 19 years ago

[FIX]sometimes get ###!!! ASSERTION: tearoff not empty in dtor: '!(GetInterface()||GetNative()||GetJSObject())'


(Core :: XPConnect, defect, P1)






(Reporter: mvl, Assigned: benjamin)




(2 files, 2 obsolete files)

when using my sharedics provider, which reloads an ics file every 10 seconds, I
sometimes get
###!!! ASSERTION: tearoff not empty in dtor:

It usually shows only on the first load, while the window is drawing.
line 145 is "var newItem = aItem.clone();"
I get this assertion now when i have 2 calendars defined, and the
calanderManager tries to create them on startup.
This one is a little less hard to reproduce, so i pokes it a little.

First, the tearoff that asserts is about a calICalendar.
defines a nsCOMPtr. This gets filled in
From what i understand, with the tearoff the assertion is about.
Now when the nsCOMPtr goes out of scope, is decreases the refcount. It turns out
the be zore now, so it gets destroyed. But it isn't ready to, so it asserts.

What i don't get is why this tearoff is used, when it seems to have a zero
refcount. Why isn't it already gone? Or shouldn't it be zero to begin with,
because it still isn't cleaned up?
For fun, i tried to make the nscomptr a normal pointer. that did make the
assertion go away, but the problems later on (errors about 'calendar' being not
defined) didn't go away.
Stepping with gdb showed me that the refcnt is going from 2 to 1 after
(and not block closes. After that if but before } it is also 1. it should be 2)

Commenting out the if block made the assertion go away. But jsut removing it
would be wrong, i guess.

Some more testing showed that commenting out
also made the asserion go away. Now i don't know why.

Blocks: 293461
I poked some more, and found this:
does a QueryInterface on the object (which is implemented in javascript). This
means that a javascript call is made, so somewhere a call to is
made. There is a magic number 20 in there. I think that before the object is
created, mNumEvaluations is such that the QI during the creation makes is >20,
and GC is called. This GC makes the half-created object go away to soon, and
things go wrong.
The magic 20 explains the randomness of this: if the code is changed a little
bit, or the amount of data is different, mNumEvaluations is different, and GC
won't be called.
Attached patch Possible patch (obsolete) — Splinter Review
mvl, can you reproduce this reliably?  If so, does this patch help?
Comment on attachment 184526 [details] [diff] [review]
Possible patch

No, this doesn't work.
Attachment #184526 - Attachment is obsolete: true
The idea here is that an auto-marking ptr should mark the whole
XPCWrappedNative, including the flat JSObject, which we'd normally allow to
just die as needed.  Otherwise, if we GC while we're in the middle of creating
the wrapper, when the JSObject is only reachable from the wrapper, we lose.

The second change is to auto-mark tearoffs also, since otherwise they could go
away when GC runs (since they are in the middle of being created and hence
aren't on the callstack they won't get marked, and then they will be swept by
XPCWrappedNativeScope::SweepAllWrappedNativeTearOffs in the GC callback.
Attachment #184535 - Flags: second-review?(brendan)
Attachment #184535 - Flags: first-review?(dbradley)
> +        if (mFlatJSObject) {
> +            ::JS_MarkGCThing(cx, mFlatJSObject,
> +                             "XPCWrappedNative::mFlatJSObject", nsnull);
> +        }

That test should be:

    if (mFlatJSObject && mFlatJSObject != (JSObject*)JSVAL_ONE) {

since we can GC before we've created our flat JSObject...

Comment on attachment 184535 [details] [diff] [review]
This should do it

I'm torn. The one thing that I don't like is that this adds AutoMark to all the
various classes needed for AutoMark and only one is really using it. Another
option would be to create another marking class for this purpose. That ofcourse
adds another class to the mix. On the up side it would be a bit more efficient
since the existing paths wouldn't have to call AutoMark needlessly.

As far as the current patch do we need to test mPtr a second time in the
AutoMarkingPtr class's MarkBeforeJSFinalize? Would mPtr go away during the
MarkBeforeJSFinalize call? I thought most of the implementations were just bit
We could definitely combine the mPtr tests into one test; good catch there.

I'd assume that most AutoMark calls for the other classes are completely
optimized away, since those are inline methods.  So this is really adding lots
of AutoMark calls for XPCWrappedNative.  And we do need those, I think.
Right, but XPCWrappedNative is the only one that needs this and we're just 
adding clutter to the other classes that play no part in this, just for the 
fact that we're trying to add this on the existng auto marking structure.

It just seems like we're trying to do something very specific here, but bolt 
it into a rather general facility for the sake of convenience.

Also I'll do the jband nit mantra about spaces between keywords and the left 
paren such as "if (" should be "if("
Right.  I could create a new auto-marking class if you prefer; it doesn't really
matter that much to me.  Just seemed to make sense to reuse existing code...  

I guess I could make the "real" auto-marker for XPCWrappedNative inherit from
the current auto-marker (and change classnames appropriately).  Would you prefer

Will fix the spacing nit.
Comment on attachment 184535 [details] [diff] [review]
This should do it


The alternatives I can think of at the moment have drawbacks as well.
Derivation just adds the opportunity for someone to easily pick the wrong

Probably should look at moving this to a template . Maybe at that point
something more elegant will present itself.

So go for it with the couple of nits addressed and Brendan's blessing
Attachment #184535 - Flags: first-review?(dbradley) → first-review+
Blocks: 295220
Happily, this patch seems to fix a separate problem I had where JS interface
reference was being incorrectly GCed while still live.
Component: Base → XPConnect
Flags: second-review?(brendan)
Flags: first-review+
OS: Linux → All
Product: Calendar → Core
Hardware: PC → All
Version: unspecified → Trunk
Comment on attachment 184535 [details] [diff] [review]
This should do it

Didn't realize that switching components was gonna mess with the review flags. 
Putting them back.
Attachment #184535 - Flags: superreview?(brendan)
Attachment #184535 - Flags: review+
Assignee: shaver → bzbarsky
Priority: -- → P1
Summary: sometimes get ###!!! ASSERTION: tearoff not empty in dtor: '!(GetInterface()||GetNative()||GetJSObject())' → [FIX]sometimes get ###!!! ASSERTION: tearoff not empty in dtor: '!(GetInterface()||GetNative()||GetJSObject())'
Target Milestone: --- → mozilla1.8beta3
Attached patch Updated to dbradley's comments (obsolete) — Splinter Review
Attachment #184535 - Attachment is obsolete: true
Attachment #184719 - Flags: superreview?(brendan)
Attachment #184535 - Attachment is obsolete: false
Attachment #184535 - Flags: superreview?(brendan)
Flags: blocking1.8b3?
Flags: blocking1.8b3? → blocking1.8b3+
Comment on attachment 184719 [details] [diff] [review]
Updated to dbradley's comments

Hey, this was a bug I was gonna fix some day -- thanks for actually fixing it. 
Nit: a blank line before the multi-line comments such as "// During shutdown,
..." would be easier on the eyes.

sr+a=me, noting dbradley's r=.

Attachment #184719 - Flags: superreview?(brendan)
Attachment #184719 - Flags: superreview+
Attachment #184719 - Flags: review+
Attachment #184719 - Flags: approval1.8b3+
can someone check this in for boris so it can make the alpha2 train?
->me for checkin
Assignee: bzbarsky → benjamin
Fixed on trunk for 1.8b3.
Closed: 19 years ago
Resolution: --- → FIXED
I can't find this checkin in bonsai or in lxr. Something went wrong, or is
bonsai just very, very slow?
Comment on attachment 184719 [details] [diff] [review]
Updated to dbradley's comments

mozilla/js/src/xpconnect/src/xpcwrappednative.cpp	1.96
mozilla/js/src/xpconnect/src/xpcprivate.h	1.159
Attachment #184719 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.