1331619 - Crash when clicking Push Notification while only private window is opened @ nsContentUtils::DispatchFocusChromeEvent

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Tooru Fujisawa [:arai]]

Steps to reproduce:
  1. open Nightly or Firefox 50.1.0
  2. open https://gauntface.github.io/simple-push-demo/
  3. click "Enable Push Notifications"
  4. choose "Always Receive Notification" (release) or "Allow Notification" (nightly)
  5. copy curl command shown in the page to send push notification
  6. open Private Browsing window
  7. close normal window (so that only Private Browsing window is opened)
  8. run the curl command copied in step 5
  9. receive push notification
 10. double click the notification

Actual result:
  Private Browsing window shows "Gah. Your tab just crashed."

Expected result:
  Opens the link either in normal window or private window (maybe normal window is better?)

here's 2 crash reports for nightly and release
  nightly https://crash-stats.mozilla.com/report/index/f94e6aef-1a88-40ae-a884-e75622170117
  release https://crash-stats.mozilla.com/report/index/ecef0410-255c-4953-b67e-0d9732170117

marking as s-s just in case, since it looks like crashing with random(?) address dereference.

Comment 1

[Daniel Veditz [:dveditz]]

Ben: does this look like your area? If not can you think of someone else?

Comment 2

[Andrew McCreight [:mccr8]]

My guess is that the problem is that OpenWindowRunnable::Run() should check the return value of OpenWindow2, which is failing for some reason, so the window is null.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I think comment 2 is correct.

This appears to be a nullptr de-ref.  Can we open up the bug?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Seems this code was added in bug 1172870.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make ServiceWorkerClients check the return value of OpenWindow2(). r=catalinb

Comment 6

[Nicholas Nethercote [inactive]]

bkelly: please add "[must_use]" to the openWindow2 IDL declaration! That will cause MOZ_MUST_USE to be added to the generated C++ declaration, which will prevent any future repeats of this problem. (And you could do likewise with other methods in the same .idl file, if appropriate.)

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> bkelly: please add "[must_use]" to the openWindow2 IDL declaration! That
> will cause MOZ_MUST_USE to be added to the generated C++ declaration, which
> will prevent any future repeats of this problem. (And you could do likewise
> with other methods in the same .idl file, if appropriate.)

Can you file a follow-up for that?  We will want to uplift this bug and I don't want to have to roto-till a bunch of other code in the process.

Comment 8

[Nicholas Nethercote [inactive]]

I filed 1332453 for adding [must_use].

Comment 9

[Cătălin Badea (:catalinb)]

Comment on attachment 8828454 [details] [diff] [review]
Make ServiceWorkerClients check the return value of OpenWindow2(). r=catalinb

Review of attachment 8828454 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77034223b8a6
Make ServiceWorkerClients check the return value of OpenWindow2(). r=catalinb

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/77034223b8a6

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8828454 [details] [diff] [review]
Make ServiceWorkerClients check the return value of OpenWindow2(). r=catalinb

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1172870
[User impact if declined]: Crashes if they receive a push notification without a content window open.
[Is this code covered by automated tests?]: We have many tests, but they don't exercise this path.
[Has the fix been verified in Nightly?]: I verified that nightly no longer crashes with the STR.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal risk.
[Why is the change risky/not risky?]: We are simply adding some error checking to avoid a nullptr deref.
[String changes made/needed]: None

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8828454 [details] [diff] [review]
Make ServiceWorkerClients check the return value of OpenWindow2(). r=catalinb

add missing error checking in ServiceWorkerClients, beta52+, aurora53+

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5cf2dfa61bf5b52680f7e4bed862c359a7169a62

https://hg.mozilla.org/releases/mozilla-aurora/rev/86c2de8ec8d1741d9fa27cb8707bfd8248d5d1e0