1280497 - fatal assertion when adding notification permission for page in a container tab (permission user context should be set to default!)

Status: VERIFIED FIXED

(Core :: DOM: Security, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Steps to reproduce:
 1. log in to a slack instance (you need access to one) in a container tab
 2. click the yellow bar at the top where slack says it wants permission for notifications
 3. (maybe, forgot if I needed to do this) click in the doorhanger to accept adding the notification permission

This causes a fatal assertion (which was added in bug 1238723), in ContentChild::RecvAddPermission.

Assertion failure: attrs.mUserContextId == nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID (permission user context should be set to default!), at /home/dbaron/builds/ssd/mozilla-central/mozilla/dom/ipc/ContentChild.cpp:2534


#5  0x00007f6a211ef3d0 in <signal handler called> ()
    at /lib/x86_64-linux-gnu/libpthread.so.0
#6  0x00007f6a1de91797 in mozilla::dom::ContentChild::RecvAddPermission(IPC::Permission const&) (this=<optimized out>, permission=...)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/dom/ipc/ContentChild.cpp:2525
#7  0x00007f6a1ca8e183 in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) (this=0x1a8ca80, msg__=...)
    at /home/dbaron/builds/ssd/mozilla-central/obj/firefox-debugopt/ipc/ipdl/PContentChild.cpp:8576
#8  0x00007f6a1c7364f7 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) (this=0x1a8cae8, aMsg=...)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/ipc/glue/MessageChannel.cpp:1658
#9  0x00007f6a1c740a1a in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (this=this@entry=0x1a8cae8, aMsg=aMsg@entry=<unknown type in /home/dbaron/bin/running-firefox/libxul.so, CU 0x27a7831, DIE 0x287cb61>)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/ipc/glue/MessageChannel.cpp:1596
#10 0x00007f6a1c741ba2 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() (this=0x1a8cae8)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/ipc/glue/MessageChannel.cpp:1563
#11 0x00007f6a1c742bcf in nsRunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run() (args=..., m=<optimized out>, o=<optimized out>)
    at /home/dbaron/builds/ssd/mozilla-central/obj/firefox-debugopt/dist/include/nsThreadUtils.h:722
#12 0x00007f6a1c742bcf in nsRunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run() (m=<optimized out>, o=<optimized out>, this=<optimized out>)
    at /home/dbaron/builds/ssd/mozilla-central/obj/firefox-debugopt/dist/include/nsThreadUtils.h:729
#13 0x00007f6a1c742bcf in nsRunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run() (this=<optimized out>)
    at /home/dbaron/builds/ssd/mozilla-central/obj/firefox-debugopt/dist/include/nsThreadUtils.h:756
#14 0x00007f6a1c743aa8 in mozilla::ipc::MessageChannel::DequeueTask::Run() (this=<optimized out>)
    at /home/dbaron/builds/ssd/mozilla-central/obj/firefox-debugopt/dist/include/mozilla/ipc/MessageChannel.h:476
#15 0x00007f6a1c743aa8 in mozilla::ipc::MessageChannel::DequeueTask::Run() (this=<optimized out>)
    at /home/dbaron/builds/ssd/mozilla-central/obj/firefox-debugopt/dist/include/mozilla/ipc/MessageChannel.h:495
#16 0x00007f6a1c2e0603 in nsThread::ProcessNextEvent(bool, bool*) (this=0x1b8f8a0, aMayWait=<optimized out>, aResult=0x7fff840f1907)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/xpcom/threads/nsThread.cpp:1029
#17 0x00007f6a1c30d36b in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x1b8f8a0, aMayWait=<optimized out>)
---Type <return> to continue, or q <return> to quit---
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/xpcom/glue/nsThreadUtils.cpp:290
#18 0x00007f6a1c737b53 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x1a8a0d0, aDelegate=0x7fff840f1c18)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/ipc/glue/MessagePump.cpp:132
#19 0x00007f6a1c6f4186 in MessageLoop::RunInternal() (this=0x7fff840f1c18)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/ipc/chromium/src/base/message_loop.cc:235
#20 0x00007f6a1c6f41da in MessageLoop::Run() (this=<optimized out>)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/ipc/chromium/src/base/message_loop.cc:228
#21 0x00007f6a1c6f41da in MessageLoop::Run() (this=<optimized out>)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/ipc/chromium/src/base/message_loop.cc:208
#22 0x00007f6a1e0c5657 in nsBaseAppShell::Run() (this=0x210d7b0)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/widget/nsBaseAppShell.cpp:156
#23 0x00007f6a1e9c9283 in XRE_RunAppShell() ()
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/toolkit/xre/nsEmbedFunctions.cpp:834
#24 0x00007f6a1c737bf3 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (this=0x1a8a0d0, aDelegate=0x7fff840f1c18)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/ipc/glue/MessagePump.cpp:285
#25 0x00007f6a1c6f4186 in MessageLoop::RunInternal() (this=0x7fff840f1c18)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/ipc/chromium/src/base/message_loop.cc:235
#26 0x00007f6a1c6f41da in MessageLoop::Run() (this=<optimized out>)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/ipc/chromium/src/base/message_loop.cc:228
#27 0x00007f6a1c6f41da in MessageLoop::Run() (this=this@entry=0x7fff840f1c18)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/ipc/chromium/src/base/message_loop.cc:208
#28 0x00007f6a1e9c9a75 in XRE_InitChildProcess(int, char**, XREChildData const*) (aArgc=3, 
    aArgc@entry=4, aArgv=aArgv@entry=0x7fff840f3128, aChildData=aChildData@entry=0x7fff840f2ff8)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/toolkit/xre/nsEmbedFunctions.cpp:664
#29 0x0000000000408908 in content_process_main(int, char**) (argc=<optimized out>, argv=0x7fff840f3128)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/ipc/app/../contentproc/plugin-container.cpp:224
#30 0x00007f6a1a76c830 in __libc_start_main (main=
    0x4081e6 <main(int, char**)>, argc=5, argv=0x7fff840f3128, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fff840f3118)
    at ../csu/libc-start.c:291
#31 0x0000000000408339 in _start ()
(gdb) f 6
#6  0x00007f6a1de91797 in mozilla::dom::ContentChild::RecvAddPermission (
    this=<optimized out>, permission=...)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/dom/ipc/ContentChild.cpp:2525
2525	  MOZ_ASSERT(permissionManager,
(gdb) l
2520	#if MOZ_PERMISSIONS
2521	  nsCOMPtr<nsIPermissionManager> permissionManagerIface =
2522	    services::GetPermissionManager();
2523	  nsPermissionManager* permissionManager =
2524	    static_cast<nsPermissionManager*>(permissionManagerIface.get());
2525	  MOZ_ASSERT(permissionManager,
2526	         "We have no permissionManager in the Content process !");
2527	
2528	  nsAutoCString originNoSuffix;
2529	  PrincipalOriginAttributes attrs;
(gdb) p attrs.mUserContextId
$1 = 2

Comment 1

[Dave Huseby [:huseby]]

try push for the fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2bd2f184f19

Comment 2

[Dave Huseby [:huseby]]

Attachment: Bug_1280497.patch

This patch removes the fatal ASSERT because it is irrelevant in this case.  I added a line in PermissionManager to ensure that the user context id is set to the default in the function called by the permission hash key generation code.

Comment 3

[Dave Huseby [:huseby]]

I verified this fix with the following procedure:

1) enable userContextId in prefs.
2) open personal context tab, go here: http://www.bennish.net/web-notifications.html
3) click the "authorize" button and then grant the permission in the door hanger.
4) click the "show" button to test that the notification works.
5) open a business context tab, go to the same site.
6) click the "authorize" button and notice that the permission is already granted because permissions aren't isolated by user context id.
7) click the "show" button to test that the notification works.

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8763251 [details] [diff] [review]
Bug_1280497.patch

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

It's OK because for now we don't have permissions per container.

Comment 5

[Tanvi Vyas[:tanvi]]

Thanks Dave for the fix!


I have a few things I'd like to understand better about permissions:

What was the usercontextId set to instead of 0 in the report from comment 0?  And where was it set?  And where is the code that forces the usercontextId to 0?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e327cdc2f35
fixes bug 1280497 by removing the assert and adding missing code in permission manage. r= baku

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e327cdc2f35

Comment 9

[Andrea Marchesini [:baku]]

> What was the usercontextId set to instead of 0 in the report from comment 0?
> And where was it set?  And where is the code that forces the usercontextId
> to 0?

It was the userContextId of the page. but we don't use userContextId attribute for permission checks.
It must be 0. It could change in the future, if we want to have different permissions for different UCI.

Comment 10

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I think this needs to be uplifted to 49 so that we can run the test pilot there without this crash.

Comment 11

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Comment on attachment 8763251 [details] [diff] [review]
Bug_1280497.patch

Approval Request Comment
[Feature/regressing bug #]: Testpilot for containers
[User impact if declined]: Permissions UI will not function in certain situation with containers test-pilot. This patch also more generally addresses containers interaction with permission prompts, so may also fix undiscovered bugs. 
[Describe test coverage new/current, TreeHerder]: Some Origin Attributes testing is covered in CAPS unit tests, more is in progress.
[Risks and why]:  This involves platform changes that cannot be reprecated by an addon.  In order to do a Test Pilot experiment for Containers in September / Firefox 49 release, we need this code in release.  Without it, testing is delayed until November and getting the capabilities of the feature to users is delayed even further.
[String/UUID change made/needed]: None

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8763251 [details] [diff] [review]
Bug_1280497.patch

Container/test pilot feature, let's try uplift to aurora.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1ee8ca4958a5

Comment 14

[Kamil Jozwiak [:kjozwiak]]

I reproduced the original issue using the STR from comment#0 and comment#3 using the following build:
* fx50.0a1, buildId: 20160726173203, changeset: b9f4f3806395 (using --enable-debug)
* Received the following assertion: https://pastebin.mozilla.org/8887108

I went through verification using the following builds using the test cases from comment#0 and comment#3 without any issues:

* https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-27-03-02-30-mozilla-central/
* fx50.0a1 buildId: 20160727103716, changeset: fef429fba4c6 tip (using --enable-debug)
* https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-27-00-40-19-mozilla-aurora/
* fx49.0a2 buildId: 20160727113033, changeset: a9e8f70d870c tip (using --enable-debug)