300853 - Caps crash on cleanup [@ DomainPolicy::Drop][@ 0x7a6f6d5c]

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Giorgio Maone [:ma1]]

Previously masked by bug 217967, whose crash happened early, rather than being
deferred to cleanup, and likely caused by residual "old" code still not properly
handling the home-made DomainPolicy reference counting (e.g.
nsScriptSecurityManager destructor, brutally deleting mDefaultPolicy instead of
dropping it).
Now I'm going to sleep (2:20 A.M. in my timezone), and tomorrow I'll attach a
coffee smelling patch ;-)

Comment 1

[Giorgio Maone [:ma1]]

Attachment: More consistent DomainPolicy lifecycle management [Checked in: Comment 8]

Sorry to be so late, but I've no way been able to reproduce the crash in a
debug session.
Hence, I'm trying to remove the hypotetical cause (premature deletion dued to
non-generalized use of reference count), crossing my fingers and hoping that
this patch estinguishes crash reports as soon as it gets landed :)

1) Removed the most prominent illegal direct delete (mDefaultPolicy in
nsScriptSecurityManager detructor) - not sure it is the cause, though, because
as far as I can see nsScriptSecurityManager deletion happens (or should happen)
after all nsPrincipal objects have been already deleted.
2) Added mDefaultPolicy null assignment to avoid double dropping under
exceptional circumnstances (e.g. out of memory errors during InitPolicies() )
3) Added an assertion to track deletions happening with an illegal reference
count
4) Incidentally, edited contributors list (also in nsPrincipal) to reflect bug
217967 changes and save caillon from getting blamed for my sins ;)

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 189625 [details] [diff] [review]
More  consistent DomainPolicy lifecycle management
[Checked in: Comment 8]

r=caillon, but its probably worth a look to see what else needs similar
patching.

Comment 3

[Giorgio Maone [:ma1]]

(In reply to comment #2)
> its probably worth a look to see what else needs similar
> patching.
 
That's why I added this line:

NS_ASSERTION(mRefCount == 0, "Wrong refcount in DomainPolicy dtor");
 
I've been browsing since yesterday watching this assertion, and nothing violated
it so far...

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 189625 [details] [diff] [review]
More  consistent DomainPolicy lifecycle management
[Checked in: Comment 8]

sr=dveditz

Comment 5

[Serge Gautherie (:sgautherie)]

+(K) Regression:
In v1.7.x, I've seen/crashed it for the first time after installing v1.7.10
(assumed fine up to v1.7.8);
In MAS-SM Trunk, I had seen it 2-3 times in +/- recent nightlies ("never" before).

Comment 6

[Frank Wein [:mcsmurf]]

*** Bug 301898 has been marked as a duplicate of this bug. ***

Comment 7

[Giorgio Maone [:ma1]]

Comment on attachment 189625 [details] [diff] [review]
More  consistent DomainPolicy lifecycle management
[Checked in: Comment 8]

I'm going to attach a branch backport in minutes

Comment 8

[Giorgio Maone [:ma1]]

Attachment: Trivial backport to aviary1.0.1 branch

My patch in attachment #189625 [details] [diff] [review] has been landed by timeless on trunk (2005-07-19
14:55). 
Trunk builds have disappeared from talkback records for this bug since then, so
I assume the bug is fixed.
This is a trivial backport of attachment #189625 [details] [diff] [review] for Aviary1.0.1 branch.
Should I close this bug as FIXED just now, or rather wait for this patch to be
landed on branch?

Comment 9

[Giorgio Maone [:ma1]]

Marking FIXED as per timeless kind explaination :)
Yet to be fixed in 1.7 branches.

Comment 10

[timeless]

this is a guess. jay/mao: correct me if i'm wrong

Comment 11

[timeless]

*** Bug 302166 has been marked as a duplicate of this bug. ***

Comment 12

[Asa Dotzler [:asa]]

1.7.11 is not blocked by this.

Comment 13

[Petr Soucek]

(In reply to comment #12)
> 1.7.11 is not blocked by this.

Why do you think that 1.7.11 is not blocked by this?
My opinion is that even 1.7.10 should not have been released with this annoying
crash on exit, this was the most frequent crash reported by talkback.

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 190309 [details] [diff] [review]
Trivial backport to aviary1.0.1 branch

(In reply to comment #12)
> 1.7.11 is not blocked by this.

Unexpected since the fix is at hand, but well ... let's have v1.7.12 yet :-/

Comment 15

[Frank Wein [:mcsmurf]]

*** Bug 304320 has been marked as a duplicate of this bug. ***

Comment 16

[Jay Patel [:jay]]

Adding topcrash+ keyword since this is a major regression since 1.7.10.  We
really should make sure this makes it into 1.7.12, as it makes up close to 40%
of all crashes for 1.7.11:
http://talkback-public.mozilla.org/reports/mozilla/M1711/M1711-topcrashers.html

Sorry this didn't make it into 1.7.11, it should have, and would have if I was
on top of this during that short release cycle.

Comment 17

[Mats Palmgren (inactive)]

*** Bug 306986 has been marked as a duplicate of this bug. ***

Comment 18

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

attachment 190309 [details] [diff] [review] checked in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_2005124_BRANCH.

Comment 19

[Mike Schroepfer]

Can we get a final verification?