1270180 - More MOZ_CRASH to gfxDevCrash and MOZ_CRASH GFX prefix

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

See bug 1219494 - a few more uses of MOZ_CRASH without GFX prefix or MOZ_CRASH that should be gfxDevCrash to take care of.

Comment 1

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

Attachment: MozReview Request: Bug 1270180: More instances of MOZ_CRASH converted to gfxDevCrash and getting GFX prefix. r?jgilbert

Review commit: https://reviewboard.mozilla.org/r/50501/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50501/

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8748741 [details]
MozReview Request: Bug 1270180: More instances of MOZ_CRASH converted to gfxDevCrash and getting GFX prefix. r?jgilbert

https://reviewboard.mozilla.org/r/50501/#review47399

::: gfx/gl/SharedSurfaceD3D11Interop.cpp:164
(Diff revision 1)
> -            printf_stderr("wglDXRegisterObject(0x%x, 0x%x, %u, 0x%x, 0x%x) failed:"
> -                          " GetLastError(): 0x%x\n", mDXGLDeviceHandle, dxObject, name,
> -                          type, access, error);
> -#endif
> -            MOZ_CRASH();
> +            gfxCriticalError() << "wglDXRegisterObject("
> +                               << gfx::hexa(mDXGLDeviceHandle) << ", "
> +                               << gfx::hexa(dxObject) << ", "
> +                               << name << ", "
> +                               << gfx::hexa(type) << ", "
> +                               << gfx::hexa(access) << ") failed:"
> +                               << " GetLastError(): " << gfx::hexa(error);

It's preferable for complex string formatting to use nsPrintfCString.

printf_stderr("asdofasd", a, b, c)
becomes
const nsPrintfCString text("asdofasd", a, b, c)
gfxCriticalError() << text.BeginReading();

::: gfx/gl/SharedSurfaceD3D11Interop.cpp:180
(Diff revision 1)
> -            printf_stderr("wglDXUnregisterObject(0x%x, 0x%x) failed: GetLastError():"
> -                          " 0x%x\n", mDXGLDeviceHandle, hObject, error);
> -#endif
> -            MOZ_CRASH();
> +            gfxCriticalError() << "wglDXUnregisterObject("
> +                               << gfx::hexa(mDXGLDeviceHandle) << ", "
> +                               << gfx::hexa(hObject) << ") failed: GetLastError():"
> +                               << gfx::hexa(error);

Yeah, this stuff is hard to read now.

::: gfx/layers/ImageDataSerializer.cpp:115
(Diff revision 1)
>      case BufferDescriptor::TRGBDescriptor:
>        return aDescriptor.get_RGBDescriptor().format();
>      case BufferDescriptor::TYCbCrDescriptor:
>        return gfx::SurfaceFormat::YUV;
>      default:
> -      MOZ_CRASH();
> +      MOZ_CRASH("GFX: Format from buffer descriptor");

Just use the function name directly: FormatFromBufferDescriptor

::: gfx/layers/composite/TextureHost.cpp:244
(Diff revision 1)
>          return CreateTextureHostD3D11(aDesc, aDeallocator, aFlags);
>        }
>  #endif
>      default:
>        MOZ_CRASH("GFX: Unsupported Surface type host");
> +      break;

I don't think this is needed?

::: gfx/layers/ipc/SharedPlanarYCbCrImage.cpp:196
(Diff revision 1)
>    // pointers out of the TextureClient and keeps them around, which works only
>    // because the underlyin BufferTextureData is always mapped in memory even outside
>    // of the lock/unlock interval. That's sad and new code should follow this example.
>    if (!mTextureClient->Lock(OpenMode::OPEN_READ) || !mTextureClient->BorrowMappedYCbCrData(mapped)) {
> -    MOZ_CRASH();
> +    gfxDevCrash(LogReason::YCbCr) << "Cannot open for read";
> +    return false;

If Lock() succeeds but Borrow() fails, don't we need to unlock if this is going to be fallible?

I'm assuming gfxDevCrash doesn't always crash, hence the new returns here and elsewhere.

Comment 3

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

Comment on attachment 8748741 [details]
MozReview Request: Bug 1270180: More instances of MOZ_CRASH converted to gfxDevCrash and getting GFX prefix. r?jgilbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50501/diff/1-2/

Comment 4

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

(In reply to Milan Sreckovic [:milan] from comment #3)
> Comment on attachment 8748741 [details]
> MozReview Request: Bug 1270180: More instances of MOZ_CRASH converted to
> gfxDevCrash and getting GFX prefix. r?jgilbert
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/50501/diff/1-2/

Addressing the review comments.

And, yes, gfxDevCrash() doesn't crash on beta and release.  I've removed the one from the IPDL code, left the other one in, which was in a function that was already "ready to fail".

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8748741 [details]
MozReview Request: Bug 1270180: More instances of MOZ_CRASH converted to gfxDevCrash and getting GFX prefix. r?jgilbert

https://reviewboard.mozilla.org/r/50501/#review47551

::: gfx/layers/ipc/SharedPlanarYCbCrImage.cpp:139
(Diff revisions 1 - 2)
>      // The caller expects a pointer to the beginning of the writable part of the
>      // buffer which is where the y channel starts by default.
>      return mapped.y.data;
>    } else {
> -    gfxDevCrash(LogReason::YCbCr) << "GFX: Cannot borrow mapped YCbCr data";
> +    MOZ_CRASH("GFX: Cannot borrow mapped YCbCr data");
>      return nullptr;

Should ideally omit returns after MOZ_CRASHes.

Comment 6

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

I just don't like the code that looks like this:

  if (something) {
    return whatever;
  } else {
    MOZ_CRASH("GFX: Cannot borrow mapped YCbCr data");
  }
}

even though __declspec(noreturn) is buried in the definition of MOZ_CRASH :)

Comment 7

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

Comment on attachment 8748741 [details]
MozReview Request: Bug 1270180: More instances of MOZ_CRASH converted to gfxDevCrash and getting GFX prefix. r?jgilbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50501/diff/2-3/

Comment 8

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

Comment on attachment 8748741 [details]
MozReview Request: Bug 1270180: More instances of MOZ_CRASH converted to gfxDevCrash and getting GFX prefix. r?jgilbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50501/diff/3-4/

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7389f08fb91c

Comment 10

[Kelsey Gilbert [:jgilbert]]

(In reply to Milan Sreckovic [:milan] from comment #6)
> I just don't like the code that looks like this:
> 
>   if (something) {
>     return whatever;
>   } else {
>     MOZ_CRASH("GFX: Cannot borrow mapped YCbCr data");
>   }
> }
> 
> even though __declspec(noreturn) is buried in the definition of MOZ_CRASH :)

That's fair, though I've gotten used to it.
I think it's better because it doesn't make it look like a function is fallible when it's not, because of the fake return values. It makes it look like the caller may need to handle it.

Comment 11

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

Darn, I only removed the returns in the places you pointed out, not everywhere.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7389f08fb91c

Comment 13

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

(In reply to Milan Sreckovic [:milan] from comment #11)
> Darn, I only removed the returns in the places you pointed out, not
> everywhere.

I'll fix that in bug 1272767.