175888 - Calling nsGlobalHistory::CloseDB() must be safe

Status: VERIFIED FIXED

(Core Graveyard :: History: Global, defect, P1)

Description

[Kai Engert (:KaiE:)]

Because of bug 175320, in the future the profile shutdown events will be seen on
application shutdown. We must make sure that nsGlobalService does not run into
trouble when because of those events, CloseDB will get called twice.

I tried to guess whether calling CloseDB() twice is already safe, but I'm having
trouble making the decision.

The following code is inside CloseDB:
  ExpireEntries(PR_FALSE /* don't notify */);
  err = Commit(kSessionCommit);

  // order is important here - logically smallest objects first
  mMetaRow = nsnull;
  
  if (mTable)
    mTable->Release();

  if (mStore)
    mStore->Release();

  if (mEnv)
    mEnv->Release();

  mTable = nsnull; mEnv = nsnull; mStore = nsnull;

Is it ok to execute that twice?

Could we possibly check whether mStore is null, and prevent cleanup in that
case? I suggest, because OpenDB is returning success if mStore is already set.
On the other, CloseDB seems to treat mStore just like one of many objects to
clean up.

Comment 1

[Kai Engert (:KaiE:)]

Alec, Blake, do you know the answer?

Comment 2

[Alec Flett]

hmm.. I would imagine its safe. if mStore is null, then ExpireEntries and Commit
should be no-ops.

Comment 3

[Jaime Rodriguez, Jr.]

Bug 175320 topembed+/nsbeta1+, and is blocked by this bug, so I am marking this
one as nsbeta1+/topembed+.

Comment 4

[Kai Engert (:KaiE:)]

Oops. I notice that ExpireEntries is not a no-op.

Actually, it will call OpenDB!

CloseDB() -> ExpireEntries() -> RemoveMatchingRows() -> OpenDB()

Comment 5

[Alec Flett]

oh! that's bad. I guess we need the check at the start of CloseDB...well, I'll
review any patches :) 

Comment 6

[Kai Engert (:KaiE:)]

Attachment: Patch v1

I don't really dare a patch that makes assumptions about the existing code, and
I don't want to use any existing state variable to make a decision.

I'm attaching a patch that tries to be as safe as possible.
The idea of the patch is to avoid executing the code in CloseDB multiple times,
if no calls to OpenDB were seen in between.
However, as soon as a call to OpenDB was seen, we allow the call to CloseDB, as
parts of its action might have become necessary.

I'm choosing this approach, because I don't know what could happen between the
profile-before-change notification and the actual destruction.

Comment 7

[Kai Engert (:KaiE:)]

-> me, since it looks I'm writing the patch :)

Comment 8

[Alec Flett]

Comment on attachment 103670 [details] [diff] [review]
Patch v1

Kai - that is why we have the reviewer system - to catch assumptions like that.
This allows you to attempt to write simpler code (i.e. instead of what is
written here) and ask your reviewers if you are making too many assumptions.

As a reviewer and coauthor of this code, I can vouch for the fact that mStore
indicates that the store is open. This code is in fact broken, because nothing
ever resets mAvoidMultipleCloseDB, so that once the DB is closed, it will never
be closed again. (for example, if you switch profiles once, all future profile
switches will leave the DB open)

Please just check mStore and return early if mStore is null.

Comment 9

[Kai Engert (:KaiE:)]

> nothing ever resets mAvoidMultipleCloseDB, so that once the DB is closed, 
> it will never be closed again. (for example, if you switch profiles once, 
> all future profile switches will leave the DB open)

? The reset is happening at the beginning of OpenDB. The avoid flag will get set
to false, the check in CloseDB will fail and the rest of the close function will
get executed.

But I'll attach the simpler patch.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Simpler patch as suggested

Comment 11

[Alec Flett]

Comment on attachment 103728 [details] [diff] [review]
Patch v2

looks great. sr=alecf

Comment 12

[Kai Engert (:KaiE:)]

Chris, Blake, could one of you please review this small patch?

Comment 13

[Jaime Rodriguez, Jr.]

i think the checkin of this patch can wait until someone urgently needs the
changes to land on the 1.0 branch. setting ETA to 11/15 for now.

Comment 14

[Kai Engert (:KaiE:)]

I need help finding additional reviewers. I pinged both waterson and blaker by
private mail 2 weeks ago, but they did not respond.

Comment 15

[Alec Flett]

as a former owner of history, I hope that my sr= could help count towards a
module-owner review. this is pretty basic stuff, so I'd like to say that any
reviewer should do..

(that said, waterson is off on other projects, and is probably a bad candidate
for reviews)

Comment 16

[Kai Engert (:KaiE:)]

fixed on trunk

Comment 17

[sairuh (rarely reading bugmail)]

rs vrfy.