Closed Bug 1331619 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- disabled
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: arai, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Attachments

(1 file)

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.
Ben: does this look like your area? If not can you think of someone else?
Flags: needinfo?(bkelly)
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.
I think comment 2 is correct.

This appears to be a nullptr de-ref.  Can we open up the bug?
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(bkelly)
Seems this code was added in bug 1172870.
Group: core-security
Component: DOM: Push Notifications → DOM: Service Workers
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.)
(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.
I filed 1332453 for adding [must_use].
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
Attachment #8828454 - Flags: review?(catalin.badea392) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77034223b8a6
Make ServiceWorkerClients check the return value of OpenWindow2(). r=catalinb
https://hg.mozilla.org/mozilla-central/rev/77034223b8a6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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
Attachment #8828454 - Flags: approval-mozilla-beta?
Attachment #8828454 - Flags: approval-mozilla-aurora?
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+
Attachment #8828454 - Flags: approval-mozilla-beta?
Attachment #8828454 - Flags: approval-mozilla-beta+
Attachment #8828454 - Flags: approval-mozilla-aurora?
Attachment #8828454 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.