Closed Bug 1277027 Opened 3 years ago Closed 3 years ago

Fix a few places in WebGL/Canvas2D where graphics MOZ_CRASH does not have a GFX prefixed message

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: milan, Assigned: ernest)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

There are a few files in dom/canvas that use MOZ_CRASH, but don't have GFX: prefix, like the majority of the graphics code.  Makes it easier to do a quick search in the crash reports.  See also bug 1272767.
Assignee: nobody → eyim
Whiteboard: [gfx-noted]
Comment on attachment 8759293 [details]
Bug 1277027 - fix MOZ_CRASH to have GFX prefix in webGL code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57288/diff/1-2/
Comment on attachment 8759293 [details]
Bug 1277027 - fix MOZ_CRASH to have GFX prefix in webGL code

https://reviewboard.mozilla.org/r/57288/#review54118
Attachment #8759293 - Flags: review?(milan) → review+
Attachment #8759293 - Flags: review?(bgirard)
Will need someone to help me land, if/when this is approved.
Nice, thanks Ernest.

I wouldn't mind you doing a pass through MOZ_RELEASE_ASSERT calls we make, in gfx/ and dom/canvas/ and making sure they all have a string description (second parameter), with the GFX: prefix.  Pretty sure none of them do right now.  In this bug is fine.
Comment on attachment 8759293 [details]
Bug 1277027 - fix MOZ_CRASH to have GFX prefix in webGL code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57288/diff/2-3/
Attachment #8759293 - Flags: review?(bgirard)
I feel like we need a policy for putting so many string in the binary. I also added a lot via the failure ID and over time these strings really add up.
Comment on attachment 8759293 [details]
Bug 1277027 - fix MOZ_CRASH to have GFX prefix in webGL code

https://reviewboard.mozilla.org/r/57288/#review54424

Looks good, just a few string nitpicks

::: dom/canvas/WebGLContext.cpp:675
(Diff revision 3)
>                                    nsACString* const out_failureId)
>  {
>      std::queue<gl::SurfaceCaps> fallbackCaps;
>      PopulateCapFallbackQueue(baseCaps, &fallbackCaps);
>  
> -    MOZ_RELEASE_ASSERT(!gl);
> +    MOZ_RELEASE_ASSERT(!gl, "GFX: Create context flags set.");

Already have a context

::: dom/canvas/WebGLContext.cpp:2109
(Diff revision 3)
>      gl::GLContext* gl = webgl->GL();
>      gl->MakeCurrent();
>  
>      auto compression = usage->format->compression;
>      if (compression) {
> -        MOZ_RELEASE_ASSERT(!xOffset && !yOffset && !zOffset);
> +        MOZ_RELEASE_ASSERT(!xOffset && !yOffset && !zOffset, "GFX: texture offsets not zeroed.");

Can't zero compressed texture with offsets
Attachment #8759293 - Flags: review?(bgirard) → review+
Comment on attachment 8759293 [details]
Bug 1277027 - fix MOZ_CRASH to have GFX prefix in webGL code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57288/diff/3-4/
if no more issues, could you please try and/or land this for me?
Flags: needinfo?(bgirard)
Try submited, let me know when it's ready to land.
Flags: needinfo?(bgirard) → needinfo?(eyim)
Comment on attachment 8759293 [details]
Bug 1277027 - fix MOZ_CRASH to have GFX prefix in webGL code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57288/diff/4-5/
Could you please resubmit try?
Flags: needinfo?(eyim) → needinfo?(bgirard)
Done, if you can't submit from mozreview you should still be able to push using:
hg push -f try

Just looking to save you the extra delay of me pushing it.
Flags: needinfo?(bgirard)
Comment on attachment 8759293 [details]
Bug 1277027 - fix MOZ_CRASH to have GFX prefix in webGL code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57288/diff/5-6/
Pushed by b56girard@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17276a2bbd6a
fix MOZ_CRASH to have GFX prefix in webGL code r=BenWa,milan
https://hg.mozilla.org/mozilla-central/rev/17276a2bbd6a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.