Closed
Bug 1323777
Opened 6 years ago
Closed 6 years ago
Crash in GetDisplayMode
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: mccr8, Assigned: smaug)
References
Details
(Keywords: crash)
Crash Data
Attachments
(3 files)
1.10 KB,
patch
|
bdahl
:
review+
gchang
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.38 KB,
patch
|
smaug
:
review+
gchang
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
5.07 KB,
patch
|
smaug
:
review+
gchang
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-cbb37d24-7307-4d60-a9df-2491d2161213. ============================================================= There are about 160 of these crashes, almost all on Android, and these are near-null crashes. I looked at a half-dozen, and they were all inside nsMediaQuery::Matches, being called somewhere from PresShell::Destroy. Maybe we're accessing something that has gone away? Unfortunately there are no useful line numbers in GetDisplayMode. The first line of GetDisplayMode is this: nsCOMPtr<nsISupports> container = aPresContext->GetRootPresContext()-> Document()->GetContainer(); That seems like it could go badly if we're in PresShell::Destroy.
Assignee | ||
Comment 1•6 years ago
|
||
Doesn't help that layout uses non-Get* prefix naming convention differently to DOM. In layout code non-Get* may return often null after some sort of destroy. So like in this case, if root prescontext has been unlinked, Document() returns null.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bugs
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8818966 -
Flags: review?(bdahl)
Comment 3•6 years ago
|
||
I think it's unacceptable that collecting telemetry on user font usage has side effects like this (i.e. calling FlushUserFontSet()). So I'd like to make additional changes to prevent that from occurring...
Comment 4•6 years ago
|
||
First, there is no reason to run this stuff more than once. mHaveShutDown is set to true at the end of PresShell::Destroy() and I think it was intended to make additional calls NOPs.
Attachment #8819438 -
Flags: review?(bugs)
Comment 5•6 years ago
|
||
Attachment #8819439 -
Flags: review?(bugs)
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3017c709fb575e44737752786a86f9eb51879b38&exclusion_profile=false
Assignee | ||
Comment 7•6 years ago
|
||
Comment on attachment 8819438 [details] [diff] [review] part 2 - make PresShell::Destroy() return immediately if it's already been called. Should be fine
Attachment #8819438 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 8•6 years ago
|
||
Comment on attachment 8819439 [details] [diff] [review] part 3 - Ensure that collecting telemetry on user font usage don't have unwanted side effects. yeah, I think we want this.
Attachment #8819439 -
Flags: review?(bugs) → review+
Comment 9•6 years ago
|
||
Crash volume for signature 'GetDisplayMode': - nightly (version 53): 0 crashes from 2016-11-14. - aurora (version 52): 14 crashes from 2016-11-14. - beta (version 51): 724 crashes from 2016-11-14. - release (version 50): 332 crashes from 2016-11-01. - esr (version 45): 0 crashes from 2016-07-06. Crash volume on the last weeks (Week N is from 01-02 to 01-08): W. N-1 W. N-2 W. N-3 W. N-4 W. N-5 W. N-6 W. N-7 - nightly 0 0 0 0 0 0 0 - aurora 2 1 3 8 0 0 0 - beta 47 59 86 115 155 162 72 - release 31 39 49 65 69 55 14 - esr 0 0 0 0 0 0 0 Affected platforms: Windows, Mac OS X, Linux Crash rank on the last 7 days: Browser Content Plugin - nightly - aurora #394 - beta #105 #3664 - release #928 #4915 - esr
Updated•6 years ago
|
Attachment #8818966 -
Flags: review?(bdahl) → review+
Comment 10•6 years ago
|
||
Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/581720c25bf0 make GetDisplayMode more null-safe, r=bdahl https://hg.mozilla.org/integration/mozilla-inbound/rev/65fb78694c3a part 2 - make PresShell::Destroy() return immediately if it's already been called. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/021414da3a4c part 3 - Ensure that collecting telemetry on user font usage don't have unwanted side effects. r=smaug
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/581720c25bf0 https://hg.mozilla.org/mozilla-central/rev/65fb78694c3a https://hg.mozilla.org/mozilla-central/rev/021414da3a4c
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 12•6 years ago
|
||
Please request Aurora/Beta approval on this when you feel that it's sufficiently baked.
status-firefox53:
--- → affected
Flags: needinfo?(bugs)
Assignee | ||
Comment 13•6 years ago
|
||
Comment on attachment 8818966 [details] [diff] [review] patch Approval Request Comment [Feature/Bug causing the regression]: bug 1104916 [User impact if declined]: crashes [Is this code covered by automated tests?]: NA [Has the fix been verified in Nightly?]: Based on crash-stat this particular crash isn't happening anymore, but there is another GetDisplayMode related issue. [Needs manual test from QE? If yes, steps to reproduce]: NA [List of other uplifts needed for the feature/fix]: NA [Is the change risky?]: no [Why is the change risky/not risky?]: null checks [String changes made/needed]: NA
Flags: needinfo?(bugs)
Attachment #8818966 -
Flags: approval-mozilla-beta?
Attachment #8818966 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 14•6 years ago
|
||
Comment on attachment 8819438 [details] [diff] [review] part 2 - make PresShell::Destroy() return immediately if it's already been called. Approval Request Comment [Feature/Bug causing the regression]: see comment 13 [Is the change risky?]: Shouldn't be risky. Telemetry specific code is handled safer way during presshell destroy
Attachment #8819438 -
Flags: approval-mozilla-beta?
Attachment #8819438 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 15•6 years ago
|
||
Comment on attachment 8819439 [details] [diff] [review] part 3 - Ensure that collecting telemetry on user font usage don't have unwanted side effects. Approval Request Comment See comment 14
Attachment #8819439 -
Flags: approval-mozilla-release?
Attachment #8819439 -
Flags: approval-mozilla-beta?
Updated•6 years ago
|
Attachment #8819439 -
Flags: approval-mozilla-release? → approval-mozilla-aurora?
Comment 16•6 years ago
|
||
Comment on attachment 8818966 [details] [diff] [review] patch Fix a crash. Aurora53+.
Attachment #8818966 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•6 years ago
|
Attachment #8819438 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•6 years ago
|
Attachment #8819439 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 17•6 years ago
|
||
Comment on attachment 8818966 [details] [diff] [review] patch let's get this into 52 beta as well
Attachment #8818966 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•6 years ago
|
Attachment #8819438 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•6 years ago
|
Attachment #8819439 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 18•6 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/bd48d1daf4c2 https://hg.mozilla.org/releases/mozilla-aurora/rev/66bd19aeb873 https://hg.mozilla.org/releases/mozilla-aurora/rev/d70c225f090a
Comment 19•6 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/405ee5de0673 https://hg.mozilla.org/releases/mozilla-beta/rev/cd797ef148c3 https://hg.mozilla.org/releases/mozilla-beta/rev/f0969e1a9c3b
You need to log in
before you can comment on or make changes to this bug.
Description
•