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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

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).

Comment 1

[Daniel Holbert [:dholbert]]

Sorry, bug links in comment 0 were meant to be for bug 1266868. (bug 1266868 comment 0, bug 1266868 comment 2)

Comment 2

[Daniel Holbert [:dholbert]]

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.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

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.)

Comment 4

[Jonathan Watt [:jwatt]]

Comment on attachment 8748497 [details] [diff] [review]
fix v1

Much clearer, thanks, Daniel!

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/805453c94735

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/805453c94735