1175521 - crash in RtlEnterCriticalSection | MessageLoop::PostTask_Helper(tracked_objects::Location const&, Task*, int, bool)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Tracy Walker [:tracy]]

This bug was filed from the Socorro interface and is 
report bp-0ee12a3a-100a-41d9-a463-526152150616.
=============================================================

[Tracking Requested - why for this release]:

This has been creeping up the topcrash list on Nightly.  Currently #7 at 67 crashed in the past seven days.  It is also on 38, 39 and 40. But highest in volume on 40 and 41.

The only reporter comment is: "Heavy duty imgur.com browsing with animated images/html5 videos. started to use 30% processor. tried to shut down browser. This caused crash."

Comment 1

[Benjamin Smedberg]

Shutdown crash: compositor appears to be trying to post something to a dead message loop.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

I'll track this for 39+ but it seems unlikely we can land a fix in time for 39 release.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Can we tell if it crept up because we took care of crashes above it, or are the overall numbers trending up as well?

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Check if the loop is there before proceeding. r=nical

This is a wallpaper patch, but we can see if it helps nightly.  Should still figure out if it's expected for the compositor loop to be null.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Can we tell if this was there before 33?

Comment 6

[Tracy Walker [:tracy]]

(In reply to Milan Sreckovic [:milan] from comment #3)
> Can we tell if it crept up because we took care of crashes above it, or are
> the overall numbers trending up as well?

I think it's more of the former.  But the numbers have been up a little more consistently (for Nightly and DevEd) beginning 20150604. 

 (In reply to Milan Sreckovic [:milan] from comment #5)
> Can we tell if this was there before 33?

There is a single crash reported on 9.0.1 https://crash-stats.mozilla.com/report/index/2dd5c293-c18d-4adf-9ae3-3e31c2150612.  All of the rest are 38.0 or later.

Comment 7

[Nicolas Silva [:nical]]

Comment on attachment 8623775 [details] [diff] [review]
Check if the loop is there before proceeding. r=nical

Review of attachment 8623775 [details] [diff] [review]:
-----------------------------------------------------------------

It's fine to paper over this, but there are some things that deserve further fixing in here.
Looks like we are getting in an invalid state in the middle of shutting down, and the error code is trying to use resources that are probably already gone (like the event loop). I think that the right thing to do, since various things can trigger posting to messages loops or other resources that are destroyed during shutdown, is to figure out if all of these things are happening on the same thread and if so maybe keep a state somewhere to have the "dangerous" things check that we are not shutting down and cancel/assert whatever they are about to do if it is the case (except things that are supposed to be hapenning that during shutdown, obviously).

This also means that the crash we are looking at is the symptom of the real issue: whatever caused us do get into MessageChannel::NotifyMaybeChannelError() and the cascade of OnChannelError that follows.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Sounds like a plan - let's land the wallpaper patch (no try run - let me know if you'd like one), and leave open and assigned to :nical for an investigation and a real fix.  This will still assert in debug and drop a message in any crash report if it happens.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Topcrash

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36c731ffdb2f

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36c731ffdb2f

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Milan Sreckovic [:milan] from comment #8)
> Sounds like a plan - let's land the wallpaper patch (no try run - let me
> know if you'd like one), and leave open and assigned to :nical for an
> investigation and a real fix.  This will still assert in debug and drop a
> message in any crash report if it happens.

Just for book keeping, can we close this out and do the investigation in a follow up?

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Assuming the crashes go away, sure.  I'll close this one, we can reopen if the crashes aren't gone, and clone for a followup.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Milan, can we have an uplift request to aurora? Thanks

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8623775 [details] [diff] [review]
Check if the loop is there before proceeding. r=nical

Approval Request Comment
[Feature/regressing bug #]: Seems to be 38 and later
[User impact if declined]: Topcrash
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: No crashes on nightly since this landed.
[String/UUID change made/needed]:

Comment 16

[Kate Glazko]

Comment on attachment 8623775 [details] [diff] [review]
Check if the loop is there before proceeding. r=nical

Approving for uplift to Aurora, low risk, it has been in Nightly for approximately a week.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8623775 [details] [diff] [review]
Check if the loop is there before proceeding. r=nical

[Triage Comment]
Sorry, we meant beta.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/50c5e2844aeb

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Since this was a topcrash, let's track for upcoming ESR38 release.

Milan, do you think we should be fixing this (backporting?) for ESR38 release?

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Not a security problem, I don't think it would get approved for an ESR release, though the patch is simple enough.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Looking at the past week, the only crashes that would have been this one instead, are the shutdown hangs like bug 1127270.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Milan, if the backport is simple and helps rather than hurts our end-users I think we should consider taking it. You might be the best judge as to how safe the backport fix would be. Thanks!

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8623775 [details] [diff] [review]
Check if the loop is there before proceeding. r=nical

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: top crash
User impact if declined: more crashes?
Fix Landed on Version: 41, uplifted to 40
Risk to taking this patch (and alternatives if risky): low risk, we are stopping a nullptr dereference.
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8623775 [details] [diff] [review]
Check if the loop is there before proceeding. r=nical

I agree with Ritu that we should take this topcrash fix in ESR38 as stability has noticeably improved in 39 and 40. ESR38+

Ritu - Can you please add this to the relnotes for ESR 38.2.0.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/df846dc43142

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Done.

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Forgot to add that this bug will be mentioned in ESR 38.2.0 release notes.