crash in mozilla::dom::Notification::GetOrigin(nsPIDOMWindow*, nsString&)

VERIFIED FIXED in Firefox 27

Status

()

--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: ananuti, Assigned: qdot)

Tracking

({crash, regression, reproducible})

28 Branch
mozilla28
x86
Windows 7
crash, regression, reproducible
Points:
---

Firefox Tracking Flags

(firefox26 unaffected, firefox27+ verified, firefox28+ verified)

Details

(Whiteboard: [systemsfe], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
This bug was filed from the Socorro interface and is 
report bp-71b5e5fa-acb9-42b3-94e0-db9c32131128.
=============================================================

steps

- start firefox with new profile
- sign-in gmail
- enable desktop notifications for gmail
- granted notification
- close firefox immediately when new mail notification popup.
(Reporter)

Comment 1

5 years ago
aurora 27.0a2:
bp-68ea853f-fd20-4edb-a5c0-9bbd82131128
bp-fe5aa4da-bbec-4ccf-98d7-6ddb62131128
status-firefox26: --- → unaffected
status-firefox27: --- → affected
tracking-firefox27: --- → ?
Keywords: regressionwindow-wanted
Flags: needinfo?(nsm.nikhil)
(Reporter)

Comment 2

5 years ago
last good nightly 20131023030205
first bad nightly 20131024030204
pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=21d97baadc05&tochange=19fd3388c372
(Reporter)

Comment 3

5 years ago
last good: 4ceaf2926e5b
first bad: ea2c865af416
pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4ceaf2926e5b&tochange=ea2c865af416
suspected: mozilla-central changeset 119d46e5cec3 Michael Henretty — Bug 899574 - Part 2, add Notification.get() with notification storage. r=bent
Keywords: regressionwindow-wanted → regression

Comment 5

5 years ago
If reproducible, this will get annoying for users (close browser on notification, crash).
tracking-firefox27: ? → +
tracking-firefox28: ? → +
Whiteboard: [systemsfe]
Reassigning ni? to owner of bug 899574.
Flags: needinfo?(nsm.nikhil) → needinfo?(mhenretty)
Can't repro on linux 64-bit nightly 2013-11-27 (will try on windows next). Tried both gmail repro and using QA's notification test page (http://mozilla.github.io/qa-testcase-data/webapi/notifications/index.html).

There's a chance this could've somehow been caused by GetOwner() returning null during CloseInternal (possibly due to the fact that CloseInternal is called async and the window might've been closed before the task is run, but that's a sheer guess from looking at the code). While there's an assert to check for the validity of the owner window in GetOrigin, that's not gonna fire in release builds, leading to the crash. However, that also means we wouldn't be able to clean up our storage.
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #7)
> Can't repro on linux 64-bit nightly 2013-11-27 (will try on windows next).
> Tried both gmail repro and using QA's notification test page
> (http://mozilla.github.io/qa-testcase-data/webapi/notifications/index.html).
> 
> There's a chance this could've somehow been caused by GetOwner() returning
> null during CloseInternal (possibly due to the fact that CloseInternal is
> called async and the window might've been closed before the task is run, but
> that's a sheer guess from looking at the code). While there's an assert to
> check for the validity of the owner window in GetOrigin, that's not gonna
> fire in release builds, leading to the crash. However, that also means we
> wouldn't be able to clean up our storage.

Yeah if the window is closed it can return null. Lets just return an error in this case?
Created attachment 8341861 [details] [diff] [review]
Patch 1 (v1) - Change null check from assert to full check/fail
Assignee: nobody → kyle
Attachment #8341861 - Flags: review?(mhenretty)
Attachment #8341861 - Flags: review?(anygregor)
Flags: needinfo?(mhenretty)
Comment on attachment 8341861 [details] [diff] [review]
Patch 1 (v1) - Change null check from assert to full check/fail

Lets ask a dom peer.
Attachment #8341861 - Flags: review?(anygregor) → review?(mrbkap)
Attachment #8341861 - Flags: review?(mrbkap) → review+
Attachment #8341861 - Flags: review?(mhenretty) → review+
https://hg.mozilla.org/mozilla-central/rev/da8591a80c4f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox28: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

5 years ago
Keywords: verifyme
(Reporter)

Comment 13

5 years ago
verified on nightly 20131207030203 build.

qdot, is this fix ready to uplift on aurora 27?
status-firefox28: fixed → verified
Flags: needinfo?(kyle)
Yeah, should just work.
Flags: needinfo?(kyle)
(Reporter)

Comment 15

5 years ago
Comment on attachment 8341861 [details] [diff] [review]
Patch 1 (v1) - Change null check from assert to full check/fail

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 899574
User impact if declined: get crash when close firefox on notification
Testing completed (on m-c, etc.): m-c since 12/06, aurora 28.0a2
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none
Attachment #8341861 - Flags: approval-mozilla-beta?
Attachment #8341861 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 17

5 years ago
Mozilla/5.0 (Windows NT 6.1; rv:27.0) Gecko/20100101 Firefox/27.0

Reproduced with Nightly from 2013-11-28 and Firefox 27 beta 1.
Verified as fixed on Firefox 27 beta 2 (Build ID: 20131216183647) with STR from comment 0.
Status: RESOLVED → VERIFIED
status-firefox27: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.