1127534 - Desktop notification with icon hits "Assertion failure: nsContentUtils::IsSystemPrincipal(triggeringPrincipal)"

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: nsContentUtils::IsSystemPrincipal(triggeringPrincipal), at /Users/jruderman/trees/mozilla-central/image/src/imgLoader.cpp:714

Comment 1

[Jesse Ruderman]

Attachment: stack (no e10s)

Comment 2

[Jesse Ruderman]

Attachment: stack (e10s)

Comment 3

[Jesse Ruderman]

Should I be scared?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Hey Jesse,
I see you filed that bug quite a while ago [2015-01-29]. It seems to work for me using the latest nightly. I assume the problem got fixed in the meantime. Also, Notifications are quite heavily used these days so I would be surprised if there is an issue.

If it's still problem, please let me know, otherwise we can close this bug with 'worksforme'.

Comment 5

[Jesse Ruderman]

I can still reproduce on Mac.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jesse Ruderman from comment #5)
> I can still reproduce on Mac.

Ah, I should have checked the bug-flags before answering, because I tested on Linux only. But apparently that's a mac specific problem.

Looking at the code reveals that mac sends null [1] as the context to LoadImage, which then calls NewImageChannel with a null ctx [2] which causes the loadingPrincipal to become the triggeringPrincipal, which ultimately triggers the assertion [3].

It seems that we can't query a doc/node within the OSXNotificationCenter.

Jonas, how would you like to proceed?

[1] http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/OSXNotificationCenter.mm#250
[2] http://mxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#1601
[3] http://mxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#792

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Since this channel is created in the parent process, there's no way we can get a document here in the e10s case. Technically we could probably get a document in the non-e10s case, but that doesn't seem worth it.

I'd say just remove the assertion and document that notifications (and eventually favicons) create a channel in the parent process and so we can't get a requestingNode in that case.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also, Jesse, you are awesome! Bugs like this, with a testcase and a stack, rock!

Comment 9

[Tanvi Vyas[:tanvi]]

(In reply to Jonas Sicking (:sicking) from comment #7) 
> I'd say just remove the assertion and document that notifications (and
> eventually favicons) create a channel in the parent process and so we can't
> get a requestingNode in that case.


What do you mean by eventually favicons?

Can we get a requestingPrincipal even if we can't get a node?  Favicons should go through security checks, and without a principal they can't.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, we can get a principal since you can send a principal across an IPC boundary. But you can't send a node.

The favicon code does its own security checks, and then load the image using a <xul:image>. That means that it currently uses a system principal. Which is fine because it's doing its own security checks.

But we talked about making it possible to set a loadingPrincipal on a <xul:image> such that we do the load using the "correct" principal instead, and remove the custom favicon security checks.

But that's not the case yet.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1127534_remove_assertion_imgLoader.patch

(In reply to Jonas Sicking (:sicking) from comment #7)
> I'd say just remove the assertion and document that notifications (and
> eventually favicons) create a channel in the parent process and so we can't
> get a requestingNode in that case.

As discussed with Jonas over IRC, we decided to just remove the assertion.

Jonas said it's ok to r+ the patch in his name.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/87ff486a4b6afacf9e8dba60a61e17ce431e33b4
changeset:  87ff486a4b6afacf9e8dba60a61e17ce431e33b4
user:       Christoph Kerschbaumer <[e-mail]>
date:       Sun Aug 02 10:42:22 2015 -0700
description:
Bug 1127534 - Remove assertion before creating a channel (r=sicking)

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87ff486a4b6a