Closed Bug 874090 Opened 7 years ago Closed 7 years ago

Crash in mozilla::dom::Notification::GetPermissionInternal

Categories

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

22 Branch
All
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox21 --- unaffected
firefox22 + verified
firefox23 + verified
firefox24 --- verified

People

(Reporter: epinal99-bugzilla2, Assigned: baku)

References

Details

(Keywords: crash, regression, reproducible)

Crash Data

Attachments

(1 file, 4 obsolete files)

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
Severity: normal → critical
Crash Signature: [@ mozilla::dom::Notification::GetPermissionInternal(nsISupports*, mozilla::ErrorResult&) ]
var n = new Notification('some title');

is enough to crash Nightly..
Keywords: regression
Hardware: x86_64 → All
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.
Blocks: 782211
Version: 24 Branch → 22 Branch
Over to dougt to reassign.
Assignee: nobody → doug.turner
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #752201 - Flags: review?(wchen)
Assignee: doug.turner → amarchesini
Duplicate of this bug: 871460
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.
Attachment #752201 - Flags: review?(wchen) → review-
Attached patch patch (obsolete) — Splinter Review
Test is included in the patch.
Attachment #752201 - Attachment is obsolete: true
Attachment #752591 - Attachment is obsolete: true
Attachment #752606 - Flags: review?
Attached patch patch (obsolete) — Splinter Review
Attachment #752606 - Attachment is obsolete: true
Attachment #752606 - Flags: review?
Attachment #752607 - Flags: review?(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
Attachment #752607 - Flags: review?(wchen) → review+
Attached patch patchSplinter Review
Attachment #752607 - Attachment is obsolete: true
Attachment #753514 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ddcf4ed9674f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Can we get an uplift nomination for Aurora/Beta?
Flags: needinfo?(amarchesini)
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.
Attachment #753514 - Flags: approval-mozilla-beta?
Attachment #753514 - Flags: approval-mozilla-aurora?
Attachment #753514 - Flags: approval-mozilla-beta?
Attachment #753514 - Flags: approval-mozilla-beta+
Attachment #753514 - Flags: approval-mozilla-aurora?
Attachment #753514 - Flags: approval-mozilla-aurora+
Keywords: verifyme
Flags: needinfo?(amarchesini)
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.