525538 - [webgl] bogus readPixels leads to crash [@ ConvertUTF8toUTF16::write]

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Paul Nickerson]

Attachment: testcase

Load the testcase and press refresh until it crashes.

Comment 1

[Paul Nickerson]

Attachment: stack

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Interesting; that stack is way outside of the canvas calls, but something inside readPixels might be going wrong.  I would think it would throw since '10 20 30' can't be converted to an integer, but I'm not sure...

Comment 3

[Jesse Ruderman]

Attachment: valgrind output

Comment 4

[Jesse Ruderman]

Vlad, does that help?  Should be much closer to the source of the memory corruption.

Comment 5

[Jesse Ruderman]

Attachment: valgrind output with line numbers

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ah yep, looks like the buffer being allocated by readpixels isn't big enough for the params.  It's probably just not calculating the size correctly, or not checking for overflow (thought I did that everywhere, but guess not).

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

WebGL only -- should be a short fix, so eta a few days once I get back to WebGL work.

Comment 8

[Joe Drew (not getting mail)]

Vlad: when will you get back to WebGL work? If it's not soon, we should get someone else to work on this.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Here's another error reported by valgrind, that happens upon first loading of the testcase, complaining about GetCanvasSize jumping depending on an uninitialized value:

Canvas 3D: creating PBuffer...
Canvas 3D: can't get a native PBuffer, trying OSMesa...
Canvas 3D: ready
==8679== Thread 1:
==8679== Conditional jump or move depends on uninitialised value(s)
==8679==    at 0x59F3981: nsHTMLCanvasFrame::GetCanvasSize() (nsHTMLCanvasFrame.cpp:123)
==8679==    by 0x59F3B37: nsHTMLCanvasFrame::ComputeSize(nsIRenderingContext*, nsSize, int, nsSize, nsSize, nsSize, int) (nsHTMLCanvasFrame.cpp:164)
==8679==    by 0x59FDEB6: nsHTMLReflowState::InitConstraints(nsPresContext*, int, int, nsMargin const*, nsMargin const*) (nsHTMLReflowState.cpp:1847)
==8679==    by 0x59FA162: nsHTMLReflowState::Init(nsPresContext*, int, int, nsMargin const*, nsMargin const*) (nsHTMLReflowState.cpp:283)
==8679==    by 0x59F9DF1: nsHTMLReflowState::nsHTMLReflowState(nsPresContext*, nsHTMLReflowState const&, nsIFrame*, nsSize const&, int, int, int) (nsHTMLReflowState.cpp:176)
==8679==    by 0x5A134C9: nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, int&) (nsLineLayout.cpp:772)
==8679==    by 0x599E986: nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) (nsBlockFrame.cpp:3716)
==8679==    by 0x599DEAD: nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, int*, LineReflowStatus*, int) (nsBlockFrame.cpp:3511)
==8679==    by 0x599D93B: nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, int*) (nsBlockFrame.cpp:3365)
==8679==    by 0x599AD20: nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, int*) (nsBlockFrame.cpp:2461)
==8679==    by 0x5998AC1: nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) (nsBlockFrame.cpp:1907)
==8679==    by 0x5995C61: nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) (nsBlockFrame.cpp:1009)

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Now that that other error is fixed (see bug 566603), I get this new valgrind error:

Canvas 3D: creating PBuffer...
Canvas 3D: can't get a native PBuffer, trying OSMesa...
Canvas 3D: ready
++DOMWINDOW == 13 (0x318d9860) [serial = 25] [outer = 0x1c60b400]
==18207== Thread 1:
==18207== Jump to the invalid address stated on the next line
==18207==    at 0x0: ???
==18207==    by 0x6A69AC0: nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) (nsComponentManager.cpp:1677)
==18207==    by 0x6A0272A: CallCreateInstance(char const*, nsISupports*, nsID const&, void**) (nsComponentManagerUtils.cpp:170)
==18207==    by 0x6A028D9: nsCreateInstanceByContractID::operator()(nsID const&, void**) const (nsComponentManagerUtils.cpp:210)
==18207==    by 0x5845B4A: nsCOMPtr<nsIScriptError>::assign_from_helper(nsCOMPtr_helper const&, nsID const&) (nsCOMPtr.h:1249)
==18207==    by 0x58454AE: nsCOMPtr<nsIScriptError>::nsCOMPtr(nsCOMPtr_helper const&) (nsCOMPtr.h:621)
==18207==    by 0x5F5C2B3: PrintWarningOnConsole(JSContext*, char const*) (nsDOMClassInfo.cpp:1572)
==18207==    by 0x5F67B7E: nsWindowSH::GlobalScopePolluterGetProperty(JSContext*, JSObject*, long, long*) (nsDOMClassInfo.cpp:4688)
==18207==    by 0x7C5D98E: JSScopeProperty::get(JSContext*, JSObject*, JSObject*, long*) (jsscope.h:996)
==18207==    by 0x7C56870: js_NativeGet (jsobj.cpp:4617)
==18207==    by 0x7C290BF: js_Interpret (jsops.cpp:2295)
==18207==    by 0x7C3C607: js_Execute (jsinterp.cpp:1073)
==18207==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==18207==

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

This is hard to reproduce, I get a different valgrind error every time, however the next one is perhaps interesting because the invalid address 0xa5a5a5a53f000000 looks funny. Does this give anyone an idea where that value might be coming from?

==30059== Thread 1:
==30059== Jump to the invalid address stated on the next line
==30059==    at 0xA5A5A5A53F000000: ???
==30059==    by 0xAA0B51D: g_closure_invoke (in /usr/lib/libgobject-2.0.so.0.2400
.1)
==30059==    by 0xAA19FA5: signal_emit_unlocked_R (in /usr/lib/libgobject-2.0.so.
0.2400.1)
==30059==    by 0xAA233F3: g_signal_emit_valist (in /usr/lib/libgobject-2.0.so.0.
2400.1)
==30059==    by 0xAA235C2: g_signal_emit (in /usr/lib/libgobject-2.0.so.0.2400.1)
==30059==    by 0x8DF2B6F: gtk_object_dispose (in /usr/lib/libgtk-x11-2.0.so.0.20
00.0)
==30059==    by 0xAA0DF5F: g_object_run_dispose (in /usr/lib/libgobject-2.0.so.0.
2400.1)
==30059==    by 0x67C1FCD: nsWindow::Destroy() (nsWindow.cpp:789)
==30059==    by 0x67C11B4: nsWindow::~nsWindow() (nsWindow.cpp:486)
==30059==    by 0x67C1267: nsWindow::~nsWindow() (nsWindow.cpp:487)
==30059==    by 0x67F9B9A: nsBaseWidget::Release() (nsBaseWidget.cpp:72)
==30059==    by 0x67C13F7: nsWindow::Release() (nsWindow.cpp:500)
==30059==  Address 0xa5a5a5a53f000000 is not stack'd, malloc'd or (recently) free
'd
==30059==

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

According to firebot, a5a5a5a5 is jemalloc allocated unintialized junk memory.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

OK so WebGLContext::ReadPixels is being passed very reasonable arguments:

Breakpoint 1, mozilla::WebGLContext::ReadPixels (this=0x7fffec19b100, x=0, y=1, 
    width=1, height=35, format=6406, type=5121)

See, the string's been replaced by 0 and the huge number by 1. There's no overflow here. The value of 'len' in that function is 35, as expected.

However, commenting out the actual GL call, gl->fReadPixels(), does make the crash go away.

Can anyone make sense of that?

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Fix WebGLContext::ReadPixels() by honoring GL_PACK_ALIGNMENT

The problem was that we didn't honor the value of GL_PACK_ALIGNMENT when computing the size of the return buffer. FYI, the default value is 4 and since here we have 1 byte per pixel, the buffer needed to be 4x bigger.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 446037 [details] [diff] [review]
Fix WebGLContext::ReadPixels() by honoring GL_PACK_ALIGNMENT

GL_PACK_ALIGNMENT specifies the alignment of each pixel *row*, not each pixel.  So basically we need to compute a stride (width * size) and then increase it until it's a multiple of the pack alignment.

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Fix WebGLContext::ReadPixels() by honoring GL_PACK_ALIGNMENT, Take 2

*facepalms*

Sorry. New patch attached.

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Fix WebGLContext::ReadPixels() by honoring GL_PACK_ALIGNMENT, Take 3

fix comment.

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Almost -- the last row isn't aligned, so it should be something like (height-1)*alignedRowSize + width*size

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Fix WebGLContext::ReadPixels() by honoring GL_PACK_ALIGNMENT, Take 4

This time computing the buffer length taking into account the fact that we don't need padding after the last row.

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Fix WebGLContext::ReadPixels() by honoring GL_PACK_ALIGNMENT, Take 5

oops, missing semicolon in the previous patch

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

pushed out