473845 - Break a potential cycle with XPConnect and mozStorageService

Status: VERIFIED FIXED

(Core :: SQLite and Embedded Database Bindings, defect, P2)

Description

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

We seem to be creating cycles with XPConnect and mozStorageService.  We should just release the reference we hold at xpcom-shutdown in storage.

Locally, this makes browser chrome leaks go to zero.  yay!

Comment 1

[Serge Gautherie (:sgautherie)]

Comment on attachment 357257 [details] [diff] [review]
v1.0

>+                 "Did not properlyshut down mozStorageService!");

Nit: missing space.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 357257 [details] [diff] [review]
v1.0

As per our discussion let's just free the cached xpconnect on xpcom-shutdown and leave the sqlite shutdown order alone. Also fix the xpconnect() getter to handle this case so you can't leak xpconnect if one of your wrappers is still alive.

Comment 3

[Dave Townsend [:mossop]]

Drop the leak threshold to 0 as well if the follow up patch works as well as
the existing one does.

Comment 4

[Dave Townsend [:mossop]]

Attachment: simpler patch

Like so?

This clears the leaks from browser-chrome on my machine, haven't tested on other platforms though. Only one thought is whether Init really needs to return a failure if we fail to get and use the observer service, the only consequence is a shutdown leak. But I guess if that happens things are pretty screwed anyway.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 357969 [details] [diff] [review]
simpler patch

The problem with this patch is that it's perfectly legal for someone to try and use xpconnect during xpcom-shutdown.  bent and I discussed how to get around this yesterday though, so I'll post a new patch.

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v3.0

Comment 7

[Shawn Wilsher :sdwilsh]

And leaks are at zero locally.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, drat, I wasn't CC'd.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 357988 [details] [diff] [review]
v3.0

>+already_AddRefed<nsIXPConnect>
> mozStorageService::XPConnect()

I think you could make this much simpler by using nsCOMPtr's forget method (this case is what it was esigned for):

  nsCOMPtr<nsIXPConnect> retval;

  if (sXPConnect) {
    // normal
    retval = sXPConnect;
  }
  else {
    // shutdown
    retval = do_GetService(nsIXPConnect::GetCID());
  }

  return retval.forget();

>+    // We chache XPConnect for our language helpers.
>+    (void)CallGetService(nsIXPConnect::GetCID(), &sXPConnect);

You don't check the return here so if that call failed you'd crash on shutdown in Shutdown's NS_RELEASE...

I think you should reverse the order of the calls in Init to do the observer service stuff first, then cache xpconnect, and use NS_IF_RELEASE in shutdown. That way you're guaranteed not to leak or crash on shutdown. Mossop is right, though, that if any of that fails we're screwed anyway. Meh.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, and 'chache' is not spelled correctly in that comment i pasted above :)

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: v3.1

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 358020 [details] [diff] [review]
v3.1

r=me.

Comment 13

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/44fd2796cee1

Comment 14

[Shawn Wilsher :sdwilsh]

Leak threshold was different for 1.9.1, so I had to manually fix that, but no other changes were needed:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f7c35e9f75cb

Comment 15

[Serge Gautherie (:sgautherie)]

V.Fixed, per bug 465952.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

I had to commit http://hg.mozilla.org/mozilla-central/rev/64ff884c9127 to get my build to start properly.

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> Leak threshold was different for 1.9.1, so I had to manually fix that, but no
> other changes were needed:
But linux seems fine, so I suspect we are fine across the board with it.

Comment 18

[Brad Jackson]

My recent builds from 1.9.1 branch have no bookmarks in the menu or sidebar with f7c35e9f75cb applied. I have to back it out or apply 64ff884c9127 to restore bookmarks. Can we get this fix checked into the branch?

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b1ec9aa4489f