Closed
Bug 1331619
Opened 8 years ago
Closed 8 years ago
Crash when clicking Push Notification while only private window is opened @ nsContentUtils::DispatchFocusChromeEvent
Categories
(Core :: DOM: Service Workers, defect)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: arai, Assigned: bkelly)
References
Details
(Keywords: crash)
Attachments
(1 file)
(deleted),
patch
|
catalinb
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
Ben: does this look like your area? If not can you think of someone else?
Flags: needinfo?(bkelly)
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
status-firefox50:
--- → wontfix
status-firefox51:
--- → wontfix
status-firefox52:
--- → affected
status-firefox-esr45:
--- → disabled
Assignee | ||
Comment 4•8 years ago
|
||
Seems this code was added in bug 1172870.
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8828454 -
Flags: review?(catalin.badea392)
Updated•8 years ago
|
Group: core-security
Updated•8 years ago
|
Component: DOM: Push Notifications → DOM: Service Workers
Assignee | ||
Updated•8 years ago
|
Blocks: 1172870, ServiceWorkers-stability
Comment 6•8 years ago
|
||
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.)
Assignee | ||
Comment 7•8 years ago
|
||
(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•8 years ago
|
||
I filed 1332453 for adding [must_use].
Comment 9•8 years ago
|
||
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+
Comment 10•8 years ago
|
||
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77034223b8a6
Make ServiceWorkerClients check the return value of OpenWindow2(). r=catalinb
Comment 11•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 12•8 years ago
|
||
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 13•8 years ago
|
||
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+
Assignee | ||
Comment 14•8 years ago
|
||
Updated•8 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•