Closed
Bug 1219494
Opened 9 years ago
Closed 8 years ago
More gfxCrash instead of MOZ_CRASH
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: milan, Assigned: milan)
References
Details
(Keywords: feature, Whiteboard: [gfx-noted])
Attachments
(4 files, 16 obsolete files)
6.68 KB,
patch
|
milan
:
review+
cbook
:
checkin+
|
Details | Diff | Splinter Review |
30.59 KB,
patch
|
milan
:
review+
cbook
:
checkin+
|
Details | Diff | Splinter Review |
18.84 KB,
patch
|
milan
:
review+
cbook
:
checkin+
|
Details | Diff | Splinter Review |
16.13 KB,
patch
|
milan
:
review+
cbook
:
checkin+
|
Details | Diff | Splinter Review |
Between gfx, dom/canvas, imagelib, there are about 60 files that use MOZ_CRASH. Review and see which ones should use gfxCrash instead.
Assignee | ||
Comment 1•9 years ago
|
||
Updated•9 years ago
|
Assignee: nobody → milan
Whiteboard: [gfx-noted]
Comment 2•9 years ago
|
||
I've forgotten what gfxCrash() means. Perhaps we can give it a more descriptive name?
Assignee | ||
Comment 3•9 years ago
|
||
Sure, I'm open to suggestions. It's a graphics take on MOZ_CRASH, thus the name. It is assert in all debug builds, MOZ_CRASH in nightly and dev edition, and telemetry for GFX_CRASH histogram on Beta and Release. We probably want "crash" in the name, we could add "telemetry" in there as well. gfx prefix just because it looks similar to all the other gfxCriticalError, gfxWarning, etc. gfxForgivingCrash? gfxCrashOrTelemetry? gfxMozCrash? gfxCrash565_16?
Assignee | ||
Comment 4•9 years ago
|
||
Non-withstanding the discussion about renaming the gfxCrash.
Attachment #8680333 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8681500 -
Attachment is obsolete: true
Attachment #8681502 -
Attachment is obsolete: true
Attachment #8681503 -
Attachment is obsolete: true
Attachment #8681505 -
Attachment is obsolete: true
Attachment #8683719 -
Flags: review?(mchang)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8683720 -
Flags: review?(mchang)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8683724 -
Flags: review?(mchang)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8683725 -
Flags: review?(mchang)
Comment 12•9 years ago
|
||
Comment on attachment 8683719 [details] [diff] [review] Part 1. TextureD3D11 and gfxCrash for the most part replacing MOZ_CRASH. Remaining MOZ_CRASH get GFX prefix to find them easier. r=mchang Review of attachment 8683719 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/d3d11/TextureD3D11.cpp @@ +1252,5 @@ > RefPtr<IDXGIKeyedMutex> mutex; > hr = mD3D10Texture->QueryInterface((IDXGIKeyedMutex**)getter_AddRefs(mutex)); > > if (FAILED(hr) || !mutex) { > gfxCriticalError() << "Failed to get KeyedMutex: " << hexa(hr); Can you merge the gfxCriticalError above and just make it a gfxCrash? Or do we not have the equivalent of a MOZ_CRASH? @@ +1278,5 @@ > hr = mD3D11Texture->QueryInterface((IDXGIKeyedMutex**)getter_AddRefs(mutex)); > > if (FAILED(hr) || !mutex) { > gfxCriticalError() << "Failed to get KeyedMutex: " << hexa(hr); > + MOZ_CRASH("GFX: Cannot get D3D11 KeyedMutex"); same as above.
Attachment #8683719 -
Flags: review?(mchang) → review+
Comment 13•9 years ago
|
||
Comment on attachment 8683720 [details] [diff] [review] Part 2. gfx/2d and gfxCrash for the most part replacing MOZ_CRASH. Remaining MOZ_CRASH get GFX prefix to find them easier. r=mchang Review of attachment 8683720 [details] [diff] [review]: ----------------------------------------------------------------- Mostly looks fine. Just want to make sure, is there a reason sometimes we changed the MOZ_CRASH to just a gfxDevCrash and a return? ::: gfx/2d/DataSurfaceHelpers.cpp @@ +238,5 @@ > { > if (aSrcRect.Overflows() || > IntRect(aDestPoint, aSrcRect.Size()).Overflows()) { > + gfxDevCrash(LogReason::InvalidRect) << "Invalid rects " << (int)aSrcRect.Overflows(); > + return; do we want to not crash here on release anymore? ::: gfx/2d/DrawTargetCairo.cpp @@ +1216,5 @@ > // allocation in ~99% of cases. > Vector<cairo_glyph_t, 1024 / sizeof(cairo_glyph_t)> glyphs; > if (!glyphs.resizeUninitialized(aBuffer.mNumGlyphs)) { > + gfxDevCrash(LogReason::GlyphAllocFailedCairo) << "glyphs allocation failed"; > + return; same: Is it actually safe to not crash on release anymore? ::: gfx/2d/DrawTargetSkia.cpp @@ +100,5 @@ > > RefPtr<DataSourceSurface> surf = aSurface->GetDataSurface(); > if (!surf) { > + gfxDevCrash(LogReason::SourceSurfaceIncompatible) << "Non-skia SourceSurfaces need to be DataSourceSurfaces"; > + return result; same. ::: gfx/2d/FilterNodeSoftware.cpp @@ +1501,5 @@ > } > targetData += stride; > } > } else { > + gfxDevCrash(LogReason::FilterInputFormat) << "Bad format in flood render " << (int)format; same
Updated•9 years ago
|
Attachment #8683724 -
Flags: review?(mchang) → review+
Comment 14•9 years ago
|
||
Comment on attachment 8683725 [details] [diff] [review] Part 4. gfx/layers and gfxCrash for the most part replacing MOZ_CRASH. Remaining MOZ_CRASH get GFX prefix to find them easier. r=mchang Review of attachment 8683725 [details] [diff] [review]: ----------------------------------------------------------------- Mostly same thing. In places that we currently have a MOZ_CRASH and are renaming it to a gfxCrash, are those places on release builds where we can actually continue? ::: gfx/layers/basic/BasicLayerManager.cpp @@ +875,5 @@ > parent->GetEffectiveTransform() : > Matrix4x4(); > Matrix transform; > if (!transform3d.CanDraw2D(&transform)) { > + gfxDevCrash(LogReason::CannotDraw3D) << "GFX: We should not have a 3D transform that CanDraw2D() is false!"; Is this something we can recover from? ::: gfx/layers/client/TextureClientPool.cpp @@ +76,1 @@ > << aPool << "-" << aPool->GetFormat() << ", nullptr, " same.
Attachment #8683725 -
Flags: review?(mchang) → review+
Assignee | ||
Comment 15•9 years ago
|
||
Good points, I will double check all the cases you mentioned as "should we leave MOZ_CRASH". Certainly in some cases putting a MOZ_CRASH is a wishful thinking, because we can get into that code in normal conditions. On the TextureD3D11.cpp comment - I left both critical error and MOZ_CRASH in there because I can get more information in the crash report that way - MOZ_CRASH won't let me pass the HRESULT back in the crash report...
Assignee | ||
Comment 16•9 years ago
|
||
Part 1, with the dependency on bug 1222033 https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba294e17194f
Attachment #8683719 -
Attachment is obsolete: true
Attachment #8685030 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8685030 -
Flags: checkin?
Assignee | ||
Comment 17•9 years ago
|
||
Checkin for Part 1 only: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba294e17194f, and note the dependency on bug 1222033 (currently on inbound)
Keywords: checkin-needed
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f549771f1c9
Keywords: checkin-needed
Updated•9 years ago
|
Attachment #8685030 -
Flags: checkin? → checkin+
Comment 19•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0f549771f1c9
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 20•8 years ago
|
||
Do you still want to land the other parts or can I cancel the review request? Thanks!
Flags: needinfo?(milan)
Assignee | ||
Comment 21•8 years ago
|
||
I want to make changes you suggested, or at least investigate if they need to be made, so, yes, I'll cancel the review request until I do so.
Flags: needinfo?(milan)
Assignee | ||
Comment 22•8 years ago
|
||
Comment on attachment 8683720 [details] [diff] [review] Part 2. gfx/2d and gfxCrash for the most part replacing MOZ_CRASH. Remaining MOZ_CRASH get GFX prefix to find them easier. r=mchang Will address the comments before requesting another review.
Attachment #8683720 -
Flags: review?(mchang)
Assignee | ||
Comment 23•8 years ago
|
||
Went through the ones that changed from "crash always" to "telemetry in release+beta instead" - these are the cases where the failure is either input data related, or the system fails to give us what we want, and the functions making these calls already have the "this could fail" early returns. The rest are annotated MOZ_CRASH with GFX: to find them easier.
Attachment #8683720 -
Attachment is obsolete: true
Attachment #8690277 -
Flags: review?(mchang)
Assignee | ||
Updated•8 years ago
|
Comment 24•8 years ago
|
||
Comment on attachment 8690277 [details] [diff] [review] Part 2. gfx/2d and gfxCrash for the most part replacing MOZ_CRASH. Remaining MOZ_CRASH get GFX prefix to find them easier. r=mchang Review of attachment 8690277 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Milan Sreckovic [:milan] from comment #23) > Went through the ones that changed from "crash always" to "telemetry in > release+beta instead" - these are the cases where the failure is either > input data related, or the system fails to give us what we want, and the > functions making these calls already have the "this could fail" early > returns. > The rest are annotated MOZ_CRASH with GFX: to find them easier. Thanks for checking! I think I mentioned this in a few places in part 3 and 4. Can you please do that there as well?
Attachment #8690277 -
Flags: review?(mchang) → review+
Assignee | ||
Comment 25•8 years ago
|
||
(In reply to Mason Chang PTO [:mchang] from comment #24) > > Thanks for checking! I think I mentioned this in a few places in part 3 and > 4. Can you please do that there as well? Yes, most are removed except for the "bad 2D" which just proceeds, and should be fine. I'll update the patches.
Assignee | ||
Comment 26•8 years ago
|
||
Attachment #8683724 -
Attachment is obsolete: true
Attachment #8691503 -
Flags: review+
Assignee | ||
Comment 27•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfc68a968b7a
Attachment #8683725 -
Attachment is obsolete: true
Attachment #8691504 -
Flags: review+
Assignee | ||
Comment 28•8 years ago
|
||
Missing include.
Attachment #8690277 -
Attachment is obsolete: true
Attachment #8691583 -
Flags: review+
Assignee | ||
Comment 29•8 years ago
|
||
Fixing a typo.
Attachment #8691503 -
Attachment is obsolete: true
Attachment #8691586 -
Flags: review+
Assignee | ||
Comment 30•8 years ago
|
||
Attachment #8691504 -
Attachment is obsolete: true
Attachment #8691587 -
Flags: review+
Assignee | ||
Comment 31•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac3e225319be
Assignee | ||
Comment 32•8 years ago
|
||
Part 1 already landed, this is a checkin-needed for Part 2, 3 and 4.
Keywords: leave-open → checkin-needed
Comment 33•8 years ago
|
||
Hi Milan, part 2 failed to apply: renamed 1219494 -> 1219494.2.p5 applying 1219494.2.p5 patching file gfx/2d/DataSurfaceHelpers.cpp Hunk #1 FAILED at 15 1 out of 2 hunks FAILED -- saving rejects to file gfx/2d/DataSurfaceHelpers.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh 1219494.2.p5 could you take a look, thanks!
Flags: needinfo?(milan)
Keywords: checkin-needed
Assignee | ||
Comment 34•8 years ago
|
||
Rebase.
Attachment #8691583 -
Attachment is obsolete: true
Flags: needinfo?(milan)
Attachment #8692567 -
Flags: review+
Assignee | ||
Comment 35•8 years ago
|
||
Rebase.
Attachment #8691587 -
Attachment is obsolete: true
Attachment #8692568 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 36•8 years ago
|
||
I have a feeling this will need rebasing once things currently on inbound land.
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(milan)
Assignee | ||
Comment 37•8 years ago
|
||
Rebase
Attachment #8691586 -
Attachment is obsolete: true
Flags: needinfo?(milan)
Attachment #8697386 -
Flags: review+
Assignee | ||
Comment 38•8 years ago
|
||
Rebase
Attachment #8692568 -
Attachment is obsolete: true
Attachment #8697387 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8692567 -
Flags: checkin?
Assignee | ||
Updated•8 years ago
|
Attachment #8697386 -
Flags: checkin?
Assignee | ||
Updated•8 years ago
|
Attachment #8697387 -
Flags: checkin?
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 39•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/279f56c0f49e https://hg.mozilla.org/integration/mozilla-inbound/rev/9515714585dd https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f5472e714b
Keywords: checkin-needed
Updated•8 years ago
|
Attachment #8697387 -
Flags: checkin? → checkin+
Updated•8 years ago
|
Attachment #8697386 -
Flags: checkin? → checkin+
Updated•8 years ago
|
Attachment #8692567 -
Flags: checkin? → checkin+
Comment 40•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/279f56c0f49e https://hg.mozilla.org/mozilla-central/rev/9515714585dd https://hg.mozilla.org/mozilla-central/rev/f4f5472e714b
Status: REOPENED → RESOLVED
Closed: 9 years ago → 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•