1281702 - "An assert from the graphics logger" assertion message should include the log message

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

https://hg.mozilla.org/mozilla-central/file/c9edfe35619f/gfx/2d/Logging.h#l519 produces output like:

[GFX1]: Texture deallocated too late during shutdown
Assertion failure: false (An assert from the graphics logger), at gfx/2d/Logging.h:519

Having these on separate lines confuses anything that tries to bucket failures by the assertion message, such as FuzzManager. Bugs on oranges are indistinguishable: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=ALL%20su%3A%22An%20assert%20from%20the%20graphics%20logger%22&order=bugs.bug_id%20desc&list_id=13081556

jwalden, what's the best way to fix this? MOZ_ASSERT only takes string literals; what can this code use instead?

Comment 1

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

Would the world explode if we changed:

MOZ_ASSERT(false, "An assert from the graphics logger");

to

MOZ_ASSERT(false, "%s", aString.c_str());

where aString is a std::string?

Comment 2

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

Heh, I guess not; I thought the vargs in the macro was for that kind of thing, but not really.

Comment 3

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

Attachment: WIP: Extra, non-string-literal parameter to MOZ_ASSERT.

Something like this?  Jesse, does it fix your issue?

Comment 4

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

Attachment: Bug 1281702: Report the actual error message, rather than a generic one from the graphics logger.

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

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8771550 [details]
Bug 1281702: Report the actual error message, rather than a generic one from the graphics logger.

https://reviewboard.mozilla.org/r/64670/#review61978

I see that we have a number of places in the tree that simply make direct calls to what `MOZ_ASSERT` expands to:

```
  MOZ_ReportAssertionFailure(...);
  MOZ_CRASH();
```

I think that is a better option than adding another case to `MOZ_ASSERT`.

Comment 6

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

(In reply to Nathan Froyd [:froydnj] from comment #5)
> ...
> I think that is a better option than adding another case to `MOZ_ASSERT`.

Good idea.

Comment 7

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

Comment on attachment 8771550 [details]
Bug 1281702: Report the actual error message, rather than a generic one from the graphics logger.

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

Comment 8

[Nathan Froyd [:froydnj]]

My understanding of Jesse's request is that the actual crash message differ based on the particular thing we are logging, and this new patch doesn't fix that?  Is that correct, Jesse?

Comment 9

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

It's the assert message that will be different - the crash message "doesn't matter" (e.g., it was empty before.)

Comment 10

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8771550 [details]
Bug 1281702: Report the actual error message, rather than a generic one from the graphics logger.

https://reviewboard.mozilla.org/r/64670/#review62260

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f45d79f6bc7
Report the actual error message, rather than a generic one from the graphics logger. r=mchang

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f45d79f6bc7