1316271 - Crash in nsContentUtils::SubjectPrincipal

Status: RESOLVED DUPLICATE of bug 1317167

(Core :: DOM: Core & HTML, defect, P1)

Description

[Ting-Yu Chou [:ting] (away)]

This bug was filed from the Socorro interface and is 
report bp-6dbf1e0c-692f-4fcb-b52c-4b2eb2161108.
=============================================================
#1 top crash on Android for Nightly 20161106030203, 12 crashes from 7 installations.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

The code is using hazard nsContentUtils::IsCallerChrome when nsContentUtils::LegacyIsCallerChromeOrNativeCode() should be used.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #1)
> The code is using hazard nsContentUtils::IsCallerChrome when
> nsContentUtils::LegacyIsCallerChromeOrNativeCode() should be used.

Please stop suggesting that - the "Legacy" is there for a reason. We've talked about this several times.

The correct solution is to remove the SubjectPrincipal call, and have the method take a boolean parameter indicating whether the caller is privileged. Then we mark the relevant WebIDL entry points as [NeedsSubjectPrincipal], and pass "true" for system entry points.

Should be a relatively straightforward mechanical change for a DOM hacker.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yes, we should be aiming to eliminate IsCallerChrome, as well as the Legacy* APIs, from the tree...

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

FWIW, my point is that nsContentUtils::IsCallerChrome() is so super hazard API, that we shouldn't have such method in tree at all, but just use nsContentUtils::LegacyIsCallerChromeOrNativeCode() until we've removed all the cases that is needed.
This particular code is now in release builds, so we happily crash there.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Bobby, IsCallerChrome tests for system principal or IsUniversalXPConnectEnabled().  The latter isn't really captured by webidl's NeedsSubjectPrincipal, right?

Do we not really care about IsUniversalXPConnectEnabled() anymore?

Comment 6

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #5)
> Bobby, IsCallerChrome tests for system principal or
> IsUniversalXPConnectEnabled().  The latter isn't really captured by webidl's
> NeedsSubjectPrincipal, right?
> 
> Do we not really care about IsUniversalXPConnectEnabled() anymore?

Pretty much. UniversalXPConnect only matters insofar as it's necessary for supporting legacy tests. If you can remove support for it at a callsite while keeping the tree green, feel free to do so. If a test breaks, feel free to fix the test.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, right.  I had totally forgotten how much we had nerfed it.

OK, filed bug 1316480.

Comment 8

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: Android Nightly top crash.

Comment 9

[Marcia Knous [:marcia]]

Tracking 52+ for this new top crash.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Bug 1316758 suggests the right course of action is removing the call altogether.

That bug also points out that the existing LegacyIsCallerChromeOrNativeCode() callers in this file are wrong because they allow pages to do stuff they shouldn't be able to do.  :(

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Blake, looks we need to have bug 1316758 be prioritized to fix this top crash. Thank you.

Comment 12

[Blake Wu [:bwu][:blakewu]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #11)
> Hi Blake, looks we need to have bug 1316758 be prioritized to fix this top
> crash. Thank you.
Sure!

Comment 13

[Alastor Wu [:alwu]]

Same issue, root cause is on the bug1317167 comment13.