812716 - Desktop Notifications: Race in nsDOMDesktopNotification constructor

Status: RESOLVED FIXED

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

Description

[Gregor Wagner [:gwagner]]

mrbkap found a race condition when we create the desktop notification object.

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

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

I'm not sure that this can actually race, but the early return in the if (!GetOwner()) case can definitely cause crashes. It's pretty hairy to reference count an object in its constructor, so this seems a lot saner in general.

Comment 3

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e2d2e78a956

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Note to blocking-basecamp triagers: this is a very low-risk correctness bug that could prevent child-process crashes in certain edge cases.

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9e2d2e78a956

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

basecamp- but please nom for beta if this is really low risk.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

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

[Approval Request Comment]
User impact if declined: In certain rare cases, content processes could crash when sending desktop notifications.
Testing completed (on m-c, etc.): This has been on m-c for a few days.

This patch is extremely low-risk, and is easily provably correct via code inspection.

Comment 8

[Alex Keybl [:akeybl]]

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

[Triage Comment]
Very low risk fix that may resolve some B2G instability. Approving for branches.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d75a9dd5b181
https://hg.mozilla.org/releases/mozilla-beta/rev/076e16981f2f