Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: smaug)

Tracking

({crash})

unspecified
mozilla54
Unspecified
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed, firefox54 fixed)

Details

(crash signature)

Attachments

(3 attachments)

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.
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: nobody → bugs
Posted patch patchSplinter Review
Attachment #8818966 - Flags: review?(bdahl)
Blocks: 1104916
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...
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 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+
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+
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
Attachment #8818966 - Flags: review?(bdahl) → review+
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
Please request Aurora/Beta approval on this when you feel that it's sufficiently baked.
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?
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?
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?
Attachment #8819439 - Flags: approval-mozilla-release? → approval-mozilla-aurora?
Comment on attachment 8818966 [details] [diff] [review]
patch

Fix a crash. Aurora53+.
Attachment #8818966 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8819438 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8819439 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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+
Attachment #8819438 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8819439 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.