Closed Bug 1247380 Opened 4 years ago Closed 4 years ago

45.0b4 crash spike in gfxContext::PushGroupAndCopyBackground at address 0x0

Categories

(Core :: Graphics, defect, critical)

x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
e10s ? ---
firefox44 --- affected
firefox45 + fixed
firefox46 --- unaffected
firefox47 --- fixed

People

(Reporter: kairo, Assigned: bas.schouten)

References

Details

(Keywords: crash, crashreportid, Whiteboard: gfx-noted)

Crash Data

Attachments

(1 file)

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-84a86876-d189-4320-af90-091ce2160210.
=============================================================

Stack Trace:
0 	xul.dll 	gfxContext::PushGroupAndCopyBackground(gfxContentType, float, mozilla::gfx::SourceSurface*, mozilla::gfx::Matrix const&) 	gfx/thebes/gfxContext.cpp
1 	xul.dll 	mozilla::layers::BasicLayerManager::PushGroupForLayer(gfxContext*, mozilla::layers::Layer*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&) 	gfx/layers/basic/BasicLayerManager.cpp

All those stacks are only those two frames, and have an address of 0x0.

This crash signature spiked in 45.0b4 and is 6.8% of total crashes (and 4.8% of non-e10s crashes) - while it was only 1.1% of non-e10s crashes in 45.0b3.
Bas, is this related to your push group / pop group stuff?
Flags: needinfo?(bas)
Whiteboard: gfx-noted,crash
The spike isn't limited to b4, b3(/2/1) shows the same spike. 
e10s hit with tipple crashes compared to non-e10s for this.
(In reply to Mason Chang [:mchang] from comment #1)
> Bas, is this related to your push group / pop group stuff?

No, I don't think it is, I'll have a patch up here soon though.
Flags: needinfo?(bas)
This fix is a little speculative as I don't know what causes this and I can't reproduce it locally, by guess would be something like a device loss. But even if it's something else I'm fine with losing subpixel AA instead of crashing. This implements that. On trunk this bug is superceeded by native pushlayer. I'll prepare a patch for beta and request uplift once this has been r+'ed.
Assignee: nobody → bas
Status: NEW → ASSIGNED
Keywords: crashreportid
Whiteboard: gfx-noted,crash → gfx-noted
Tracking for 46 since this is a top crash. It doesn't appear to affect 46 or 47.
Comment on attachment 8718301 [details]
MozReview Request: Bug 1247380: Only copy the background if we can succesfully get a snapshot. r=jrmuizel

As dvander points out on IRC Snapshot() doesn't seem to have the ability to return null.
Attachment #8718301 - Flags: review?(jmuizelaar) → review-
Comment on attachment 8718301 [details]
MozReview Request: Bug 1247380: Only copy the background if we can succesfully get a snapshot. r=jrmuizel

Cairo does. We've dealt with a bunch of cairo surface state errors in the past (one of them was when using certain fonts for some reason iirc). This is presumably why this crash signature spiked, we've made a number of places inside the Cairo Moz2D code return nullptr more aggressively when there's state issues recently.
Attachment #8718301 - Flags: review- → review?(jmuizelaar)
To make people happier with some additional information, I believe this was a regression from http://hg.mozilla.org/mozilla-central/rev/6c49bb716593. Before that change CreateSimilarDT would return a nullptr when the surface was in an error state, this would cause us to crash in gfxContext::PushNewDT.
Comment on attachment 8718301 [details]
MozReview Request: Bug 1247380: Only copy the background if we can succesfully get a snapshot. r=jrmuizel

https://reviewboard.mozilla.org/r/34495/#review31275
Attachment #8718301 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8718301 [details]
MozReview Request: Bug 1247380: Only copy the background if we can succesfully get a snapshot. r=jrmuizel

Approval Request Comment
[Feature/regressing bug #]: Technically not a regression, changed stack from a fix for the PushNewDT crash.
[User impact if declined]: More crashes.
[Describe test coverage new/current, TreeHerder]: None, since this code is no longer used on 46 or higher
[Risks and why]: Very low, just a nullcheck
[String/UUID change made/needed]: None
Attachment #8718301 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/b2b9363f3edc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
control-no-addons 10.94%, experiment-no-addons 33.77%. (low samples, experiment just started.)
Hints to me that it is possibly an advert.
Comment on attachment 8718301 [details]
MozReview Request: Bug 1247380: Only copy the background if we can succesfully get a snapshot. r=jrmuizel

Fix a top crash, taking it.
Should be in 45 beta 6.
Attachment #8718301 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
has problems with beta uplift:

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r b2b9363f3edc
grafting 328530:b2b9363f3edc "Bug 1247380: Only copy the background if we can succesfully get a snapshot. r=jrmuizel"
merging gfx/thebes/gfxContext.cpp
warning: conflicts while merging gfx/thebes/gfxContext.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Flags: needinfo?(bas)
This is also the #1 Top Crash Score in 44.0.2 release.
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #17)
> This is also the #1 Top Crash Score in 44.0.2 release.

Hrm, it seems the cause of this got uplifted in bug 1107792. In theory this just 'moves' a crash to another signature rather than causing a new one though. I'll land this patch on beta now. If it works it could be worth uplifting if there is going to be another dot release. But it should be noted this is not a regression.
Flags: needinfo?(bas)
(In reply to Bas Schouten (:bas.schouten) from comment #19)
> https://hg.mozilla.org/releases/mozilla-beta/rev/01b9ef7f3dce

setting flags
Blocks: e10s-crashes
You need to log in before you can comment on or make changes to this bug.