The default bug view has changed. See this FAQ.

about:memory crash in nsScriptSecurityManager::GetSubjectPrincipal when refreshed

RESOLVED FIXED in Firefox 16

Status

()

Core
XPConnect
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Grant, Assigned: njn)

Tracking

({crash, regression})

16 Branch
mozilla17
All
Windows 7
crash, regression
Points:
---

Firefox Tracking Flags

(firefox15 unaffected, firefox16+ fixed, firefox17 fixed)

Details

(Whiteboard: [qa-], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Hi all,

Every few days I get a crash with the latest nightlies, When I refresh about:memory it crashes Firefox completely: Here are some of the bug reports:

https://crash-stats.mozilla.com/report/index/bp-b5a081db-8c77-4872-b51b-e96132120710

http://crash-stats.mozilla.com/report/index/bp-ec98a57b-9589-4e13-bd58-8cf792120708

Seems to be crashing on JS_GetCompartmentPrincipals.

Any help?

Comment 1

5 years ago
Does it happen in Safe Mode (see http://support.mozilla.org/kb/troubleshoot-firefox-issues-using-safe-mode)?
Which extension causes the issue?
WFM fine on Nightly 16.0a1 (2012-07-10) on Win 7.
(Reporter)

Comment 3

5 years ago
Thanks for the feedback, I've had this happen a fair number of times recently - I'm going to leave a session on overnight without addons and see if it still does it in the morning.

I really wish I could reproduce it more frequently, such is bug finding....

Comment 4

5 years ago
There are other crashes with the same kind of comment.
It first appeared in 16.0a1/20120706075126. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=675f55c4310c&tochange=a8f682801a6d

More reports at:
https://crash-stats.mozilla.com/report/list?signature=JS_GetCompartmentPrincipals
https://crash-stats.mozilla.com/report/list?signature=JS_GetFlatStringChars
Assignee: nobody → general
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ JS_GetCompartmentPrincipals ] [@ JS_GetFlatStringChars ]
Component: General → JavaScript Engine
Ever confirmed: true
Keywords: crash, regression
Product: Firefox → Core
Summary: about:memory crash when refreshed → about:memory crash in nsScriptSecurityManager::GetSubjectPrincipal when refreshed

Comment 5

5 years ago
It might be a regression from bug 687724.
Assignee: general → nobody
Crash Signature: [@ JS_GetCompartmentPrincipals ] [@ JS_GetFlatStringChars ] → [@ JS_GetCompartmentPrincipals ] [@ JS_GetFlatStringChars ] [@ nsScriptSecurityManager::GetSubjectPrincipal(JSContext*, unsigned int*)]
Component: JavaScript Engine → Security: CAPS
Hardware: x86 → All
(Assignee)

Comment 6

5 years ago
Bug 687724 definitely looks relevant to the stack traces in comment 0.  However...

> More reports at:
> https://crash-stats.mozilla.com/report/
> list?signature=JS_GetCompartmentPrincipals
> https://crash-stats.mozilla.com/report/list?signature=JS_GetFlatStringChars

The first group crashes in JS_GetCompartmentPrincipals().  The second group crashes in JS_GetFlatStringChars().  We've had plenty of crashes in both of those prior to bug 687724 landing.  So I wonder if this is just making an existing problem more noticeable by executing the buggy code more frequently.
Blocks: 687724

Comment 7

5 years ago
(In reply to Nicholas Nethercote [:njn] from comment #6)
>  We've had plenty of crashes in both of those prior to bug 687724 landing.
But not with the following stack trace which the bug is about:
Frame 	Module 	Signature 	Source
0 	mozjs.dll 	JS_GetCompartmentPrincipals 	js/src/jsfriendapi.cpp:179
1 	xul.dll 	nsScriptSecurityManager::GetSubjectPrincipal 	caps/src/nsScriptSecurityManager.cpp:2291
2 	xul.dll 	nsScriptSecurityManager::IsCapabilityEnabled 	caps/src/nsScriptSecurityManager.cpp:2443
3 	xul.dll 	xpc::ContentScriptHasUniversalXPConnect 	js/xpconnect/wrappers/XrayWrapper.cpp:773
4 	xul.dll 	xpc::XrayUtils::IsTransparent 	js/xpconnect/wrappers/XrayWrapper.cpp:1178
5 	xul.dll 	xpc::AccessCheck::isScriptAccessOnly 	js/xpconnect/wrappers/AccessCheck.cpp:379
6 	xul.dll 	XPCWrappedNative::GetWrappedNativeOfJSObject 	js/xpconnect/src/XPCWrappedNative.cpp:1849
7 	xul.dll 	nsXPConnect::GetNativeOfWrapper 	js/xpconnect/src/nsXPConnect.cpp:1449
8 	xul.dll 	xpc::XPCJSRuntimeStats::initExtraCompartmentStats 	js/xpconnect/src/XPCJSRuntime.cpp:1721
9 	mozjs.dll 	JS::StatsCompartmentCallback 	js/src/MemoryMetrics.cpp:62
10 	mozjs.dll 	js::IterateCompartmentsArenasCells 	js/src/jsgc.cpp:4054
11 	mozjs.dll 	js::IterateChunks 	js/src/jsgc.cpp:4077
12 	xul.dll 	xpc::JSMemoryMultiReporter::CollectReports 	js/xpconnect/src/XPCJSRuntime.cpp:1759
(Assignee)

Comment 8

5 years ago
(In reply to Scoobidiver from comment #7)
> (In reply to Nicholas Nethercote [:njn] from comment #6)
> >  We've had plenty of crashes in both of those prior to bug 687724 landing.
> But not with the following stack trace which the bug is about:

I realize that.  It's still possible that bug 687724 made an existing problem more obvious.
Probably has something to do with bug 754202, but that's just the bug where we pull principals off the compartment. Here we seem to be ending up with a bad context compartment.
(Assignee)

Comment 10

5 years ago
> Here we seem to be ending up with a bad context compartment.

The context in question is one that I fake up before doing the memory measurements:

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSRuntime.cpp#1740

Is that relevant?
Yes, because it appears that you don't enter a compartment, and thus will hit the assertion here, or crash shortly thereafter on release builds:

http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#2287

Can you use the SafeJSContext instead?
(Assignee)

Comment 12

5 years ago
> Yes, because it appears that you don't enter a compartment, and thus will
> hit the assertion here, or crash shortly thereafter on release builds:

Is that explanation consistent with the fact that this crash is intermittent?  I've never seen this crash myself...

> Can you use the SafeJSContext instead?

Do you mean nsXPConnect::GetSafeJSContext()?  Sure, that's easy.  How does SafeJSContext differ from JSContext?
(In reply to Nicholas Nethercote [:njn] from comment #12)
> Is that explanation consistent with the fact that this crash is
> intermittent?  I've never seen this crash myself...

If the codepaths were non-deterministic such that we sometimes did and sometimes didn't enter a compartment, yes.


> > Can you use the SafeJSContext instead?
> 
> Do you mean nsXPConnect::GetSafeJSContext()?  Sure, that's easy.  How does
> SafeJSContext differ from JSContext?

It's a special JSContext for when chrome needs to do something that requires a JSContext (so that not everybody needs to spin up a special JSContext). It runs in a default compartment with an nsNullPrincipal.
(Assignee)

Comment 14

5 years ago
> > Is that explanation consistent with the fact that this crash is
> > intermittent?  I've never seen this crash myself...
> 
> If the codepaths were non-deterministic such that we sometimes did and
> sometimes didn't enter a compartment, yes.

The memory reporter never enters any compartments.
(In reply to Nicholas Nethercote [:njn] from comment #14)
> The memory reporter never enters any compartments.

Then it's sure going to crash if it ever triggers a call to nsScriptSecurityManager::IsCapabilityEnabled.
(Assignee)

Comment 16

5 years ago
> Then it's sure going to crash if it ever triggers a call to
> nsScriptSecurityManager::IsCapabilityEnabled.

Can you suggest steps to trigger that?

Grant, have you noticed this happening after visiting any particular web sites?  I'd really like to be able to reproduce this myself.
(Assignee)

Comment 17

5 years ago
Created attachment 642475 [details] [diff] [review]
(part 1) - Use the safe JSContext in the JS memory reporter.

Use the safe JSContext in the JS memory reporter.  This let me remove
XPCJSRuntimeStats::mCx.
Attachment #642475 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 18

5 years ago
Created attachment 642478 [details] [diff] [review]
(part 2) - Enter compartments before calling GetNativeOfWrapper() in the JS memory reporter.

Let's enter the compartment before calling GetNativeOfWrapper().  I haven't
managed to reproduce this crash, maybe I'll get lucky and this will fix it.
Attachment #642478 - Flags: review?(bobbyholley+bmo)
(Reporter)

Comment 19

5 years ago
Hi Nicholas,

The only thing that seems to be consistent in this is my use of Chatzilla open for a few days - I don't think I've seen it with Chatzilla not being opened during a session. Its very hard to reproduce.
(Assignee)

Comment 20

5 years ago
Thanks, Grant.  I use Chatzilla myself and I haven't seen this, though I don't keep it open for more than about 8 hours at a time.

Updated

5 years ago
Crash Signature: [@ JS_GetCompartmentPrincipals ] [@ JS_GetFlatStringChars ] [@ nsScriptSecurityManager::GetSubjectPrincipal(JSContext*, unsigned int*)] → [@ JS_GetCompartmentPrincipals ] [@ JS_GetFlatStringChars ] [@ nsScriptSecurityManager::GetSubjectPrincipal(JSContext*, unsigned int*)] [@ nsScriptSecurityManager::GetSubjectPrincipal]
Attachment #642475 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 642478 [details] [diff] [review]
(part 2) - Enter compartments before calling GetNativeOfWrapper() in the JS memory reporter.


> JSAutoEnterCompartment::enter(JSContext *cx, JSObject *target)
> {
>-    AssertHeapIsIdle(cx);
>+    AssertHeapIsIdleOrIterating(cx);

I don't know what this means, but presumably you do.

>diff --git a/js/xpconnect/src/XPCJSRuntime.cpp b/js/xpconnect/src/XPCJSRuntime.cpp
>--- a/js/xpconnect/src/XPCJSRuntime.cpp
>+++ b/js/xpconnect/src/XPCJSRuntime.cpp
>@@ -1747,16 +1747,21 @@ class XPCJSRuntimeStats : public JS::Run
>         nsCAutoString cJSPathPrefix, cDOMPathPrefix;
>         nsCString cName;
>         GetCompartmentName(c, cName);
> 
>         // Get the compartment's global.
>         nsXPConnect *xpc = nsXPConnect::GetXPConnect();
>         JSContext *cx = xpc->GetSafeJSContext();
>         if (JSObject *global = JS_GetGlobalForCompartmentOrNull(cx, c)) {
>+            // Need to enter the compartment, otherwise GetNativeOfWrapper()
>+            // might crash.
>+            JSAutoEnterCompartment aec;
>+            aec.enterAndIgnoreErrors(cx, global);

Should we maybe just return here if we hit an error rather than ignoring? Note that entering a compartment will become infallible with bug 625199, so it probably doesn't matter too much.
Attachment #642478 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 22

5 years ago
> > JSAutoEnterCompartment::enter(JSContext *cx, JSObject *target)
> > {
> >-    AssertHeapIsIdle(cx);
> >+    AssertHeapIsIdleOrIterating(cx);
> 
> I don't know what this means, but presumably you do.

Yes.  The heap has three states:  Idle, Iterating, Collecting.  I added "Iterating" recently.  The important thing in this case is that the heap isn't in the "Collecting" state.


> >+            // Need to enter the compartment, otherwise GetNativeOfWrapper()
> >+            // might crash.
> >+            JSAutoEnterCompartment aec;
> >+            aec.enterAndIgnoreErrors(cx, global);
> 
> Should we maybe just return here if we hit an error rather than ignoring?
> Note that entering a compartment will become infallible with bug 625199, so
> it probably doesn't matter too much.

I started doing that but the fallibility bubbled up through several layers of calls all the way to jsapi.h, and it felt like more trouble than it was worth.  And if bug 625199 is on the cards, I feel even more justified!
(In reply to Nicholas Nethercote [:njn] from comment #22)

> I started doing that but the fallibility bubbled up through several layers
> of calls all the way to jsapi.h, and it felt like more trouble than it was
> worth.  And if bug 625199 is on the cards, I feel even more justified!

I just meant to return, in the same way we handle the "OrNull" case for the global.
(Assignee)

Comment 24

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4785b5869803
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b89b2b1340b

I'm going to be optimistic and let this bug be marked as RESOLVED FIXED once it merges with mozilla-central.  Grant, if you still hit it once these patches have made it to Nightly (probably two days from now), can you open a new bug?  Thanks very much.
Component: Security: CAPS → XPConnect
https://hg.mozilla.org/mozilla-central/rev/4785b5869803
https://hg.mozilla.org/mozilla-central/rev/3b89b2b1340b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Comment 26

5 years ago
I get still crashes
https://crash-stats.mozilla.com/report/index/bp-02b152df-d852-4479-aa7a-26cf72120717

Comment 27

5 years ago
(In reply to SpeciesX from comment #26)
> I get still crashes
That's normal. The first Nightly that will contain the patch is 17.0a1/20120718.
(Assignee)

Comment 28

5 years ago
If these patches fix the crashes, we'll need to backport them to Aurora 16, or back out the original patches from bug 687724.
tracking-firefox16: --- → ?
status-firefox15: --- → unaffected
status-firefox16: --- → affected
status-firefox17: --- → fixed
Comment on attachment 642475 [details] [diff] [review]
(part 1) - Use the safe JSContext in the JS memory reporter.

Flagging these patches for for m-a at mccr8's request. They're low-risk. The first one gets rid of a hand-rolled JSContext and just uses the one from XPConnect (which njn didn't know about when he did the original implementation). The second one enters a compartment, which is basically always a Good Thing.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 687724
User impact if declined: crashes
Testing completed (on m-c, etc.): baked on m-c for >1 week.
Risk to taking this patch (and alternatives if risky): Low risk. 
String or UUID changes made by this patch: None.
Attachment #642475 - Flags: approval-mozilla-aurora?
Attachment #642478 - Flags: approval-mozilla-aurora?
SpeciesX, grant, are you still experiencing these crashes?

Looking at crash stats, it seems there are still some of these around.  I guess there could be other ways to trigger it.  I'll try to look into them.
Assignee: nobody → n.nethercote
(Reporter)

Comment 31

5 years ago
(In reply to Andrew McCreight [:mccr8] from comment #30)
> SpeciesX, grant, are you still experiencing these crashes?
> 
> Looking at crash stats, it seems there are still some of these around.  I
> guess there could be other ways to trigger it.  I'll try to look into them.


Hey Andrew,

Haven't so far, I had 70~ tabs and chatzilla open and it didn't crash, hasn't since the change.

Will let you know if it does though.

Cheers
Attachment #642475 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 642478 [details] [diff] [review]
(part 2) - Enter compartments before calling GetNativeOfWrapper() in the JS memory reporter.

low risk and crash fixing, approving for Aurora.
Attachment #642478 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
tracking-firefox16: ? → +
https://hg.mozilla.org/releases/mozilla-aurora/rev/046f31d24b35
https://hg.mozilla.org/releases/mozilla-aurora/rev/eb1cb96db8a7
status-firefox16: affected → fixed
(Reporter)

Comment 34

5 years ago
Damn, im still getting a crash on about:memory - but it looks like a different reason:


https://crash-stats.mozilla.com/report/index/bp-50c761f5-e9ca-4496-a5f7-faef42120726

Firefox 17.0a1 Crash Report [@ gfxFontCache::SizeOfFontEntryExcludingThis(gfxFontCache::HashEntry*, unsigned int (*)(void const*), void*) ]

If I get it again, I will post another bug.
(Assignee)

Comment 35

5 years ago
mccr8:  Thanks for landing on Aurora.

Grant:  Yes, that's a different crash;  a new bug would be appropriate if you see it again.
Grant, to confirm, you no longer see *this* crash in Firefox 16 and 17?
(Reporter)

Comment 37

5 years ago
Hi Nicholas/Anthony,

I have since moved to a new PC and am more then happy with FF - I haven't have a crash yet :) I think its safe to say this has been fixed.

Thanks for your help guys.
Thanks for checking back in, Grant. Unfortunately, I don't think we'll be able to call this verified fixed, especially if your new PC cannot reproduce the original crash on a reportedly affected build.

If anyone else on this bug is able to help verify it fixed, please help. Thanks.
Whiteboard: [qa-]
(Assignee)

Comment 39

5 years ago
> If anyone else on this bug is able to help verify it fixed, please help.

I think your best bet for verifying is to look for the crash signature in FF16 and FF17.  Hopefully it won't appear.
No crashes found for the following signatures with Firefox 17 or 18 in the last week:
 * JS_GetCompartmentPrincipals
 * nsScriptSecurityManager::GetSubjectPrincipal(JSContext*, unsigned int*)
 * nsScriptSecurityManager::GetSubjectPrincipal

However, JS_GetFlatStringChars has a single crash on Firefox 17.0a2 in the last week:
https://crash-stats.mozilla.com/report/index/7b599cf7-e977-4b6b-bf99-d7dc72120916
You need to log in before you can comment on or make changes to this bug.