Remove nsPermissionManager's GetSingleton

RESOLVED FIXED in mozilla9

Status

()

Core
Networking: Cookies
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: stechz, Assigned: stechz)

Tracking

Trunk
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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)

Updated

6 years ago
Assignee: nobody → ben
(Assignee)

Comment 1

6 years ago
Created attachment 548847 [details] [diff] [review]
nsPermissionManager remove AddRef once hack

Dan, are you up for a review? :)
Attachment #548847 - Flags: review?(dwitte)

Comment 2

6 years ago
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?
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Comment 4

6 years ago
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

6 years ago
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?
(Assignee)

Comment 6

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

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

6 years ago
(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?)
(Assignee)

Comment 9

6 years ago
Looks like the first thing was this!
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#428

Comment 10

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 674651
(Assignee)

Comment 11

6 years ago
> 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.
(Assignee)

Comment 12

6 years ago
If we expect the permissions service to be around, I hope we can lazily initialize the DB connection.
(Assignee)

Comment 13

6 years ago
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.
(Assignee)

Comment 15

6 years ago
How about we remove GetSingleton and GetXPCOMSingleton?

Comment 16

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

6 years ago
(I think)
(Assignee)

Comment 18

6 years ago
Created attachment 548898 [details] [diff] [review]
nsPermissionManager remove AddRef once hack
Attachment #548898 - Flags: review?(dwitte)
(Assignee)

Updated

6 years ago
Attachment #548847 - Attachment is obsolete: true
Attachment #548847 - Flags: review?(dwitte)

Comment 19

6 years ago
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

6 years ago
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+
(Assignee)

Comment 21

6 years ago
Created attachment 548906 [details] [diff] [review]
Remove nsPermissionManager's GetSingleton
(Assignee)

Updated

6 years ago
Attachment #548898 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #548906 - Flags: review+
Attachment #548906 - Flags: checkin+
(Assignee)

Updated

6 years ago
Summary: nsPermissionManager remove AddRef once hack → Remove nsPermissionManager's GetSingleton

Comment 22

6 years ago
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
(Assignee)

Updated

6 years ago
Attachment #548906 - Attachment description: Remove nsPermissionManager's GetSingleton -a --post-to-bugzilla Bug 674615 → Remove nsPermissionManager's GetSingleton

Comment 23

6 years ago
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
(Assignee)

Comment 24

6 years ago
Finally pushed to inbound :) http://hg.mozilla.org/integration/mozilla-inbound/rev/21a91ca86227
http://hg.mozilla.org/mozilla-central/rev/21a91ca86227
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.