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

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jruderman, Assigned: milan)

Tracking

({feature})

Trunk
mozilla50
Unspecified
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment, 1 obsolete attachment)

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?
Flags: needinfo?(jwalden+bmo)
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?
Heh, I guess not; I thought the vargs in the macro was for that kind of thing, but not really.
Something like this?  Jesse, does it fix your issue?
Keywords: feature
Whiteboard: [gfx-noted]
Assignee: nobody → milan
Attachment #8764712 - Attachment is obsolete: true
Attachment #8771550 - Flags: review?(nfroyd)
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`.
(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.
Flags: needinfo?(jwalden+bmo)
OS: Unspecified → All
Attachment #8771550 - Attachment is obsolete: true
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/
Attachment #8771550 - Attachment description: Bug 1281702: Add MOZ_ASSERT with a third parameter, a non-string-literal detailed message, and use it in the graphics asserts. → Bug 1281702: Report the actual error message, rather than a generic one from the graphics logger.
Attachment #8771550 - Attachment is obsolete: false
Attachment #8771550 - Flags: review?(mchang)
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?
Flags: needinfo?(jruderman)
It's the assert message that will be different - the crash message "doesn't matter" (e.g., it was empty before.)
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
Attachment #8771550 - Flags: review?(mchang) → review+
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f45d79f6bc7
Report the actual error message, rather than a generic one from the graphics logger. r=mchang
https://hg.mozilla.org/mozilla-central/rev/3f45d79f6bc7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Flags: needinfo?(jruderman)
You need to log in before you can comment on or make changes to this bug.