874090 - Crash in mozilla::dom::Notification::GetPermissionInternal

Status: VERIFIED FIXED

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

Description

[Loic]

Crash found by testing bug 874054.

STR
1) Open Web Console
2) Enter:
var n = new Notification('title', {onshow:function () {console.log('Hello, World!')}});
var n2 = new Notification('title2');
n2.onshow = function () {console.log('Hello from n2')};

Result: instant crash
https://crash-stats.mozilla.com/report/index/bp-0857db5c-012e-49d5-b6d6-794b72130520

Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::dom::Notification::GetPermissionInternal 	dom/src/notification/Notification.cpp:407
1 	xul.dll 	mozilla::dom::Notification::ShowInternal 	dom/src/notification/Notification.cpp:323
2 	xul.dll 	mozilla::dom::NotificationTask::Run 	dom/src/notification/Notification.cpp:246
3 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:627
4 	xul.dll 	NS_ProcessNextEvent 	obj-firefox/xpcom/build/nsThreadUtils.cpp:238
5 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:82
6 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:212
7 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:186
8 	xul.dll 	nsBaseAppShell::Run 	widget/xpwidgets/nsBaseAppShell.cpp:163
9 	xul.dll 	nsAppShell::Run 	widget/windows/nsAppShell.cpp:113
10 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:269
11 	xul.dll 	XREMain::XRE_mainRun 	toolkit/xre/nsAppRunner.cpp:3872
12 	xul.dll 	XREMain::XRE_main 	toolkit/xre/nsAppRunner.cpp:3939
13 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:4151
14 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp:272
15 	firefox.exe 	NS_internal_main 	browser/app/nsBrowserApp.cpp:632
16 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:105
17 	firefox.exe 	__tmainCRTStartup 	crtexe.c:552
18 	kernel32.dll 	BaseThreadInitThunk 	
19 	ntdll.dll 	__RtlUserThreadStart 	
20 	ntdll.dll 	_RtlUserThreadStart

Comment 1

[Loic]

var n = new Notification('some title');

is enough to crash Nightly..

Comment 2

[Loic]

Precision for STR: it crashes only if you call "var n = new Notification('some title');" on a new blank page (Ctrl+T).

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b03bb3ce8cee&tochange=e23e43a2c14e

Suspected bug 782211.

Comment 3

[Alex Keybl [:akeybl]]

Over to dougt to reassign.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch

It's hard to create a mochitest for it. The bug was that the URI is not always set. IT can be null for about:<foo> uri.

Comment 6

[William Chen [:wchen]]

Comment on attachment 752201 [details] [diff] [review]
patch

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

Thanks for working on the fix.

::: dom/src/notification/Notification.cpp
@@ +404,5 @@
>    nsCOMPtr<nsIURI> uri;
>    principal->GetURI(getter_AddRefs(uri));
> +  if (!uri) {
> +    return NotificationPermission::Granted;
> +  }

Instead of checking for a null URI and granting permission, it should check for system principal using |nsContentUtils::IsSystemPrincipal| then grant permission. We should still check for a null URI by wrapping the relevant code in an if (uri) { ... }.

This fix also needs to happen in NotificationPermissionRequest::Run(), it has the same problem.

The code for checking the principal should probably be factored out, something to the effect of IsPermissionAlwaysGrantedForPrincipal.

I have attached a test that tests both cases that cause the crash.

Comment 7

[William Chen [:wchen]]

Attachment: Test web notification with system principal

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch

Test is included in the patch.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 10

[William Chen [:wchen]]

Comment on attachment 752607 [details] [diff] [review]
patch

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

::: dom/src/notification/Notification.cpp
@@ +112,5 @@
> +    nsCOMPtr<nsIURI> uri;
> +    mPrincipal->GetURI(getter_AddRefs(uri));
> +
> +    if (uri) {
> +    bool isFile;

fix indentation

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 12

[Phil Ringnalda (:philor)]

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

Comment 13

[Phil Ringnalda (:philor)]

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

Comment 14

[Alex Keybl [:akeybl]]

Can we get an uplift nomination for Aurora/Beta?

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 753514 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: a crash when notifications are request from a non-http page.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky):  Not too much. This code is fully tested.
String or IDL/UUID changes made by this patch:none.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3896df3b83e5
https://hg.mozilla.org/releases/mozilla-beta/rev/3e017e6d6b47

Comment 17

[Manuela Muntean [Away]]

Verified fixed with Firefox 22 beta 4 (build ID: 20130605070403) on a Windows 7 64bit machine, using the STR from comment 2.

Also, no new crash reports in Socorro since 2013-05-24 regarding the last month, and there is only 1 crash on 22.0b1.

Reports are available here: 

https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=mozilla%3A%3Adom%3A%3ANotification%3A%3AGetPermissionInternal%28nsISupports%2A%2C%20mozilla%3A%3AErrorResult%26amp%3B%29&reason_type=contains&date=06%2F06%2F2013%2009%3A17%3A58&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozilla%3A%3Adom%3A%3ANotification%3A%3AGetPermissionInternal%28nsISupports%2A%2C%20mozilla%3A%3AErrorResult%26%29

Comment 18

[Ioana (away)]

Verified as fixed with the STR in comment 0 on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0

There is only one crash with this signature in Socorro for the last 4 weeks:
https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=mozilla%3A%3Adom%3A%3ANotification%3A%3AGetPermissionInternal%28nsISupports%2A%2C%20mozilla%3A%3AErrorResult%26%29

Comment 19

[Ioana (away)]

Verified as fixed on Firefox 24 beta 1:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0

There are no Firefox 24 or 25 crashes with this signature in Socorro either.
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Adom%3A%3ANotification%3A%3AGetPermissionInternal%28nsISupports%2A%2C+mozilla%3A%3AErrorResult%26%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&hang_type=any&date=2013-08-07+09%3A00%3A00&range_value=4