1569405 - Crash in [@ nsContentPermissionRequestProxy::GetPrincipal]

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P3)

Description

[[:philipp]]

This bug is for crash report bp-70349c7d-05d1-448f-a1fc-b46540190726.

Top 10 frames of crashing thread:

0 xul.dll nsresult nsContentPermissionRequestProxy::GetPrincipal dom/base/nsContentPermissionHelper.cpp:880
1 xul.dll XPTC__InvokebyIndex 
2  @0xd0979fd3b7 
3 xul.dll round 
4 xul.dll static bool XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1157
5 xul.dll static bool XPC_WN_GetterSetter js/xpconnect/src/XPCWrappedNativeJSOps.cpp:983
6 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:535
7 xul.dll js::CallGetter js/src/vm/Interpreter.cpp:730
8 xul.dll js::NativeGetProperty js/src/vm/NativeObject.cpp:2554
9 xul.dll js::GetProperty js/src/vm/Interpreter.cpp:4489

this cross-platform signature has been around for a while already (it's rather low volume).

it's heavily correlated to users of cs builds - a little bit under half of all reports are submitted by them, commonly crashing on one of the subpages of https://search.seznam.cz which we are also shipping as search engine to users in the czech republic.

Comment 1

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

Bug 1444503 changed some related code.

Comment 2

[BugBot [:suhaib / :marco]]

The priority flag is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[Jens Stutte [:jstutte]]

So looking at GetPrincipal it seems to me:

• all the stacks I looked at call GetPrincipal from JS
• aRequestingPrincipal thus is most likely non-null (being probably created ad-hoc for the JS call)
• mParent is checked to be non-null already
• mParent->mPrincipal could be nullptr and we fail doing NS_ADDREF on it.

If I look at how ContentPermissionRequestParent is created there seems to be nothing down the way from PContent::Msg_PContentPermissionRequestConstructor that would prevent mPrincipal from being nullptr.

I think we could:

• add a paranoia check here
• add an assertion for example in ContentParent::AllocPContentPermissionRequestParent to check the incoming aPrincipal

Note that all crashes happen on Android only, so there might be some special Android way of initializing these objects.

:smaug, does that sound reasonable?

Comment 4

[Jens Stutte [:jstutte]]

(In reply to Jens Stutte [:jstutte] from comment #3)

• add an assertion for example in ContentParent::AllocPContentPermissionRequestParent to check the incoming aPrincipal

IIUC we can come here only through nsContentPermissionUtils::AskPermission where we feed in a principal coming from ContentPermissionRequestBase::GetPrincipal which seems to be expected to be potentially nullptr looking at the NS_IF_ADDREF there.

So I assume that we can just make nsContentPermissionRequestProxy::GetPrincipal tolerant for nullptr principals, too.

Comment 5

[Jens Stutte [:jstutte]]

And that NS_IF_ADDREF has been added on purpose by bug 1512179. Unfortunately I see no discussion in that bug or patch if having nullptr principals is really expected, but for symmetry I'd propose to do the same here.

Comment 6

[Jens Stutte [:jstutte]]

Attachment: Bug 1569405 - Avoid NS_ADDREF on nullptr principals by using NS_IF_ADDREF. r?smaug

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a25bf269e7b3
Avoid NS_ADDREF on nullptr principals by using NS_IF_ADDREF. r=smaug

Comment 8

[Norisz Fay [:noriszfay]]

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

Comment 9

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:jstutte, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Comment 10

[Jens Stutte [:jstutte]]

Comment on attachment 9322731 [details]
Bug 1569405 - Avoid NS_ADDREF on nullptr principals by using NS_IF_ADDREF. r?smaug

Beta/Release Uplift Approval Request

• User impact if declined: Apparently only on Android we see some crashes associated with this.
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This really just adds a null check that prevents us from crashing in this specific point and it seems that anywhere else we handle the same thing being null, already.
• String changes made/needed:
• Is Android affected?: Yes

Comment 11

[Dianna Smith [:diannaS]]

Comment on attachment 9322731 [details]
Bug 1569405 - Avoid NS_ADDREF on nullptr principals by using NS_IF_ADDREF. r?smaug

Approved for 112.0b3

Comment 12

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2b1afe72ea4c