873378 - Crash Report [@ gfxImageSurface::CopyFrom(gfxImageSurface*) ]

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Paul Feher]

This bug was filed from the Socorro interface and is 
report bp-f8c343c1-8e20-41c2-82cc-d079f2130517 .
============================================================= 
Aurora 23.0a2 2013-05-16
LG Nexus 4 (Android 4.2.2)

Steps to reproduce:
1) Go to http://people.mozilla.com/~nhirata/html_tp/accel_bug615597.html
2) Rotate the device several times portrait/landscape

Crash stack:
0 	libc.so 	libc.so@0x18688 	
1 	libxul.so 	gfxImageSurface::CopyFrom 	gfx/thebes/gfxImageSurface.cpp:289
2 	libxul.so 	gfxReusableSurfaceWrapper::GetWritable 	gfxReusableSurfaceWrapper.cpp:67
3 	libxul.so 	mozilla::layers::TextureClientTile::LockImageSurface 	gfx/layers/client/TextureClient.cpp:217
4 	libxul.so 	mozilla::layers::TextureClientTile::LockImageSurface 	gfx/layers/client/TextureClient.cpp:215
5 	libxul.so 	mozilla::layers::BasicTiledLayerBuffer::ValidateTileInternal 	gfx/layers/client/TiledContentClient.cpp:213
6 	libxul.so 	mozilla::FrameLayerBuilder::DrawThebesLayer 	layout/base/FrameLayerBuilder.cpp:3324
7 		@0x6de10ffe 	
8 	libxul.so 	nsRefPtr<mozilla::layers::ImageContainer>::assign_assuming_AddRef 	obj-firefox/dist/include/mozilla/mozalloc.h:225
9 	libmozglue.so 	arena_dalloc 	jemalloc.c:4675
10 	libxul.so 	nsRegion::SetToElements 	nsRegion.cpp:278
11 	libxul.so 	nsRegion::Copy 	nsRegion.cpp:606
12 	libxul.so 	nsRegion::And 	nsRegion.cpp:732
13 	libxul.so 	mozilla::layers::BasicTiledLayerBuffer::ValidateTile 	gfx/layers/client/TiledContentClient.cpp:268
14 	libxul.so 	nsRegion::SetToElements 	nsRegion.cpp:278
15 	libxul.so 	mozilla::layers::TiledLayerBuffer<mozilla::layers::BasicTiledLayerBuffer, mozill 	gfx/layers/TiledLayerBuffer.h:416

Comment 2

[Aaron Train [:aaronmt]]

100% reproducible visiting this URL in Nightly for Android (06/19)

http://kieranhealy.org/philcites/

--
LG Nexus 4 (Android 4.2.2)
HTC One (Android 4.1)

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

Since this is reproducible is this worth having an owner?

Comment 4

[Joe Drew (not getting mail)]

*sigh* that is not what I wanted to do, bugzilla.

Comment 5

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

Let me make sure we have a device, then I'll reassign.

Comment 6

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

BenWa, can you take a look at this? I reproduced it on Nexus 4 Kats has using the Comment 2 STR.

Comment 7

[Benoit Girard (:BenWa)]

Just a quick update since it's been two weeks. I have an environment and I'm starting to work on this issue.

Comment 8

[Benoit Girard (:BenWa)]

(In reply to Aaron Train [:aaronmt] from comment #2)
> 100% reproducible visiting this URL in Nightly for Android (06/19)
> 
> http://kieranhealy.org/philcites/
> 
> --
> LG Nexus 4 (Android 4.2.2)
> HTC One (Android 4.1)

I'm seeing a crash with this spammed:
W/Adreno200-GSL(21135): <sharedmem_gpumem_alloc:991>: sharedmem_gpumem_alloc: mmap failed errno 12 Out of memory

Looks like I'll have to debug that first.

Comment 9

[Benoit Girard (:BenWa)]

Some progress on the real bug now that Comment 8 went away for now.

It seems like the write lock is set on all the tiles. It's most likely not reset correctly because we should only be locking one tile at a time. This forces us to copy the entire screen leading to an allocation failing and SIGSEGV.

Comment 10

[Benoit Girard (:BenWa)]

Alright my previous guess was wrong. The problem is that we allocate 200 layers of size 512^2 which is about 200MB so we OOM.

I think we need to have a cap on our layers after which we just flatten everything.

Comment 11

[Benoit Girard (:BenWa)]

Attachment: Enforce a max of 20 layers

I'll be testing this out tomorrow

Comment 12

[Benoit Girard (:BenWa)]

Attachment: Enforce a max of 20 layers on mobile

Comment 13

[Benoit Girard (:BenWa)]

Comment on attachment 775711 [details] [diff] [review]
Enforce a max of 20 layers on mobile

Asking roc instead to review this to get another opinion.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 775711 [details] [diff] [review]
Enforce a max of 20 layers on mobile

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

This is actually restricting the maximum number of active, non-static-content child layers any layer can have. That's fine, but you should document that more carefully. Also this pref should be add explicitly to all.js with the value -1, and its behavior documented in a comment there.

Comment 15

[Benoit Girard (:BenWa)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> Comment on attachment 775711 [details] [diff] [review]
> Enforce a max of 20 layers on mobile
> 
> Review of attachment 775711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is actually restricting the maximum number of active,
> non-static-content child layers any layer can have.

Ohh right, matt pointed that out but I forgot to fix it. Either will fix this bug but I imagine a global max-layer per tree is simpler to understand and more useful to have. I'll change it to that unless you object.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I actually think your current behavior is better and easier to understand.

Comment 17

[Benoit Girard (:BenWa)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> I actually think your current behavior is better and easier to understand.

It's not what I'd expect when setting layers.max-active and I think we can miss cases where do you want to restrict the layer count. But I'm fine with landing this patch and changing this later as needed.

Comment 18

[Benoit Girard (:BenWa)]

Attachment: patch

Added pref to all.js

Comment 19

[Benoit Girard (:BenWa)]

Attachment: patch - ready to land. r=roc

Tree is closed.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c89002460da

Comment 21

[Benoit Girard (:BenWa)]

Bustage fix for unexpected tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5efda3c30be3

I didn't push to try because I didn't think we really had tests with 20 layers. We should consider forcing the page to render in a single thebes and seeing which tests pass/fail unexpectedly.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c89002460da
https://hg.mozilla.org/mozilla-central/rev/5efda3c30be3

Comment 23

[Markus Stange [:mstange]]

+/* static */ int32_t
+nsDisplayItem::MaxActiveLayers()
+{
+  static int32_t sMaxLayers = false;
+  static bool sMaxLayersCached = false;
+
+  if (!sMaxLayersCached) {
+    Preferences::AddIntVarCache(&sMaxLayers, "layers.max-active", -1);
+    sMaxLayersCached = true;
+  }
+
+  return sMaxLayersCached;
+}
                    ^^^^^^ !!

This effectively returns 1 every time.

Comment 24

[Benoit Girard (:BenWa)]

AAhh thanks! I knew it was something silly about this pref not working causing these regressions.

Comment 25

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

Once it did start returning 20 on mobile, it started failing reftest-2: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=25623153&tree=Mozilla-Inbound, possibly because of the filtering at the edges.  The test and the reference result end up with different layers tree and I'm guessing that's where the differences come from.  Maybe - it would explain why 1 worked, and 20 does not (with 1, the layer trees are the same), but the trees are also different with the old code, yet the test passed.  Perhaps something changed in the layerization algorithm during the three days we had this code in?

Comment 26

[Matt Woodrow (:mattwoodrow)]

Milan, do you have those layer tree dumps handy?

In particular being able to compare the one with a 20 limit, vs no-limit would be useful.

Comment 27

[Benoit Girard (:BenWa)]

we likely want to blackout my bustage fix which was incorrect.

(In reply to Benoit Girard (:BenWa) from comment #21)
> Bustage fix for unexpected tests:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5efda3c30be3

Comment 28

[Dave Hylands [:dhylands]]

This changeset causes bug 897239 (crash when trying to open bluetooth or wifi in FirefoxOS settings app).

Comment 29

[Dave Hylands [:dhylands]]

Comment on attachment 777947 [details] [diff] [review]
patch - ready to land. r=roc

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

::: layout/base/nsDisplayList.cpp
@@ +1507,5 @@
> +    Preferences::AddIntVarCache(&sMaxLayers, "layers.max-active", -1);
> +    sMaxLayersCached = true;
> +  }
> +
> +  return sMaxLayersCached;

Shouldn't this be sMaxLayers ?

Comment 30

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

Right - see bug 896250 that fixed that.  There are regressions in Android tests with the "proper" fix though, so that got backed out.
with the layer limit set to 1: https://tbpl.mozilla.org/?tree=Try&rev=32105e7ca6ee
with the layer limit set to -1: https://tbpl.mozilla.org/?tree=Try&rev=6e0879b39110
I believe we get the same result with the layer limit set to 20, but I don't have a try run for that.
It's just that we never actually tested this fix, because of the typo, we were setting the layer limit to 1, rather than 20.

Comment 31

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> Milan, do you have those layer tree dumps handy?
> 
> In particular being able to compare the one with a 20 limit, vs no-limit
> would be useful.

Not handy, and I'm in two days of training/meetings so it will be a couple of days before I get them back.  I do have a few images, I will attach those.

Comment 32

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

Attachment: LayersAtMaxSetTo1.png

Comment 33

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

Attachment: LayersMaxSetToAtMinus1Or20.png

Comment 34

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

Attachment: RefTestDifferences-4pixels.png

Comment 35

[Dave Hylands [:dhylands]]

I landed a patch to fix MaxActiveLayers in bug 897239 so I'll reclose this bug.

Comment 36

[Dave Hylands [:dhylands]]

Can we revert this patchset?

Its screwing up windows and b2g.

Comment 37

[David Zbarsky (:dzbarsky)]

I'm sorry, I backed this out for causing bug 897239 and the other regressions.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0a9de949678

Comment 38

[Daniel Holbert [:dholbert]]

Looks like this backout is causing Android reftest orange, because we didn't yet back out the other cset from this bug, from comment 21.

Comment 39

[Daniel Holbert [:dholbert]]

I backed out comment 21, so now this should be "fully backed out" (conceptually), and the long stream of R2 orange should now turn green: 
 https://hg.mozilla.org/integration/mozilla-inbound/rev/de06fec529a5

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Looks like the backout of this is causing the same kinds of asserts we were getting the last time bug 896250 attempted to land. Inbound's closed until this is resolved.

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I filed bug 898713 to track fixing the orange resulting from this backout.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It seems like any relanding of this should include, at minimum:
https://hg.mozilla.org/mozilla-central/rev/4c89002460da
https://hg.mozilla.org/mozilla-central/rev/da5fa63e90cb
though I'm not confident that the second covers all the regressions from the first.

(I'd expect it to *not* need to include https://hg.mozilla.org/mozilla-central/rev/5efda3c30be3 .)

Comment 43

[Benoit Girard (:BenWa)]

Thanks for the help on this bug. I spent the weekend trapped in an airports moving from flight to flight. It took 24 hours for American airlines to get me from LAX to YYZ from an initially direct flight.

Comment 44

[Benoit Girard (:BenWa)]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=9ac8f21619bc

Comment 45

[Ryan VanderMeulen [:RyanVM]]

We managed to hit the asserts from bug 898713 that were keeping the tree closed today after pushing bug 777196 to b2g-inbound. I'll point out that it was green on Try. Just as with bug 887868, it seems unlikely to have directly been at fault. Any idea what underlying issue is happening here?

https://tbpl.mozilla.org/php/getParsedLog.php?id=26032309&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=26032281&tree=B2g-Inbound

Comment 46

[Benoit Girard (:BenWa)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #45)
> We managed to hit the asserts from bug 898713 that were keeping the tree
> closed today after pushing bug 777196 to b2g-inbound. I'll point out that it
> was green on Try. Just as with bug 887868, it seems unlikely to have
> directly been at fault. Any idea what underlying issue is happening here?
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=26032309&tree=B2g-Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=26032281&tree=B2g-Inbound

I don't know what the problem is but it looks like we have a bug that isn't triggered right now but changing the code sufficiently will trigger it which is at least what this patch did. This should be investigated in a new bug.

Comment 47

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5319d82251e

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b5319d82251e