Closed Bug 674615 Opened 9 years ago Closed 9 years ago

Remove nsPermissionManager's GetSingleton

Categories

(Core :: Networking: Cookies, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: stechz, Assigned: stechz)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → ben
Dan, are you up for a review? :)
Attachment #548847 - Flags: review?(dwitte)
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?
(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.
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.
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?
(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?
(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.
(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?)
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.
Blocks: 674651
> 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.
If we expect the permissions service to be around, I hope we can lazily initialize the DB connection.
Oh, I think I understand now. Basically we can fix this bug by ensuring that ContentParent.cpp uses the service manager to get nsPermissionManager?
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.
How about we remove GetSingleton and GetXPCOMSingleton?
(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...)
(I think)
Attachment #548898 - Flags: review?(dwitte)
Attachment #548847 - Attachment is obsolete: true
Attachment #548847 - Flags: review?(dwitte)
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 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.
Attachment #548898 - Flags: review?(dwitte) → review+
Attachment #548898 - Attachment is obsolete: true
Attachment #548906 - Flags: review+
Attachment #548906 - Flags: checkin+
Summary: nsPermissionManager remove AddRef once hack → Remove nsPermissionManager's GetSingleton
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
Attachment #548906 - Attachment description: Remove nsPermissionManager's GetSingleton -a --post-to-bugzilla Bug 674615 → Remove nsPermissionManager's GetSingleton
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
http://hg.mozilla.org/mozilla-central/rev/21a91ca86227
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.