1323777 - Crash in GetDisplayMode

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 3

[Mats Palmgren (inactive)]

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

[Mats Palmgren (inactive)]

Attachment: part 2 - make PresShell::Destroy() return immediately if it's already been called.

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.

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 3 - Ensure that collecting telemetry on user font usage don't have unwanted side effects.

Comment 6

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3017c709fb575e44737752786a86f9eb51879b38&exclusion_profile=false

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8819438 [details] [diff] [review]
part 2 - make PresShell::Destroy() return immediately if it's already been called.

Should be fine

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/581720c25bf0
https://hg.mozilla.org/mozilla-central/rev/65fb78694c3a
https://hg.mozilla.org/mozilla-central/rev/021414da3a4c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you feel that it's sufficiently baked.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8818966 [details] [diff] [review]
patch

Fix a crash. Aurora53+.

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8818966 [details] [diff] [review]
patch

let's get this into 52 beta as well

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

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