960378 - PROCESS-CRASH | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | application crashed [@ mozilla::gl::ReadPixelsIntoImageSurface(mozilla::gl::GLContext*, gfxImageSurface*)] on Android 4.0 Debug

Reporter

Description

•

9 years ago

Android 4.0 *Debug* tests currently only run on Cedar. We are trying to get these tests to pass so that they can be turned on on trunk.

On Cedar, on Android 4.0 Debug, mochitest-gl tests consistently fail with a crash.

https://tbpl.mozilla.org/php/getParsedLog.php?id=33063997&tree=Cedar&full=1#error0

16:34:25     INFO -  6640 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/texture-size-cube-maps.html] Test passed - 0, 0, 2, 1 should be red
16:34:25     INFO -  6641 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/texture-size-cube-maps.html] Test passed - 0, 1, 2, 1 should be yellow
16:34:25     INFO -  6642 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/texture-size-cube-maps.html] Test passed - 0, 0, 2, 1 should be yellow
16:34:25     INFO -  6643 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/texture-size-cube-maps.html] Test passed - 0, 1, 2, 1 should be magenta
16:34:25     INFO -  INFO | automation.py | Application ran for: 0:08:04.362758
16:34:25     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmphv3Ni5pidlog
16:34:25     INFO -  /data/anr/traces.txt not found
16:34:26     INFO -  mozcrash INFO | Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/cedar-android-debug/1389761500/fennec-29.0a1.en-US.android-arm.crashreporter-symbols.zip
16:34:41  WARNING -  PROCESS-CRASH | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | application crashed [@ mozilla::gl::ReadPixelsIntoImageSurface(mozilla::gl::GLContext*, gfxImageSurface*)]
16:34:41     INFO -  Crash dump filename: /tmp/tmpkgwgqj/1ea9ac52-abf1-ac60-3a8303db-410e6859.dmp
16:34:41     INFO -  Operating system: Android
16:34:41     INFO -                    0.0.0 Linux 3.2.0+ #2 SMP PREEMPT Thu Nov 29 08:06:57 EST 2012 armv7l pandaboard/pandaboard/pandaboard:4.0.4/IMM76I/5:eng/test-keys
16:34:41     INFO -  CPU: arm
16:34:41     INFO -       2 CPUs
16:34:41     INFO -  Crash reason:  SIGSEGV
16:34:41     INFO -  Crash address: 0x0
16:34:41     INFO -  Thread 13 (crashed)
16:34:41     INFO -   0  libxul.so!mozilla::gl::ReadPixelsIntoImageSurface(mozilla::gl::GLContext*, gfxImageSurface*) [GLReadTexImageHelper.cpp:9ea06b86b739 : 270 + 0x14]
16:34:41     INFO -       r4 = 0x6ac1dd40    r5 = 0x6ccde800    r6 = 0x00000000    r7 = 0x639bf4b4
16:34:41     INFO -       r8 = 0x00000010    r9 = 0x00008367   r10 = 0x000080e1    fp = 0x00000004
16:34:41     INFO -       sp = 0x5d401c98    lr = 0x61fbc89f    pc = 0x61fbce8e
16:34:41     INFO -      Found by: given as instruction pointer in context
16:34:41     INFO -   1  libxul.so!mozilla::gl::ReadScreenIntoImageSurface(mozilla::gl::GLContext*, gfxImageSurface*) [GLReadTexImageHelper.cpp:9ea06b86b739 : 479 + 0x7]
16:34:41     INFO -       r4 = 0x6ccde800    r5 = 0x6ac1dd40    r6 = 0x00000000    r7 = 0x6866be50
16:34:41     INFO -       r8 = 0x00000010    r9 = 0x63c29d0c   r10 = 0x636f9742    fp = 0x00088b88
16:34:41     INFO -       sp = 0x5d401d70    pc = 0x61fbd173
16:34:41     INFO -      Found by: call frame info
16:34:41     INFO -   2  libxul.so!mozilla::gl::SharedSurface_EGLImage::Fence() [SharedSurfaceEGL.cpp:9ea06b86b739 : 184 + 0x7]
16:34:41     INFO -       r4 = 0x6a76a390    r5 = 0x6ac1dd40    r6 = 0x00000000    r7 = 0x6866be50
16:34:41     INFO -       r8 = 0x00000010    r9 = 0x63c29d0c   r10 = 0x636f9742    fp = 0x00088b88
16:34:41     INFO -       sp = 0x5d401d90    pc = 0x61fc2a9f
16:34:41     INFO -      Found by: call frame info
16:34:41     INFO -   3  libxul.so!mozilla::gfx::SurfaceStream_TripleBuffer::SwapProducer(mozilla::gfx::SurfaceFactory*, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&) [SurfaceStream.cpp:9ea06b86b739 : 425 + 0x7]
16:34:41     INFO -       r4 = 0x69732d80    r5 = 0x68868a00    r6 = 0x6a76a398    r7 = 0x6b373218
16:34:41     INFO -       r8 = 0x6b373000    r9 = 0x620051d9   r10 = 0x697c0060    fp = 0x00000001
16:34:41     INFO -       sp = 0x5d401de8    pc = 0x61fc4427
16:34:41     INFO -      Found by: call frame info
16:34:41     INFO -   4  libxul.so!mozilla::gl::GLScreenBuffer::Swap(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&) [GLScreenBuffer.cpp:9ea06b86b739 : 425 + 0x9]
16:34:41     INFO -       r4 = 0x6a13db80    r5 = 0x00000000    r6 = 0x6a76a398    r7 = 0x6b373218
16:34:41     INFO -       r8 = 0x6b373000    r9 = 0x620051d9   r10 = 0x697c0060    fp = 0x00000001
16:34:41     INFO -       sp = 0x5d401e00    pc = 0x61fbeb99
16:34:41     INFO -      Found by: call frame info
16:34:41     INFO -   5  libxul.so!mozilla::WebGLContext::PresentScreenBuffer() [WebGLContext.cpp:9ea06b86b739 : 1189 + 0x9]
16:34:41     INFO -       r4 = 0x6dbefc00    r5 = 0x00000000    r6 = 0x6dbefc50    r7 = 0x6b373218
16:34:41     INFO -       r8 = 0x6b373000    r9 = 0x620051d9   r10 = 0x697c0060    fp = 0x00000001
16:34:41     INFO -       sp = 0x5d401e90    pc = 0x625c7e87
16:34:41     INFO -      Found by: call frame info




16:34:45     INFO -  01-15 16:33:58.226 I/GeckoDump( 2227): 6641 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/texture-size-cube-maps.html] Test passed - 0, 1, 2, 1 should be yellow
16:34:45     INFO -  01-15 16:33:58.234 I/GeckoDump( 2227): 6642 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/texture-size-cube-maps.html] Test passed - 0, 0, 2, 1 should be yellow
16:34:45     INFO -  01-15 16:33:58.234 I/GeckoDump( 2227): 6643 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/textures/texture-size-cube-maps.html] Test passed - 0, 1, 2, 1 should be magenta
16:34:45     INFO -  01-15 16:33:58.304 F/MOZ_Assert( 2227): Assertion failure: dest->Stride() == dest->Width() * destPixelSize, at ../../../gfx/gl/GLReadTexImageHelper.cpp:272
16:34:45     INFO -  01-15 16:33:59.382 F/libc    ( 2227): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1)
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290): *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290): Build fingerprint: 'pandaboard/pandaboard/pandaboard:4.0.4/IMM76I/5:eng/test-keys'
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290): pid: 2227, tid: 2247  >>> org.mozilla.fennec <<<
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290): signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 00000000
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290):  r0 00000081  r1 63422360  r2 0000007b  r3 00000000
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290):  r4 6ac1dd40  r5 6ccde800  r6 00000000  r7 639bf4b4
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290):  r8 00000010  r9 00008367  10 000080e1  fp 00000004
16:34:45     INFO -  01-15 16:33:59.812 I/DEBUG   ( 1290):  ip 400b4130  sp 5d401c98  lr 61fbc89f  pc 61fbce8e  cpsr 28000130

Geoff Brown [:gbrown]

Reporter

Comment 1

•

9 years ago

More examples:

https://tbpl.mozilla.org/php/getParsedLog.php?id=33063997&tree=Cedar&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=33064019&tree=Cedar&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=32828669&tree=Cedar&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=32789284&tree=Cedar&full=1

Geoff Brown [:gbrown]

Reporter

Updated

•

9 years ago

Blocks: 940080

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

Comment 2

•

9 years ago

CJ, we deemed this high priority to look at soon - can you get somebody to assign to this?

Flags: needinfo?(cku)

OS: Mac OS X → Android

u459114

Comment 3

•

9 years ago

(In reply to Milan Sreckovic [:milan] from comment #2)
> CJ, we deemed this high priority to look at soon - can you get somebody to
> assign to this?

Milan, we will have a Chinese New Year vacation between Jan 30th and Feb 4th. Which means we can start working on this fix after Feb 4th. If you think it's ok, please assign to me, depend on internal resource, I will find correct one to work on this.

Flags: needinfo?(cku)

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

Comment 4

•

9 years ago

That timing should be OK.

Assignee: nobody → cku

u459114

Updated

•

9 years ago

Assignee: cku → mtseng

Morris Tseng [:mtseng] [:Morris] (Inactive)

Comment 5

•

9 years ago

Attached patch remove-assert (obsolete) — Details — Splinter Review

Sometimes we'll create SourceSurface with stride 16 ( http://dxr.mozilla.org/mozilla-central/source/gfx/2d/SourceSurfaceRawData.cpp#32 ), and this kind of SourceSurface will hit assertion that causes crash. But I think although stride doesn't match to width multiply destPixelSize, We still can read it through tempSurf.

Attachment #8375254 - Flags: review?(jgilbert)

Kelsey Gilbert [:jgilbert] (previously Jeff)

Assignee

Comment 6

•

9 years ago

Comment on attachment 8375254 [details] [diff] [review]
remove-assert

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

The code below relies on this assert.

Attachment #8375254 - Flags: review?(jgilbert) → review-

Kelsey Gilbert [:jgilbert] (previously Jeff)

Assignee

Comment 7

•

9 years ago

Attached patch read-overstrided-surfs (obsolete) — Details — Splinter Review

This should allow for reading into non-tightly-packed surfaces.

Morris: Does this fix the issue?

Attachment #8375254 - Attachment is obsolete: true

Attachment #8375282 - Flags: review?(bjacob)

Attachment #8375282 - Flags: feedback?(mtseng)

Kelsey Gilbert [:jgilbert] (previously Jeff)

Assignee

Comment 8

•

9 years ago

Comment on attachment 8375282 [details] [diff] [review]
read-overstrided-surfs

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

::: gfx/gl/GLReadTexImageHelper.cpp
@@ +235,5 @@
> +static int
> +CalcStride(int width, int pixelSize, int alignment)
> +{
> +    MOZ_ASSERT(alignment);
> +    

Oops, more WS.

Morris Tseng [:mtseng] [:Morris] (Inactive)

Comment 9

•

9 years ago

Comment on attachment 8375282 [details] [diff] [review]
read-overstrided-surfs

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

This patch works for me.

Attachment #8375282 - Flags: feedback?(mtseng) → feedback+

Benoit Jacob [:bjacob] (mostly away)

Comment 10

•

9 years ago

Comment on attachment 8375282 [details] [diff] [review]
read-overstrided-surfs

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

::: gfx/gl/GLReadTexImageHelper.cpp
@@ +240,5 @@
> +    int stride = width * pixelSize;
> +    if (stride % alignment) { // Extra at the end of the line?
> +        int alignmentCount = stride / alignment;
> +        stride = (alignmentCount+1) * alignment;
> +    }

This function is not needed, if the next comment is correct.

If you decide to keep it, you could at least making it much faster by asserting that alignment is a POT and then replacing expensive / and % by bit ops.

@@ +248,5 @@
> +static int
> +GuessAlignment(int width, int pixelSize, int stride)
> +{
> +    int alignment = 8; // Max GLES allows.
> +    while (CalcStride(width, pixelSize, alignment) != stride) {

This function could be written more simply as:

int padding = stride - width * pixelSize;
return 1
       + 1 * ((padding % 2) == 0)
       + 2 * ((padding % 4) == 0)
       + 4 * ((padding % 8) == 0);

The % here are cheap because the RHS is POT.

Right?

(you'd probably want to assert that padding >= 0, too).

Attachment #8375282 - Flags: review?(bjacob) → review+

Benoit Jacob [:bjacob] (mostly away)

Comment 11

•

9 years ago

... or if you prefer a straightforward implementation because this is not so performance sensitive:

int padding = stride - width * pixelSize;
return ((padding % 8) == 0) ? 8
     : ((padding % 4) == 0) ? 4
     : ((padding % 2) == 0) ? 2
     : 1;

Benoit Jacob [:bjacob] (mostly away)

Comment 12

•

9 years ago

... in fact the latter is probably uniformly better, because compilers are smart enough, and this is definitely not that performance sensitive anyway.

Benoit Jacob [:bjacob] (mostly away)

Comment 13

•

9 years ago

Oh... nevermind my above comments, I was totally wrong :-)

Go with what you have and maybe just consider the above-mentioned optimization of assuming that alignment is POT (which it is) to avoid expensive arithmetic.

Kelsey Gilbert [:jgilbert] (previously Jeff)

Assignee

Comment 14

•

9 years ago

I would rather keep this obvious and 'expensive', because any time here will be dwarfed by the time the ReadPixels takes. This call should only happen about once per frame, so we'd only incur this cost on the order of a hundred times a second.

Kelsey Gilbert [:jgilbert] (previously Jeff)

Assignee

Comment 15

•

9 years ago

This is an issue caused by bug 892505.

Depends on: 892505

Kelsey Gilbert [:jgilbert] (previously Jeff)

Assignee

Comment 16

•

9 years ago

Attached patch read-overstrided-surfs — Details — Splinter Review

r=bjacob
Fixed whitespace.

Assignee: mtseng → jgilbert

Attachment #8375282 - Attachment is obsolete: true

Attachment #8375870 - Flags: review+

Kelsey Gilbert [:jgilbert] (previously Jeff)

Assignee

Comment 17

•

9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d57e7e2dfe4

[Back August 16] Ryan VanderMeulen

Comment 18

•

9 years ago

https://hg.mozilla.org/mozilla-central/rev/5d57e7e2dfe4

Status: NEW → RESOLVED

Closed: 9 years ago

Resolution: --- → FIXED

Target Milestone: --- → mozilla30

remove-assert 9 years ago Morris Tseng [:mtseng] [:Morris] (Inactive) 1.31 KB, patch	jgilbert : review-	Details \| Diff \| Splinter Review
read-overstrided-surfs 9 years ago Kelsey Gilbert [:jgilbert] (previously Jeff) 7.21 KB, patch	bjacob : review+ mtseng : feedback+	Details \| Diff \| Splinter Review
read-overstrided-surfs 9 years ago Kelsey Gilbert [:jgilbert] (previously Jeff) 7.20 KB, patch	jgilbert : review+	Details \| Diff \| Splinter Review

Bugzilla

PROCESS-CRASH | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | application crashed [@ mozilla::gl::ReadPixelsIntoImageSurface(mozilla::gl::GLContext, gfxImageSurface)] on Android 4.0 Debug

Categories

(Core :: Graphics: CanvasWebGL, defect)

Tracking

()

People

(Reporter: gbrown, Assigned: jgilbert)

References

Details

Crash Data

Security

(public)

User Story

Attachments

(1 file, 2 obsolete files)

Description

Comment 1

Updated

Comment 2

Comment 3

Comment 4

Updated

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Comment 11

Comment 12

Comment 13

Comment 14

Comment 15

Comment 16

Comment 17

Comment 18