Bug 240790

Summary: ANGLE: ERR: TextureMtl.mm:108 (GetLayerMipIndex): \t! Unreachable reached: GetLayerMipIndex(.../TextureMtl.mm:108)
Product: WebKit Reporter: Kimmo Kinnunen <kkinnunen>
Component: ANGLEAssignee: Kyle Piddington <kpiddington>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dino, ews-watchlist, kbr, kkinnunen, kondapallykalyan, kpiddington, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=239688
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Kimmo Kinnunen 2022-05-23 02:16:45 PDT
ANGLE: ERR: TextureMtl.mm:108 (GetLayerMipIndex): \t! Unreachable reached: GetLayerMipIndex(.../TextureMtl.mm:108)


0   libANGLE-shared.dylib         	       0x106bd35cc gl::LogMessage::~LogMessage() + 292 (debug.cpp:206)
1   libANGLE-shared.dylib         	       0x106bd35bc gl::LogMessage::~LogMessage() + 276 (debug.cpp:200)
2   libANGLE-shared.dylib         	       0x106bd2f10 gl::LogMessage::~LogMessage() + 32 (debug.cpp:182)
3   libANGLE-shared.dylib         	       0x1072aff78 rx::(anonymous namespace)::GetLayerMipIndex(std::__1::shared_ptr<rx::mtl::Texture> const&, unsigned int, unsigned int) + 528 (TextureMtl.mm:108)
4   libANGLE-shared.dylib         	       0x1072afd20 rx::TextureMtl::initializeContents(gl::Context const*, unsigned int, gl::ImageIndex const&) + 1208 (TextureMtl.mm:2072)
5   libANGLE-shared.dylib         	       0x106dcc4b4 gl::FramebufferAttachmentObject::initializeContents(gl::Context const*, unsigned int, gl::ImageIndex const&) + 696 (FramebufferAttachment.cpp:346)
6   libANGLE-shared.dylib         	       0x10728cd80 gl::Texture::ensureSubImageInitialized(gl::Context const*, gl::ImageIndex const&, gl::Box const&) + 96 (Texture.cpp:2326)
7   libANGLE-shared.dylib         	       0x10728cc10 gl::Texture::setSubImage(gl::Context*, gl::PixelUnpackState const&, gl::Buffer*, gl::TextureTarget, int, gl::Box const&, unsigned int, unsigned int, unsigned char const*) + 412 (Texture.cpp:1274)
8   libANGLE-shared.dylib         	       0x106b3373c gl::Context::texSubImage2D(gl::TextureTarget, int, int, int, int, int, unsigned int, unsigned int, void const*) + 304 (Context.cpp:5132)
9   libANGLE-shared.dylib         	       0x106b33844 gl::Context::texSubImage2DRobust(gl::TextureTarget, int, int, int, int, int, unsigned int, unsigned int, int, void const*) + 128 (Context.cpp:5148)
10  libANGLE-shared.dylib         	       0x106cc5d18 GL_TexSubImage2DRobustANGLE + 316 (entry_points_gles_ext_autogen.cpp:2091)
11  WebCore                       	       0x14aaf6e00 WebCore::GraphicsContextGLANGLE::texSubImage2D(unsigned int, int, int, int, int, int, unsigned int, unsigned int, GCGLSpan<void const, 18446744073709551615ul>) + 152 (GraphicsContextGLANGLE.cpp:406)
12  WebKit                        	       0x11871e738 WebKit::RemoteGraphicsContextGL::texSubImage2D0(unsigned int, int, int, int, int, int, unsigned int, unsigned int, WTF::Span<unsigned char const, 18446744073709551615ul>&&) + 272 (RemoteGraphicsContextGLFunctionsGenerated.h:794)
13  WebKit                        	       0x1188a0c38 void IPC::callMemberFunctionImpl<WebKit::RemoteGraphicsContextGL, void (WebKit::RemoteGraphicsContextGL::*)(unsigned int, int, int, int, int, int, unsigned int, unsigned int, WTF::Span<unsigned char const, 18446744073709551615ul>&&), std::__1::tuple<unsigned int, int, int, int, int, int, unsigned int, unsigned int, WTF::Span<unsigned char const, 18446744073709551615ul> >, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul, 8ul>(WebKit::RemoteGraphicsContextGL*, void (WebKit::RemoteGraphicsContextGL::*)(unsigned int, int, int, int, int, int, unsigned int, unsigned int, WTF::Span<unsigned char const, 18446744073709551615ul>&&), std::__1::tuple<unsigned int, int, int, int, int, int, unsigned int, unsigned int, WTF::Span<unsigned char const, 18446744073709551615ul> >&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul, 8ul>) + 420 (HandleMessage.h:131)


To repro:
1. minibrowser debug
2. develop -> experimental -> enable GPU Process for webgl
3. close window, open new window
4. https://www.colorizephoto.com/converter
5. Clear website data
6. reload https://www.colorizephoto.com/converter
7. Observe GPU Process crash
Comment 1 Radar WebKit Bug Importer 2022-05-30 02:17:13 PDT
<rdar://problem/94108163>
Comment 2 Kyle Piddington 2022-06-02 17:14:16 PDT
Created attachment 459976 [details]
Patch
Comment 3 EWS Watchlist 2022-06-02 17:15:28 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 4 Kenneth Russell 2022-06-02 18:17:32 PDT
Comment on attachment 459976 [details]
Patch

This looks good, but is there a way to produce a test for this? I can help upstream it to the WebGL conformance suite.
Comment 5 Kyle Piddington 2022-06-03 15:08:50 PDT
Created attachment 460022 [details]
Patch
Comment 6 Darin Adler 2022-06-05 15:23:21 PDT
Seems like the new patch includes the expected test result, but not the test.
Comment 7 Kimmo Kinnunen 2022-06-06 04:11:53 PDT
Comment on attachment 460022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=460022&action=review

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_resources.mm:277
> +    if (!refOut || !refOut->get() || !refOut->get()->valid())

Here we deref the refOut already above, so `!refOut` does not make sense

Also if we have
A* a = new A;
then it does not make sense to check `if (a)`, that's redundant. The new never returns nullptr.

could we do
  ASSERT(refOut);
  TextureRef tex = new Texture(...);
  ANGLE_MTL_CHECK(context, tex->valid(), GL_OUT_OF_MEMORY);

  .. use tex

  *refOut = std::move(tex);
   return angle::Result::Continue;
}

> LayoutTests/fast/canvas/webgl/large-texture-creation.html:44
> +  assertMsg(err == gl.NO_ERROR || err == gl.OUT_OF_MEMORY,

glErrorShouldBe(gl, [gl.NO_ERROR, gl.OUT_OF_MEMORY], "unexpected result when creating MAX_TEXTURE_SIZE texture")

(we don't currently immediately see what's wrong with the failing tests we get in this patch)
Comment 8 Kyle Piddington 2022-06-06 14:11:03 PDT
Created attachment 460052 [details]
Patch
Comment 9 Darin Adler 2022-06-06 14:54:35 PDT
Comment on attachment 460052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=460052&action=review

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_resources.mm:306
> +    if (!(*refOut) || !(*refOut)->get() || !refOut->get()->valid())

Same issue here; if *refOut was null we’d have created on the call to reset above.
Comment 10 Kyle Piddington 2022-06-06 15:00:19 PDT
Created attachment 460053 [details]
Patch
Comment 11 Darin Adler 2022-06-06 15:17:11 PDT
Comment on attachment 460053 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=460053&action=review

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_resources.mm:276
> +    ASSERT(refOut);

Not sure we need this assertion, but if we do, we probably need it in *both* overloads of MakeTexture.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_resources.mm:278
> +    ANGLE_MTL_CHECK(context, newTexture && newTexture->valid(), GL_OUT_OF_MEMORY);

Pretty sure that "new" never returns nullptr in C++, so the "newTexture &&" part of this is not needed.

> LayoutTests/fast/canvas/webgl/large-texture-creation.html:34
> +  gl.texImage2D(gl.TEXTURE_2D,

We changed two code paths, but with only one test case we must be covering only one of the two.

> LayoutTests/fast/canvas/webgl/large-texture-creation.html:44
> +  glErrorShouldBe(gl, [gl.NO_ERROR, gl.OUT_OF_MEMORY], "unexpected result when creating MAX_TEXTURE_SIZE texture")

Shouldn’t it always be OUT_OF_MEMORY? Why is NO_ERROR a possibility?
Comment 12 Kyle Piddington 2022-06-06 16:10:45 PDT
(In reply to Darin Adler from comment #11)
> Comment on attachment 460053 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=460053&action=review
> 
> > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_resources.mm:276
> > +    ASSERT(refOut);
> 
> Not sure we need this assertion, but if we do, we probably need it in *both*
> overloads of MakeTexture.

I'll update this before landing.
> 
> > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_resources.mm:278
> > +    ANGLE_MTL_CHECK(context, newTexture && newTexture->valid(), GL_OUT_OF_MEMORY);
> 
> Pretty sure that "new" never returns nullptr in C++, so the "newTexture &&"
> part of this is not needed.
Double checked this, I believe you're right. We'd get an exception instead.
> 
> > LayoutTests/fast/canvas/webgl/large-texture-creation.html:34
> > +  gl.texImage2D(gl.TEXTURE_2D,
> 
> We changed two code paths, but with only one test case we must be covering
> only one of the two.

I don't have a good way to test the IOSurface binding API from just a webgl script, I suppose I could try to create a max-sized canvas object? 

> > LayoutTests/fast/canvas/webgl/large-texture-creation.html:44
> > +  glErrorShouldBe(gl, [gl.NO_ERROR, gl.OUT_OF_MEMORY], "unexpected result when creating MAX_TEXTURE_SIZE texture")
> 
> Shouldn’t it always be OUT_OF_MEMORY? Why is NO_ERROR a possibility?

I kept no_error a possibility because some devices, especially those with larger GPUS, can actually allocate a max-sized F32 texture. a Mac pro with an AMD card, for instance, handles this fine as far as I could tell from a local build.
Comment 13 Kyle Piddington 2022-06-08 14:17:54 PDT
Created attachment 460103 [details]
Patch
Comment 14 EWS 2022-06-09 10:17:44 PDT
kpiddington@apple.com does not have reviewer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json.

Rejecting attachment 460103 [details] from commit queue.
Comment 15 EWS 2022-06-09 12:25:16 PDT
Committed r295429 (251435@main): <https://commits.webkit.org/251435@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 460103 [details].