Closed Bug 729126 Opened 13 years ago Closed 13 years ago

"ABORT: unexpected unit 1, from 1 and 1"

Categories

(Core :: CSS Parsing and Computation, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla13

People

(Reporter: jruderman, Assigned: nonsensickle)

References

Details

(Keywords: assertion, regression, testcase)

Attachments

(2 files, 5 obsolete files)

stack trace
(deleted), text/plain
Details
fix and crashtests
(deleted), patch
nonsensickle
: review+
Details | Diff | Splinter Review
Attached file testcase (asserts fatally when loaded) (obsolete) (deleted) —
###!!! ABORT: unexpected unit 1, from 1 and 1: 'false', file layout/style/nsStyleAnimation.cpp, line 888 This assertion is part of code added in bug 522607. I guess '1' refers to the enum value eCSSUnit_Auto.
Attached file stack trace (deleted) —
No longer blocks: 522607
Blocks: 522607
Assignee: nobody → lsumar
From looking at my patch I think this is triggered by the following hunk. https://bugzilla.mozilla.org/attachment.cgi?id=598965&action=diff#a/layout/style/nsStyleAnimation.cpp_sec7 I will test and come back.
Attached patch fix (obsolete) (deleted) — — Splinter Review
This patch fixes the issue on my machine. bz, is a crashtest needed? This was a silly refactoring mistake on my part, see comment #2 link. I guess a crashtest wouldn't hurt...
Attachment #599776 - Flags: review?(bzbarsky)
Attachment #599776 - Flags: review?(bzbarsky) → review?(dbaron)
Please do add both of Jesse's test cases (from here and bug 729409) as crashtests.
Comment on attachment 599776 [details] [diff] [review] fix ># HG changeset patch ># Date 1329946763 -46800 ># User Lazar Sumar <lsumar@mozilla.com> ># Parent e61a169463c67e695ad17983fdf73c251e960da4 >Bug 729126 - Regressioin fix This could use a better commit message (more than just the spelling fix). How about: Make NS_ABORT_IF_FALSE fire on failure for only some callers of AddCSSValuePixelPercentCalc (the existing ones, and not the new ones added in bug 522607). r=dbaron with that or something like it
Attachment #599776 - Flags: review?(dbaron) → review+
Attached patch crashtests (obsolete) (deleted) — — Splinter Review
Crashtests added. Once reviewed, I'll merge the two patches.
Attachment #599183 - Attachment is obsolete: true
Attachment #599807 - Flags: review?(dbaron)
Comment on attachment 599807 [details] [diff] [review] crashtests It might make more sense to just do: <body style="..."> <script> var body = document.body; /* flush */ getComputedStyle(body, "").backgroundSize; body.style.backgroundSize = 'contain'; /* flush */ getComputedStyle(body, "").backgroundSize; </script> without any messing with onload handlers. Forcing the flush should guarantee that the transition happens, and avoid having to worry about whether the test is guaranteed to trigger a transition or whether it's guaranteed to happen before we leave the page. (And likewise for the first test.) r=dbaron with that approach
Attachment #599807 - Flags: review?(dbaron) → review+
Attached patch fix (obsolete) (deleted) — — Splinter Review
(In reply to David Baron [:dbaron] from comment #6) > Comment on attachment 599776 [details] [diff] [review] > fix > > ># HG changeset patch > ># Date 1329946763 -46800 > ># User Lazar Sumar <lsumar@mozilla.com> > ># Parent e61a169463c67e695ad17983fdf73c251e960da4 > >Bug 729126 - Regressioin fix > > This could use a better commit message (more than just the spelling fix). > How about: > > Make NS_ABORT_IF_FALSE fire on failure for only some callers of > AddCSSValuePixelPercentCalc (the existing ones, and not the new ones added > in bug 522607). > > > r=dbaron with that or something like it Message changed.
Attachment #599776 - Attachment is obsolete: true
Attachment #599810 - Flags: review+
Comment on attachment 599810 [details] [diff] [review] fix > Make NS_ABORT_IF_FALSE fire on failure for only some callers of AddCSSValuePixelPercentCalc (the existing ones, and not the new ones added in bug 522607). r=dbaron Please keep the bug number for *this* bug in your message ;)
Attached patch crashtests (obsolete) (deleted) — — Splinter Review
(In reply to David Baron [:dbaron] from comment #8) > Comment on attachment 599807 [details] [diff] [review] > crashtests > > It might make more sense to just do: > > <body style="..."> > <script> > var body = document.body; > /* flush */ getComputedStyle(body, "").backgroundSize; > body.style.backgroundSize = 'contain'; > /* flush */ getComputedStyle(body, "").backgroundSize; > </script> > > without any messing with onload handlers. Forcing the flush should > guarantee that the transition happens, and avoid having to worry about > whether the test is guaranteed to trigger a transition or whether it's > guaranteed to happen before we leave the page. (And likewise for the first > test.) > > r=dbaron with that approach Done.
Attachment #599807 - Attachment is obsolete: true
Attachment #599814 - Flags: review+
Attached patch fix and crashtests (deleted) — — Splinter Review
(In reply to Jesse Ruderman from comment #10) > Comment on attachment 599810 [details] [diff] [review] > fix > > > Make NS_ABORT_IF_FALSE fire on failure for only some callers of AddCSSValuePixelPercentCalc (the existing ones, and not the new ones added in bug 522607). r=dbaron > > Please keep the bug number for *this* bug in your message ;) Done.
Attachment #599810 - Attachment is obsolete: true
Attachment #599814 - Attachment is obsolete: true
Attachment #599818 - Flags: review+
Attachment #599818 - Flags: checkin?
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/adee75f6c428
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Attachment #599818 - Flags: checkin?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: