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

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

when using my sharedics provider, which reloads an ics file every 10 seconds, I
sometimes get
###!!! ASSERTION: tearoff not empty in dtor:
'!(GetInterface()||GetNative()||GetJSObject())'

It usually shows only on the first load, while the window is drawing.

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: gdb stack + DumpJSStack output

line 145 is "var newItem = aItem.clone();"

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

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.

http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcconvert.cpp#461
defines a nsCOMPtr. This gets filled in
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcconvert.cpp#1004
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?

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

Stepping with gdb showed me that the refcnt is going from 2 to 1 after
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappednative.cpp#1472
(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
http://lxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#2481
also made the asserion go away. Now i don't know why.

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

I poked some more, and found this:
http://lxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#2539
does a QueryInterface on the object (which is implemented in javascript). This
means that a javascript call is made, so somewhere a call to
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsJSEnvironment.cpp#1895 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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible patch

mvl, can you reproduce this reliably?  If so, does this patch help?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 184526 [details] [diff] [review]
Possible patch

No, this doesn't work.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: This should do it

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> +        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 10

[David Bradley]

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
twiddling.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[David Bradley]

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("

Comment 13

[Boris Zbarsky [:bzbarsky]]

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
that?

Will fix the spacing nit.

Comment 14

[David Bradley]

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

r=dbradley

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
class.

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

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Happily, this patch seems to fix a separate problem I had where JS interface
reference was being incorrectly GCed while still live.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to dbradley's comments

Comment 18

[Brendan Eich [:brendan]]

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=.

/be

Comment 19

[chris hofmann]

can someone check this in for boris so it can make the alpha2 train?

Comment 20

[Benjamin Smedberg]

->me for checkin

Comment 21

[Benjamin Smedberg]

Fixed on trunk for 1.8b3.

Comment 22

[Michiel van Leeuwen (email: mvl+moz@)]

I can't find this checkin in bonsai or in lxr. Something went wrong, or is
bonsai just very, very slow?

Comment 23

[timeless]

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