Closed Bug 1269992 Opened 6 years ago Closed 6 years ago

mochitests for clip-path basic shapes are interrupted by an exception

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

As noted in bug 1269990 comment 0, if you enable layout.css.clip-path-shapes.enabled, the mochitest test_transitions_per_property.html gets interrupted partway through, due to throwing an uncaught exception.

jwatt mentioned in bug 1269990 comment 2 "Yeah, this is what I filed bug 1266316 for" but I think he's referring to the earlier test failures, rather than the exception.

Filing this bug for the exception (which we can fix independently of the test failures).
Sorry, bug links in comment 0 were meant to be for bug 1266868. (bug 1266868 comment 0, bug 1266868 comment 2)
Patch coming up.

Sample test output *before* applying this patch:
{
22 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Clip-path property is circle(200px at 50% 50%) expected values of circle,200px,at,50%,50% 
23 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Function should have close-paren 
24 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Clip-path property is ellipse(200px 200px at 50% 50%) expected values of ellipse,200px,200px,at,50%,50% 
25 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | unexpected reference box found 
    test_clip_path_equals@layout/style/test/test_transitions_per_property.html:1158:7
    test_clip_path_transition@layout/style/test/test_transitions_per_property.html:1284:8
    @layout/style/test/test_transitions_per_property.html:802:5
26 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | Reference boxes should match - got "border-box", expected 200px,at,200px,200px
    SimpleTest.is@SimpleTest/SimpleTest.js:268:5
    test_clip_path_equals@layout/style/test/test_transitions_per_property.html:1161:5
    test_clip_path_transition@layout/style/test/test_transitions_per_property.html:1284:8
    @layout/style/test/test_transitions_per_property.html:802:5
27 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Function should have close-paren 
28 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | uncaught exception - TypeError: expectedList[1] is undefined at test_clip_path_equals@http://mochi.test:8888/tests/layout/style/test/test_transitions_per_property.html:1197:1
test_clip_path_transition@http://mochi.test:8888/tests/layout/style/test/test_transitions_per_property.html:1284:8
@http://mochi.test:8888/tests/layout/style/test/test_transitions_per_property.html:802:5

    simpletestOnerror@SimpleTest/SimpleTest.js:1563:11
    OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1543:1
JavaScript error: http://mochi.test:8888/tests/layout/style/test/test_transitions_per_property.html, line 1197: TypeError: expectedList[1] is undefined
}
(At this point, no further tests are run, because we've thrown an uncaught exception.)



Sample test output *after* applying this patch:
{
25 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Function should have close-paren 
26 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Clip-path property is circle(200px at 50% 50%) expected values of circle,200px,at,50%,50% 
27 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Function should have close-paren 
28 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Clip-path property is ellipse(200px 200px at 50% 50%) expected values of ellipse,200px,200px,at,50%,50% 
29 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | reference box shouldn't have anything after it 
30 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | unexpected reference box found 
    test_clip_path_equals@layout/style/test/test_transitions_per_property.html:1165:7
    test_clip_path_transition@layout/style/test/test_transitions_per_property.html:1292:8
    @layout/style/test/test_transitions_per_property.html:802:5
31 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Function should have close-paren 
32 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Clip-path property is circle(200px at 200px 200px) border-box expected values of circle,200px,at,200px,200px 
33 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | reference box shouldn't have anything after it 
34 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | unexpected reference box found 
    test_clip_path_equals@layout/style/test/test_transitions_per_property.html:1165:7
    test_clip_path_transition@layout/style/test/test_transitions_per_property.html:1292:8
    @layout/style/test/test_transitions_per_property.html:802:5
35 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Function should have close-paren 
}
(At this point, tests continue to run.  We eventually trigger the assertion mentioned in bug 1269990, but let's handle that over there.)
Attached patch fix v1Splinter Review
The current code has several mysterious pop() calls, which I didn't initially understand.

This patch:
 - adds documentation to explain them
 - adds a check that we're actually OK to perform the first pop (which just throws away a value, because we're expecting that value to be the empty string as noted in my patch; now that expectation is enforced/explained)
 - adds logic to prevent the problematic pop (from expectedList) when expectedList does not actually contain a reference box.

(So: without this patch, we accidentally clear out |expectedList| too early and throw an exception a bit further down, because we're trying to access an entry that we already popped away by accident. With this patch, we'll no longer pop that entry away by accident, so we don't throw this exception.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8748497 - Flags: review?(jwatt)
Comment on attachment 8748497 [details] [diff] [review]
fix v1

Much clearer, thanks, Daniel!
Attachment #8748497 - Flags: review?(jwatt) → review+
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/805453c94735
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Blocks: 1266570
Blocks: basic-shape
You need to log in before you can comment on or make changes to this bug.