Closed Bug 944309 Opened 6 years ago Closed 6 years ago

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

Categories

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

28 Branch
x86
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox26 --- unaffected
firefox27 + verified
firefox28 + verified

People

(Reporter: ananuti, Assigned: qdot)

References

Details

(Keywords: crash, regression, reproducible, Whiteboard: [systemsfe])

Crash Data

Attachments

(1 file)

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.
last good nightly 20131023030205
first bad nightly 20131024030204
pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=21d97baadc05&tochange=19fd3388c372
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
If reproducible, this will get annoying for users (close browser on notification, crash).
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?
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Keywords: verifyme
verified on nightly 20131207030203 build.

qdot, is this fix ready to uplift on aurora 27?
Flags: needinfo?(kyle)
Yeah, should just work.
Flags: needinfo?(kyle)
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+
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
Keywords: verifyme
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.