Break a potential cycle with XPConnect and mozStorageService

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Storage
P2
normal
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

({fixed1.9.1, mlk})

Trunk
mozilla1.9.2a1
fixed1.9.1, mlk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

9.79 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
Created attachment 357257 [details] [diff] [review]
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!
Attachment #357257 - Flags: review?(bugmail)
Comment on attachment 357257 [details] [diff] [review]
v1.0

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

Nit: missing space.
Flags: blocking1.9.1?

Updated

8 years ago
Blocks: 465952

Updated

8 years ago
Keywords: mlk
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
(Assignee)

Updated

8 years ago
Attachment #357257 - Flags: review?(bugmail) → review?(bent.mozilla)
(Assignee)

Updated

8 years ago
Target Milestone: --- → mozilla1.9.2a1
Version: 1.9.1 Branch → Trunk
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.
Attachment #357257 - Flags: review?(bent.mozilla) → review-
Blocks: 473802
Drop the leak threshold to 0 as well if the follow up patch works as well as
the existing one does.
Created attachment 357969 [details] [diff] [review]
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.
Attachment #357257 - Attachment is obsolete: true
Attachment #357969 - Flags: review?(sdwilsh)
(Assignee)

Comment 5

8 years ago
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.
Attachment #357969 - Flags: review?(sdwilsh) → review-
(Assignee)

Comment 6

8 years ago
Created attachment 357988 [details] [diff] [review]
v3.0
Attachment #357969 - Attachment is obsolete: true
Attachment #357988 - Flags: review?(bent.mozilla)
(Assignee)

Comment 7

8 years ago
And leaks are at zero locally.
(Assignee)

Updated

8 years ago
Whiteboard: [has patch][needs review bent]
Oh, drat, I wasn't CC'd.
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.
Oh, and 'chache' is not spelled correctly in that comment i pasted above :)
(Assignee)

Comment 11

8 years ago
Created attachment 358020 [details] [diff] [review]
v3.1
Attachment #358020 - Flags: review?(bent.mozilla)
(Assignee)

Updated

8 years ago
Attachment #357988 - Attachment is obsolete: true
Attachment #357988 - Flags: review?(bent.mozilla)
Comment on attachment 358020 [details] [diff] [review]
v3.1

r=me.
Attachment #358020 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 13

8 years ago
http://hg.mozilla.org/mozilla-central/rev/44fd2796cee1
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review bent]
(Assignee)

Comment 14

8 years ago
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
Keywords: fixed1.9.1
V.Fixed, per bug 465952.
Status: RESOLVED → VERIFIED
I had to commit http://hg.mozilla.org/mozilla-central/rev/64ff884c9127 to get my build to start properly.
(Assignee)

Comment 17

8 years ago
(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

8 years ago
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?
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b1ec9aa4489f
Blocks: 475085
You need to log in before you can comment on or make changes to this bug.