Mats Palmgren (inactive) Reporter
	Description • 9 years ago

This bug was filed from the Socorro interface and is 
report bp-31d2f9ff-1b19-4e5c-90a2-facfd2150531.
=============================================================

New signature in v41 that first appeared in build 20150519030202.
This method was added in bug 1164977 which landed on m-c
2015-05-19 03:32:17 PDT.

	Bobby Holley (:bholley)
	Comment 1 • 9 years ago

Hm, I can see how we might crash here, but I'd expect it to be a null-deref, which this isn't. Mats, is this your crash, or are you just filing it from finding it in socorro? Figuring out the concrete type of aPrincipal and capturing the JS stack would be very helpful.

	Mats Palmgren (inactive) Reporter
	Comment 2 • 9 years ago

Sorry, it's not my crash, I just saw that this signature is new in crash data.

	Bobby Holley (:bholley)
	Comment 3 • 9 years ago

Hm, I'm pretty stumped as to how we'd get that crash signature - it appears that the cast from nsIPrincipal to BasePrincipal is causing trouble, but I don't see any nsIPrincipal implementations that don't inherit BasePrincipal:

bholley@extraordinary /files/mozilla/repos/dd (caps_prin_crash) $ grep -R "public nsIPrincipal" *
caps/nsJSPrincipals.h:class nsJSPrincipals : public nsIPrincipal, public JSPrincipals
obj-x86_64-apple-darwin14.3.0/dist/include/nsIPrincipal.h:class nsPrincipal : public nsIPrincipal
obj-x86_64-apple-darwin14.3.0/dist/include/nsJSPrincipals.h:class nsJSPrincipals : public nsIPrincipal, public JSPrincipals
bholley@extraordinary /files/mozilla/repos/dd (caps_prin_crash) $ grep -R "public nsJSPrincipal" *
caps/BasePrincipal.h:class BasePrincipal : public nsJSPrincipals
obj-x86_64-apple-darwin14.3.0/dist/include/mozilla/BasePrincipal.h:class BasePrincipal : public nsJSPrincipals
bholley@extraordinary /files/mozilla/repos/dd (caps_prin_crash) $ 


On the off chance that it's causing this somehow, I'll try fixing the release-mode assertion against passing null - but that doesn't seem to match the behavior in the crash reports. Ideas welcome.

	(Away)
	Comment 5 • 9 years ago

I don't think it's the cast. It's the first Subsumes that's failing. (The debug symbols always point to the last line of the statement)

	Bobby Holley (:bholley)
	Comment 6 • 9 years ago

(In reply to David Major [:dmajor] from comment #5)
> I don't think it's the cast. It's the first Subsumes that's failing.

Hm, how can you tell? Wouldn't the crash be in BasePrincipal::Subsumes in that case?

If that's true, it seems more likely that the problem is the MOZ_RELEASE_ASSERT. But shouldn't that crash at a predictable location?

> (The debug symbols always point to the last line of the statement)

That is good to know.

	(Away)
	Comment 7 • 9 years ago

Here's the stack:

0029c618  61a6ab8a xul!NS_InvokeByIndex+0x27  <-- return address
0029c61c  0a4d52b0 <-- this
0029c620  00000000 <-- aOther
0029c624  0029c8a0 <-- aResult

In the disassembly I can see BasePrincipal::Subsumes inlined into BasePrincipal::Equals.

	Bobby Holley (:bholley)
	Comment 8 • 9 years ago

(In reply to David Major [:dmajor] from comment #7)
> Here's the stack:
> 
> 0029c618  61a6ab8a xul!NS_InvokeByIndex+0x27  <-- return address
> 0029c61c  0a4d52b0 <-- this
> 0029c620  00000000 <-- aOther
> 0029c624  0029c8a0 <-- aResult

Ah I see! That's very helpful. Is this from crash-stats.mozilla.com, or are you getting that from the minidump?

Given that, it seems pretty clear that it's the MOZ_RELEASE_ASSERT. I was confused because the crashing address was non-zero, but I just noticed that MOZ_REALLY_CRASH actually does ::TerminateProcess(::GetCurrentProcess(), 3) on windows. Is there any way to see that this is happening from crash-stats?

	(Away)
	Comment 9 • 9 years ago

(In reply to Bobby Holley (:bholley) from comment #8)
> Ah I see! That's very helpful. Is this from crash-stats.mozilla.com, or are
> you getting that from the minidump?

Minidump.

> Given that, it seems pretty clear that it's the MOZ_RELEASE_ASSERT. I was
> confused because the crashing address was non-zero, but I just noticed that
> MOZ_REALLY_CRASH actually does ::TerminateProcess(::GetCurrentProcess(), 3)
> on windows. Is there any way to see that this is happening from crash-stats?

Before TerminateProcess, it does a __debugbreak(), which compiles to an asm "int 3". That gets caught by Breakpad as the EXCEPTION_BREAKPOINT that we see in the crash reports.

	(Away)
	Comment 10 • 9 years ago

(In the case of EXCEPTION_BREAKPOINT, the crashing address is actually the code address of the int 3)

	Gabor Krizsanits (INACTIVE)
	Comment 11 • 9 years ago

Comment on attachment 8613742 [details] [diff] [review]
Stop asserting non-null argument to nsIPrincipal::{subsumes,equals}{,ConsideringDomain}. v1

Review of attachment 8613742 [details] [diff] [review]:
-----------------------------------------------------------------

If I get it correctly this is not really a sec crit then just a null deref, no?

::: caps/BasePrincipal.cpp
@@ +75,5 @@
>  
>  bool
>  BasePrincipal::Subsumes(nsIPrincipal* aOther, DocumentDomainConsideration aConsideration)
>  {
> +  MOZ_ASSERT(aOther);

I don't really see the reason to change this from MOZ_RELEASE_ASSERT after the fixes in this patch, could you explain it?

	Bobby Holley (:bholley)
	Comment 12 • 9 years ago

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11)
> Comment on attachment 8613742 [details] [diff] [review]
> Stop asserting non-null argument to
> nsIPrincipal::{subsumes,equals}{,ConsideringDomain}. v1
> 
> Review of attachment 8613742 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If I get it correctly this is not really a sec crit then just a null deref,
> no?
> 
> ::: caps/BasePrincipal.cpp
> @@ +75,5 @@
> >  
> >  bool
> >  BasePrincipal::Subsumes(nsIPrincipal* aOther, DocumentDomainConsideration aConsideration)
> >  {
> > +  MOZ_ASSERT(aOther);
> 
> I don't really see the reason to change this from MOZ_RELEASE_ASSERT after
> the fixes in this patch, could you explain it?

Before my patch, I was interested in catching XPCOM consumers (especially extensions) that were passing null to subsumes or equals. Now that we're explicitly handling that, I don't see any special reason why we need release-mode checking on this particular assertion.

	Bobby Holley (:bholley)
	Comment 13 • 9 years ago

(In reply to Bobby Holley (:bholley) from comment #12)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11)
> > If I get it correctly this is not really a sec crit then just a null deref,
> > no?

Correct.

	Bobby Holley (:bholley)
	Comment 14 • 9 years ago

Thanks gabor for reviews, and dmajor for the insight!

	Pulsebot
	Comment 15 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce014d3475af

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/ce014d3475af

	(Away)
	Comment 17 • 9 years ago

Stats on the 0603 nightly look good. Do you still want to [leave open]?

	Bobby Holley (:bholley)
	Comment 18 • 9 years ago

(In reply to David Major [:dmajor] from comment #17)
> Stats on the 0603 nightly look good. Do you still want to [leave open]?

Thanks for following up.