Closed
Bug 1270180
Opened 8 years ago
Closed 8 years ago
More MOZ_CRASH to gfxDevCrash and MOZ_CRASH GFX prefix
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: milan, Assigned: milan)
Details
Attachments
(1 file)
See bug 1219494 - a few more uses of MOZ_CRASH without GFX prefix or MOZ_CRASH that should be gfxDevCrash to take care of.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → milan
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50501/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50501/
Attachment #8748741 -
Flags: review?(jgilbert)
Comment 2•8 years ago
|
||
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.
Attachment #8748741 -
Flags: review?(jgilbert)
Assignee | ||
Comment 3•8 years ago
|
||
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/
Attachment #8748741 -
Flags: review?(jgilbert)
Assignee | ||
Comment 4•8 years ago
|
||
(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•8 years ago
|
||
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.
Attachment #8748741 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 6•8 years ago
|
||
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 :)
Assignee | ||
Comment 7•8 years ago
|
||
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/
Assignee | ||
Comment 8•8 years ago
|
||
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 10•8 years ago
|
||
(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.
Assignee | ||
Comment 11•8 years ago
|
||
Darn, I only removed the returns in the places you pointed out, not everywhere.
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7389f08fb91c
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 13•8 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•