Last Comment Bug 674615 - Remove nsPermissionManager's GetSingleton
: Remove nsPermissionManager's GetSingleton
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cookies (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Benjamin Stover (:stechz)
:
:
Mentors:
Depends on:
Blocks: 674651
  Show dependency treegraph
 
Reported: 2011-07-27 11:39 PDT by Benjamin Stover (:stechz)
Modified: 2011-08-31 02:03 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
nsPermissionManager remove AddRef once hack (3.00 KB, patch)
2011-07-27 11:42 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Splinter Review
nsPermissionManager remove AddRef once hack (4.53 KB, patch)
2011-07-27 13:39 PDT, Benjamin Stover (:stechz)
dwitte: review+
Details | Diff | Splinter Review
Remove nsPermissionManager's GetSingleton (4.50 KB, patch)
2011-07-27 13:56 PDT, Benjamin Stover (:stechz)
ben: review+
ben: checkin+
Details | Diff | Splinter Review

Description Benjamin Stover (:stechz) 2011-07-27 11:39:58 PDT
In my local build, I've changed some JS related to Fennec's startup and I noticed I am now crashing because nsPermissionManager is making some assumptions about module load order.

Instead, we can listen for xpcom-shutdown.
Comment 1 Benjamin Stover (:stechz) 2011-07-27 11:42:03 PDT
Created attachment 548847 [details] [diff] [review]
nsPermissionManager remove AddRef once hack

Dan, are you up for a review? :)
Comment 2 dwitte@gmail.com 2011-07-27 11:55:37 PDT
Where are you crashing? And what's changed about service teardown ordering?

It seems to me that as soon as the service manager releases the permission manager, and other services do the same in their respective destructors, that it would be destroyed. So I don't understand why this situation would stop working. It seems pretty safe to me?
Comment 3 Benjamin Stover (:stechz) 2011-07-27 12:03:00 PDT
(In reply to comment #2)
> Where are you crashing? And what's changed about service teardown ordering?

I'm assuming some component got initialized in a different order, just due to a change of order in frontend GetService calls.

> It seems to me that as soon as the service manager releases the permission
> manager, and other services do the same in their respective destructors,
> that it would be destroyed. So I don't understand why this situation would
> stop working. It seems pretty safe to me?

See the comment I removed in the patch and the added NS_ADDREF and https://bugzilla.mozilla.org/show_bug.cgi?id=209571. Essentially we aren't holding on to gPermissionsManager, hoping that whatever first initialized us will last longer than we do.
Comment 4 Benjamin Stover (:stechz) 2011-07-27 12:18:01 PDT
Sorry, your other question was "Where are you crashing?" Here:
http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#194

Because the permission manager has been deleted, so we have a dangling pointer.
Comment 5 dwitte@gmail.com 2011-07-27 12:24:21 PDT
Yeah, I read the patch, and I wrote that code so I'm aware of the bug it fixed :)

So that crash means someone's calling GetSingleton during shutdown. Which is illegal. Callstack?
Comment 6 Benjamin Stover (:stechz) 2011-07-27 12:28:02 PDT
(In reply to comment #5)
> Yeah, I read the patch, and I wrote that code so I'm aware of the bug it
> fixed :)

Well, it was 2003. :-)

> So that crash means someone's calling GetSingleton during shutdown. Which is
> illegal. Callstack?

The crash doesn't happen during shutdown. When I go to load a web page, it crashes. I think something short-lived instantiates it first and goes away, thus removing the one reference to the service and making gPermissionManager a dangling pointer.

My patch forgot to remove the reference to gPermissionManager somewhere in shutdown. I guess it should be in the destructor?
Comment 7 dwitte@gmail.com 2011-07-27 12:32:24 PDT
(In reply to comment #6)
> The crash doesn't happen during shutdown. When I go to load a web page, it
> crashes. I think something short-lived instantiates it first and goes away,
> thus removing the one reference to the service and making gPermissionManager
> a dangling pointer.

Oic. Who? :)


> My patch forgot to remove the reference to gPermissionManager somewhere in
> shutdown. I guess it should be in the destructor?

Yeah, it should.
Comment 8 dwitte@gmail.com 2011-07-27 12:33:38 PDT
(The service manager should really be the first thing to instantiate it, because the only way to do so should be via getService. So that means something xpcom-internal-ish is calling it and then dropping the reference. Why?)
Comment 9 Benjamin Stover (:stechz) 2011-07-27 12:40:20 PDT
Looks like the first thing was this!
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#428
Comment 10 dwitte@gmail.com 2011-07-27 12:45:55 PDT
Nice.

Next questions: 1) is it expected that this should be happening before the service manager wakes up; 2) if so, can this call be pushed out until after it has?

(I could answer those myself, but I haven't looked at this code in a while, and I'm not sure what's changed since.)

If the answer to 2 is no, then I'm fine with having xpcom-shutdown logic in the permission manager.
Comment 11 Benjamin Stover (:stechz) 2011-07-27 13:08:56 PDT
> Next questions: 1) is it expected that this should be happening before the
> service manager wakes up; 2) if so, can this call be pushed out until after
> it has?

So apparently I was wrong. This isn't because JS is initializing components any differently. It was my patch in bug 674651.

Without that patch in the other bug, for Fennec on my machine, the current "owner" of permission service seems to be nsContentBlocker:
https://bug674651.bugzilla.mozilla.org/attachment.cgi?id=548877

I have no idea if this is expected.
Comment 12 Benjamin Stover (:stechz) 2011-07-27 13:11:34 PDT
If we expect the permissions service to be around, I hope we can lazily initialize the DB connection.
Comment 13 Benjamin Stover (:stechz) 2011-07-27 13:15:30 PDT
Oh, I think I understand now. Basically we can fix this bug by ensuring that ContentParent.cpp uses the service manager to get nsPermissionManager?
Comment 14 Josh Matthews [:jdm] 2011-07-27 13:17:08 PDT
Precisely. Use the normal XPCOM method, then cast to the concrete type, since we're already making assumptions about the permission manager in the existing code.
Comment 15 Benjamin Stover (:stechz) 2011-07-27 13:18:55 PDT
How about we remove GetSingleton and GetXPCOMSingleton?
Comment 16 dwitte@gmail.com 2011-07-27 13:20:58 PDT
(In reply to comment #14)
> Precisely. Use the normal XPCOM method, then cast to the concrete type,
> since we're already making assumptions about the permission manager in the
> existing code.

Yeah, that'd work.

I'd like to see gPermissionManager nulled out in the dtor though, please do that :)

Don't remove GetXPCOMSingleton though, it makes me feel warm and fuzzy inside. (You could roll the concrete getter into it since we no longer need it...)
Comment 17 dwitte@gmail.com 2011-07-27 13:21:10 PDT
(I think)
Comment 18 Benjamin Stover (:stechz) 2011-07-27 13:39:36 PDT
Created attachment 548898 [details] [diff] [review]
nsPermissionManager remove AddRef once hack
Comment 19 Mozilla RelEng Bot 2011-07-27 13:40:20 PDT
Try run for 219e890cae40 is complete.
Detailed breakdown of the results available here:
    http://tbpl.mozilla.org/?tree=Try&rev=219e890cae40
Results:
    exception: 27
    success: 1
    warnings: 1
    failure: 10
Total buildrequests: 39
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bstover@mozilla.com-219e890cae40
Comment 20 dwitte@gmail.com 2011-07-27 13:52:14 PDT
Comment on attachment 548898 [details] [diff] [review]
nsPermissionManager remove AddRef once hack

>diff --git a/extensions/cookie/nsPermissionManager.cpp b/extensions/cookie/nsPermissionManager.cpp

> nsPermissionManager::~nsPermissionManager()
> {
>   RemoveAllFromMemory();
>+  NS_RELEASE(gPermissionManager);
>+  gPermissionManager = nsnull;

Whuh. Won't that crash? Also you're in the dtor so you don't need to release it. :)

r=dwitte with that.
Comment 21 Benjamin Stover (:stechz) 2011-07-27 13:56:37 PDT
Created attachment 548906 [details] [diff] [review]
Remove nsPermissionManager's GetSingleton
Comment 22 Mozilla RelEng Bot 2011-07-27 14:00:22 PDT
Try run for 4125fddfbfb1 is complete.
Detailed breakdown of the results available here:
    http://tbpl.mozilla.org/?tree=Try&rev=4125fddfbfb1
Results:
    exception: 17
Total buildrequests: 17
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bstover@mozilla.com-4125fddfbfb1
Comment 23 Mozilla RelEng Bot 2011-07-27 14:10:19 PDT
Try run for 17ac3456be6e is complete.
Detailed breakdown of the results available here:
    http://tbpl.mozilla.org/?tree=Try&rev=17ac3456be6e
Results:
    exception: 17
Total buildrequests: 17
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bstover@mozilla.com-17ac3456be6e
Comment 24 Benjamin Stover (:stechz) 2011-08-30 10:45:25 PDT
Finally pushed to inbound :) http://hg.mozilla.org/integration/mozilla-inbound/rev/21a91ca86227
Comment 25 Marco Bonardo [::mak] 2011-08-31 02:03:12 PDT
http://hg.mozilla.org/mozilla-central/rev/21a91ca86227

Note You need to log in before you can comment on or make changes to this bug.