Closed Bug 1325895 Opened 9 years ago Closed 7 years ago

treat rust error!()s like NS_ASSERTIONs wrt test failures

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: heycam, Assigned: shinglyu)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [Stylo])

Error messages printed out with error!() feel like a similar kind of severity to NS_ASSERTION, i.e. non-fatal, but relatively serious issues. It would be good to have these errors cause test failures in the same way that NS_ASSERTION does.
Blocks: stylo
Shing, can you add this to your list?
Assignee: nobody → slyu
Blocks: stylo-tooling
No longer blocks: stylo
Flags: needinfo?(slyu)
Summary: stylo: treat rust error!()s like NS_ASSERTIONs wrt test failures → treat rust error!()s like NS_ASSERTIONs wrt test failures
Whiteboard: [Stylo]
Got it!
Flags: needinfo?(slyu)
heycam, Do you know any existing test that triggers either NS_ASSERTION or error!()? I'm looking for a way to reproduce it without inserting deliberate crashes.
Flags: needinfo?(cam)
Hmm, well it looks like all the error!() calls under components/style/ are gone. Maybe the underlying problems have been fixed. There are still two under ports/geckolib/, though, related to presentation attributes. Manish, is there a way to trigger either of those two error!() calls at the moment? Many tests trigger NS_ASSERTIONs, e.g. any of the ones with "non-fatal assertion" bugs filed, like https://bugzilla.mozilla.org/show_bug.cgi?id=1324636.
Flags: needinfo?(cam) → needinfo?(manishearth)
You can trigger them with an unsupported pres attr (`background` on table cells and <body>, for example).
Flags: needinfo?(manishearth)
I replaced most of the existing error! calls with warn!() in bug 1339176. We should probably add a sanity FFI function called Servo_EmitErrorForTesting, add an API called Components.utils.emitServoErrorForTesting() that invokes it, and then use SimpleTest.expectAssertions(1, 1) in a mochitest to make sure our hooks do the right thing.
I can't reproduce an error!() using Manish's suggestion. I'll go add a test API and mochitest for that.
Summary: treat rust error!()s like NS_ASSERTIONs wrt test failures → stylo: treat rust error!()s like NS_ASSERTIONs wrt test failures
Summary: stylo: treat rust error!()s like NS_ASSERTIONs wrt test failures → treat rust error!()s like NS_ASSERTIONs wrt test failures
(In reply to Shing Lyu [:shinglyu] from comment #7) > I can't reproduce an error!() using Manish's suggestion. I'll go add a test > API and mochitest for that. If you haven't already gotten there, you ought to be able to copy what I did for nsIDebug2::rustPanic: https://dxr.mozilla.org/mozilla-central/rev/aff336ac161daa3ea350e59a288963edbd58ed39/xpcom/base/nsIDebug2.idl#83 https://dxr.mozilla.org/mozilla-central/rev/aff336ac161daa3ea350e59a288963edbd58ed39/xpcom/base/nsDebugImpl.cpp#154 https://dxr.mozilla.org/mozilla-central/rev/aff336ac161daa3ea350e59a288963edbd58ed39/toolkit/library/rust/shared/lib.rs#34
There are no longer any error!()s in style system code at all (except for one in a Servo-specific codepath) so I'm just going to WONTFIX this. (Hi Shing!)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.