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

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Pavel Cvrcek [:JasnaPaka]]

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

Comment 1

[Robert Kaiser]

[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.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Matt, do you think you could help on this? Thanks

Comment 3

[Matt Woodrow (:mattwoodrow)]

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?

Comment 4

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

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.

Comment 5

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

Attachment: (wallpaper) patch to check null pointer. r=mattwoodrow

This is admittedly a wallpaper patch, but if it stops the crash, it may be worth it.

Comment 6

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

Attachment: (wallpaper) patch to check null pointer. r=mattwoodrow

Let's get more information when we fail.

Comment 7

[Matt Woodrow (:mattwoodrow)]

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.

Comment 8

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

Attachment: (wallpaper) patch to check null pointer. r=mattwoodrow

With review comments.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a61d323e4a6

Comment 9

[Matt Woodrow (:mattwoodrow)]

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.

Comment 10

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

Attachment: (wallpaper) patch to check null pointer. Carry r=mattwoodrow

Without MOZ_UNLIKELY, claiming the try run is still valid.

Comment 11

[cmtalbert]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6105b754be10

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6105b754be10

Comment 14

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

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a988d848e759
https://hg.mozilla.org/releases/mozilla-beta/rev/8f52ac825ba7

Comment 16

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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