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

Status: VERIFIED FIXED

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

Description

[Ekanan Ketunuti]

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.

Comment 1

[Ekanan Ketunuti]

aurora 27.0a2:
bp-68ea853f-fd20-4edb-a5c0-9bbd82131128
bp-fe5aa4da-bbec-4ccf-98d7-6ddb62131128

Comment 2

[Ekanan Ketunuti]

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

Comment 3

[Ekanan Ketunuti]

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

Comment 4

[Ekanan Ketunuti]

last good b2g-inbound changeset 0ab4e1cae487
http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/b2g-inbound-win32/1382554944/

first bad b2g-inbound changeset 119d46e5cec3
http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/b2g-inbound-win32/1382560415/

caused by bug 899574 :)

Comment 5

[Alex Keybl [:akeybl]]

If reproducible, this will get annoying for users (close browser on notification, crash).

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Reassigning ni? to owner of bug 899574.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 8

[Gregor Wagner [:gwagner]]

(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?

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Change null check from assert to full check/fail

Comment 10

[Gregor Wagner [:gwagner]]

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

Lets ask a dom peer.

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Ekanan Ketunuti]

verified on nightly 20131207030203 build.

qdot, is this fix ready to uplift on aurora 27?

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yeah, should just work.

Comment 15

[Ekanan Ketunuti]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/21e6b31e84a1

Comment 17

[Ada [:adalucinet]]

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.