Closed Bug 1219494 Opened 4 years ago Closed 4 years ago

More gfxCrash instead of MOZ_CRASH

Categories

(Core :: Graphics, defect)

43 Branch
defect
Not set

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: nobody → milan
Whiteboard: [gfx-noted]
I've forgotten what gfxCrash() means. Perhaps we can give it a more descriptive name?
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?
Non-withstanding the discussion about renaming the gfxCrash.
Attachment #8680333 - Attachment is obsolete: true
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)
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 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
Attachment #8683724 - Flags: review?(mchang) → review+
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+
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...
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
Attachment #8685030 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/0f549771f1c9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Do you still want to land the other parts or can I cancel the review request? Thanks!
Flags: needinfo?(milan)
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)
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)
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)
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
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+
(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.
Part 1 already landed, this is a checkin-needed for Part 2, 3 and 4.
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
I have a feeling this will need rebasing once things currently on inbound land.
Keywords: checkin-needed
Flags: needinfo?(milan)
Attachment #8697387 - Flags: checkin? → checkin+
Attachment #8697386 - Flags: checkin? → checkin+
Attachment #8692567 - Flags: checkin? → checkin+
You need to log in before you can comment on or make changes to this bug.