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

RESOLVED FIXED in mozilla13

Status

()

Core
CSS Parsing and Computation
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: nonsensickle)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla13
assertion, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 599183 [details]
testcase (asserts fatally when loaded)

###!!! 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.
(Reporter)

Comment 1

6 years ago
Created attachment 599195 [details]
stack trace

Updated

6 years ago
No longer blocks: 522607

Updated

6 years ago
Blocks: 522607
(Assignee)

Updated

6 years ago
Assignee: nobody → lsumar
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 729409
(Assignee)

Comment 4

6 years ago
Created attachment 599776 [details] [diff] [review]
fix

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)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 7

6 years ago
Created attachment 599807 [details] [diff] [review]
crashtests

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+
(Assignee)

Comment 9

6 years ago
Created attachment 599810 [details] [diff] [review]
fix


(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+
(Reporter)

Comment 10

6 years ago
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 ;)
(Assignee)

Comment 11

6 years ago
Created attachment 599814 [details] [diff] [review]
crashtests

(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+
(Assignee)

Comment 12

6 years ago
Created attachment 599818 [details] [diff] [review]
fix and crashtests

(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?
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
OS: Mac OS X → All
(Assignee)

Updated

6 years ago
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/adee75f6c428
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Duplicate of this bug: 730327
Attachment #599818 - Flags: checkin?
You need to log in before you can comment on or make changes to this bug.