crash in mozilla::layers::RotatedContentBuffer::BeginPaint(mozilla::layers::PaintedLayer*, unsigned int)

VERIFIED FIXED in Firefox 36

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: JasnaPaka, Assigned: milan, NeedInfo)

Tracking

(Blocks 1 bug, {crash, crashreportid})

unspecified
mozilla38
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox36+ verified, firefox37 verified, firefox38 verified)

Details

(crash signature, )

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

5 years ago
This bug was filed from the Socorro interface and is 
report bp-ded58434-e081-476f-84ab-09ce52141013.
=============================================================
Random crash on Facebook.

Updated

5 years ago
Severity: normal → critical
Component: Layout → Graphics: Layers
Keywords: crashreportid

Comment 1

5 years ago
[Tracking Requested - why for this release]:

This signature seems to have roughly increased by a factor 5 or 6 when 36 landed on Beta (250-300 crashes per million ADI) compared to where it was in 35 (~50 crashes / 1M ADI).

In early 36.0b4 data, it's ~1.4% of all browser crashes.

See https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Alayers%3A%3ARotatedContentBuffer%3A%3ABeginPaint%28mozilla%3A%3Alayers%3A%3APaintedLayer%2A%2C%20unsigned%20int%29 for more info.
Matt, do you think you could help on this? Thanks
Flags: needinfo?(matt.woodrow)
I'm a bit busy with media stuff at the moment, Milan do you have anyone that can look at this?

The crash reports shows a bunch d2d/d3d allocation failures with the error code 0x8007000e (Out of memory or system resources).

The crash is because we failed to allocate destDTBufferOnWhite, and we don't check for this anywhere. We do however check for allocation failures of destDTBuffer and avoid crashing there. Fixing that and avoiding this particular crash is an easy fix.

The fact that this has spiked is more worrying, since it suggests that we're using more memory in general. Fixing this particular crash might just move crashes to a different callsite, or in the best case result in broken rendering.

I'm not sure what would have changed in 36 to cause this, is that the first version with d2d 1.1?
Flags: needinfo?(matt.woodrow) → needinfo?(milan)
I'll follow up.  Yes, D2D1.1 is in 36, but some of the reports are "just" D2D, and don't have the failed allocations.  We've also run into scenarios where the out of memory message comes from calls that shouldn't even be able to return them, so it'll be interesting.
This is admittedly a wallpaper patch, but if it stops the crash, it may be worth it.
Flags: needinfo?(milan)
Attachment #8557156 - Flags: review?(matt.woodrow)
Let's get more information when we fail.
Attachment #8557156 - Attachment is obsolete: true
Attachment #8557156 - Flags: review?(matt.woodrow)
Attachment #8557175 - Flags: review?(matt.woodrow)
Comment on attachment 8557175 [details] [diff] [review]
(wallpaper) patch to check null pointer. r=mattwoodrow

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

::: gfx/layers/RotatedBuffer.cpp
@@ +648,5 @@
>              destBufferRect = ComputeBufferRect(neededRegion.GetBounds());
>              CreateBuffer(result.mContentType, destBufferRect, bufferFlags,
>                           &destDTBuffer, &destDTBufferOnWhite);
> +            // Continue if we're missing buffer on white, but return if
> +            // we're missing the destination buffer.

We should return here if we don't have the buffer on white, there's not much point in drawing the other one without it.
Crash Signature: [@ mozilla::layers::RotatedContentBuffer::BeginPaint(mozilla::layers::PaintedLayer*, unsigned int)] → [@ mozilla::layers::RotatedContentBuffer::BeginPaint(mozilla::layers::PaintedLayer*, unsigned int)] [@ xul.dll@0x6adfa0 | xul.dll@0x255e34 | xul.dll@0x3b8d7c | xul.dll@0x21ef27b | xul.dll@0x3c0003 | xul.dll@0x3c0338 | xul.dll@0x36a379 | xul.dll@0x36a21b …
Blocks: MSE
With review comments.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a61d323e4a6
Attachment #8557175 - Attachment is obsolete: true
Attachment #8557175 - Flags: review?(matt.woodrow)
Attachment #8558125 - Flags: review?(matt.woodrow)
Comment on attachment 8558125 [details] [diff] [review]
(wallpaper) patch to check null pointer. r=mattwoodrow

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

I wouldn't bother with MOZ_UNLIKELY, branch prediction should handle this fine and be simpler to read.
Attachment #8558125 - Flags: review?(matt.woodrow) → review+
Without MOZ_UNLIKELY, claiming the try run is still valid.
Attachment #8558125 - Attachment is obsolete: true
Attachment #8558172 - Flags: review+

Comment 11

4 years ago
I was reviewing this bug and looking at how I could get our outsource graphics folk to help reproduce this in their second run and I noticed something. You note above that it is a memory related crash. This crash did not spike with 36b1 or 36b2. It spiked with 36b3. And we landed the MSE support which has known memory issues in 36b3. 

There is nothing video-specific about the URLs though, except that they are avenues to video but they are also avenues to images, and games etc: It's mostly a facebook crash. But it also happens on twitter and youtube.

Most of the comments do not state anything about watching video either. But what we are seeing is that we leak memory while watching video. So, it would make sense to crash later in some unrelated area of intensive processing after the video watching has increased our memory footprint.

This ^ at least is my theory.

Sheila & Marcia, I think we should track this with the memory related issues in MSE and see if it's rate of occurrence decreases as those fixes roll out. Sheila NI'ing you to add this to your tracking  please.
Flags: needinfo?(smooney)
Assignee: nobody → milan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6105b754be10
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8558172 [details] [diff] [review]
(wallpaper) patch to check null pointer. Carry r=mattwoodrow

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: This is 1.4% of crashes on Beta.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: We're avoiding dereferencing a null pointer, so it's possible we'll crash elsewhere.
[String/UUID change made/needed]: n/a
Attachment #8558172 - Flags: approval-mozilla-beta?
Attachment #8558172 - Flags: approval-mozilla-aurora?
Attachment #8558172 - Flags: approval-mozilla-beta?
Attachment #8558172 - Flags: approval-mozilla-beta+
Attachment #8558172 - Flags: approval-mozilla-aurora?
Attachment #8558172 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
Socorro stats over the past 4 weeks show:
[1] - [@ mozilla::layers::RotatedContentBuffer::BeginPaint(mozilla::layers::PaintedLayer*, unsigned int)]
* Firefox 36 - 1 crash in Beta 9 + 2 crashes in RC (build1)
* Firefox 37 - 0 crashes in builds after February 4th
* Firefox 38 - 1 crash in builds after February 3rd
[2] - [@ xul.dll@0x6adfa0 | xul.dll@0x255e34 | xul.dll@0x3b8d7c | xul.dll@0x21ef27b | xul.dll@0x3c0003...]
* all reported crashes are on Firefox 36 Beta 5

[1] - https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=28&signature=mozilla%3A%3Alayers%3A%3ARotatedContentBuffer%3A%3ABeginPaint%28mozilla%3A%3Alayers%3A%3APaintedLayer%2A%2C+unsigned+int%29#tab-reports
[2] - https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=28&signature=xul.dll%400x6adfa0+%7C+xul.dll%400x255e34+%7C+xul.dll%400x3b8d7c+%7C+xul.dll%400x21ef27b+%7C+xul.dll%400x3c0003+%7C+xul.dll%400x3c0338+%7C+xul.dll%400x36a379+%7C+xul.dll%400x36a21b+%7C+xul.dll%400x22b349+%7C+xul.dll%400x3c12b3+%7C+xul.dll%400x9bc74#tab-sigsummary
You need to log in before you can comment on or make changes to this bug.