Account Manager should protect itself in case Shutdown gets called twice

VERIFIED FIXED in mozilla1.0.2

Status

P1
major
VERIFIED FIXED
16 years ago
14 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

Trunk
mozilla1.0.2

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2] [ETA 10/21])

Attachments

(1 attachment)

(Assignee)

Description

16 years ago
Currently nsMsgAccountManager::Observe listens to both
NS_XPCOM_SHUTDOWN_OBSERVER_ID and "profile-before-change". That is good, and it
should stay that way.

Right now, at application exit, the profile-before-change event will not be seen.

However, in bug 175320, we intend to make a change, that will cause both events
to be seen during shutdown.

Right now, that would cause the nsMsgAccountManager to try to shutdown twice.
That would be bad.


I suggest that we add a protection mechanism to nsMsgAccountManager.
When either of the above events is seen, it should try to Shutdown.
However, it should detect whether it is already in the shut down state, and not
try to shutdown again.
(Assignee)

Updated

16 years ago
Blocks: 175320
(Assignee)

Comment 1

16 years ago
Nominating this as topembed, since it blocks bug 175320.
Keywords: topembed
(Assignee)

Comment 2

16 years ago
Created attachment 103360 [details] [diff] [review]
Patch v1

I suggest this patch to fix the problem.

The code does already set a flag to prevent additional shutdown in the
destructor. I'm adding a check for the same flag to the Shutdown method.
(Assignee)

Comment 3

16 years ago
Bhuvan, can you please review?

Updated

16 years ago
Attachment #103360 - Flags: review+

Comment 4

16 years ago
Comment on attachment 103360 [details] [diff] [review]
Patch v1

Thanks for fixing this.

r=bhuvan
(Assignee)

Comment 5

16 years ago
-> me
Assignee: racham → kaie

Comment 6

16 years ago
Marking as topembed+/nsbeta1+ as this will be needed by a major embedding customer.
Severity: normal → major
Keywords: topembed → nsbeta1+, topembed+
Priority: -- → P1
Whiteboard: [adt2] [ETA Needed]
Target Milestone: --- → mozilla1.0.2

Comment 7

16 years ago
Comment on attachment 103360 [details] [diff] [review]
Patch v1

sr=mscott
Attachment #103360 - Flags: superreview+
(Assignee)

Updated

16 years ago
Keywords: adt1.0.2, mozilla1.0.2

Updated

16 years ago
Keywords: approval
Whiteboard: [adt2] [ETA Needed] → [adt2] [ETA 10/21]

Comment 8

16 years ago
Comment on attachment 103360 [details] [diff] [review]
Patch v1

a=asa for checkin to 1.2 (on behalf of drivers).
Attachment #103360 - Flags: approval+
(Assignee)

Comment 9

16 years ago
fixed on trunk
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 10

16 years ago
nbaca: can you pls verify this as fixed on the trunk? Thanks!

Comment 11

16 years ago
Blackbird will consider this when veified on trunk.  Need to know risk and
whether or not there is any ui change.
adt+ for branch, please seek drivers approval before landing
Keywords: adt1.0.2 → adt1.0.2+

Comment 13

16 years ago
nbaca: can you pls verify this as fixed on the trunk? Thanks!

Comment 14

16 years ago
If no progress by 11/5 will consider dropping from release

Comment 15

16 years ago
This is a 'code verified' bug as far as verification concerned. 

Ninoschka, please mark this one verified (add keyword 'code verified' in your
comments for records).

Bhuvan

Comment 16

16 years ago
Verified on the trunk per Bhuvan's comments.
Status: RESOLVED → VERIFIED
Keywords: topembed+
(Assignee)

Comment 17

16 years ago
> If no progress by 11/5 will consider dropping from release

This patch is only needed because it blocks 175320.
However, the request to land 175320 on the trunk was rejected, so we don't have
a lot of testing yet.

But it turned out, fixing bug 175320 will not have the desired effect, unless we
also fix its dependent bug 177260. And the patch for bug 177260 is risky and not
yet reviewed.

As a result, it's probably reasonable to drop this patch from the branch radar,
and get it back once we have completed bug 177260.

Comment 18

16 years ago
Discussed in bBird team mtg.  Agreed to minus per comment 17.
Keywords: adt1.0.2+ → adt1.0.2-
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.