Closed Bug 1290535 Opened 7 years ago Closed 7 years ago

Assertion failure: pacedValues[propIdx].mProperty == prop (Property mismatch), at src/dom/animation/KeyframeUtils.cpp:1557

Categories

(Core :: DOM: Animation, defect, P3)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: truber, Assigned: boris)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

testcase.html
316 bytes, text/html
Details
stack
22.12 KB, text/plain
Details
Bug 1290535 - Part 1: Sort paced subproperties before calculation.
58 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1290535 - Part 2: Add test.
58 bytes, text/x-review-board-request
hiro
: review+
Details
Attached file testcase.html — 
Assertion failure: pacedValues[propIdx].mProperty == prop (Property mismatch), at src/dom/animation/KeyframeUtils.cpp:1557

Found in m-c-1469786218-debug

Stack:
    #0 0x7f98a1586b42 in mozilla::GetCumulativeDistances(nsTArray<nsTArray<mozilla::PropertyStyleAnimationValuePair> > const&, nsCSSProperty) src/dom/animation/KeyframeUtils.cpp:1556:11
    #1 0x7f98a157e42b in mozilla::KeyframeUtils::ApplySpacing(nsTArray<mozilla::Keyframe>&, mozilla::SpacingMode, nsCSSProperty, nsTArray<nsTArray<mozilla::PropertyStyleAnimationValuePair> >&) src/dom/animation/KeyframeUtils.cpp:489:27
    #2 0x7f98a1574eb7 in mozilla::dom::KeyframeEffectReadOnly::UpdateProperties(nsStyleContext*) src/dom/animation/KeyframeEffect.cpp:554:7
    #3 0x7f98a157d647 in mozilla::dom::KeyframeEffectReadOnly::SetKeyframes(nsTArray<mozilla::Keyframe>&&, nsStyleContext*) src/dom/animation/KeyframeEffect.cpp:489:5
    #4 0x7f98a157cead in mozilla::dom::KeyframeEffectReadOnly::SetKeyframes(JSContext*, JS::Handle<JSObject*>, mozilla::ErrorResult&) src/dom/animation/KeyframeEffect.cpp:466:3
Attached file stack — 

    
  
Assignee: nobody → boris.chiou
Priority: -- → P3

    
  
Status: NEW → ASSIGNED

    
    

    
  
I think the root cause is that the subproperties of this shorthand property are mismatch.
Keyframe[0] added border-top-right-radius first, and then added the shorthand property, so the order is not correct. Looks like we have to sort the subproperties after calculating the computed values.

Keyframe[0]: border-top-right-radius
Keyframe[0]: border-top-left-radius
Keyframe[0]: border-bottom-right-radius
Keyframe[0]: border-bottom-left-radius
Keyframe[1]: border-top-left-radius  // mismatch
Keyframe[1]: border-top-right-radius // mismatch
Keyframe[1]: border-bottom-right-radius
Keyframe[1]: border-bottom-left-radius

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
  
Comment on attachment 8782797 [details]
Bug 1290535 - Part 2: Add test.

https://reviewboard.mozilla.org/r/72842/#review70574

Don't we need to add another test to check that the shorthand properties surely sorted?  I mean that we check each keyframe has correct property.

        Attachment #8782797 -
        Flags: review?(hiikezoe) → review+

    
    

    
  
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)

> Don't we need to add another test to check that the shorthand properties
> surely sorted?  I mean that we check each keyframe has correct property.

OK. I can add one more mochitest for this (into dom/animations/tests/mozilla).
), to make sure the order is correct. Thanks.

    
    

    
  
Comment on attachment 8782796 [details]
Bug 1290535 - Part 1: Sort paced subproperties before calculation.

https://reviewboard.mozilla.org/r/72840/#review70576

::: dom/animation/KeyframeUtils.cpp:311
(Diff revision 1)
>    {
>      return aLhs.mComputedOffset < aRhs.mComputedOffset;
>    }
>  };
>  
> +class ComputedKeyframeValuesComparator

Can't we use TPropertyPriorityComparator template somehow instead of adding this comparator?

    
    

    
  
Comment on attachment 8782796 [details]
Bug 1290535 - Part 1: Sort paced subproperties before calculation.

https://reviewboard.mozilla.org/r/72840/#review70576

> Can't we use TPropertyPriorityComparator template somehow instead of adding this comparator?

Oh yes. I will update this soon. Thanks.

    
    

    
  
(In reply to Boris Chiou [:boris] from comment #6)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> 
> > Don't we need to add another test to check that the shorthand properties
> > surely sorted?  I mean that we check each keyframe has correct property.
> 
> OK. I can add one more mochitest for this (into
> dom/animations/tests/mozilla).
> ), to make sure the order is correct. Thanks.

But in part 1, we sort the longhand components only for the internal usage, i.e. we cannot get the sorted results from the keyframes, so the properties of the keyframes are still unchanged. Maybe there is no way to test the sorted properties. The only way is to test the computed offsets for paced spacing in this case. If the result is correct, I think the sorted result is also correct.

    
    

    
  
(In reply to Boris Chiou [:boris] from comment #9)
> (In reply to Boris Chiou [:boris] from comment #6)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> > 
> > > Don't we need to add another test to check that the shorthand properties
> > > surely sorted?  I mean that we check each keyframe has correct property.
> > 
> > OK. I can add one more mochitest for this (into
> > dom/animations/tests/mozilla).
> > ), to make sure the order is correct. Thanks.
> 
> But in part 1, we sort the longhand components only for the internal usage,
> i.e. we cannot get the sorted results from the keyframes, so the properties
> of the keyframes are still unchanged. Maybe there is no way to test the
> sorted properties. The only way is to test the computed offsets for paced
> spacing in this case. If the result is correct, I think the sorted result is
> also correct.

Does KeyframeEffectReadOnly.getProperties() help it?
http://searchfox.org/mozilla-central/rev/ae78ab94fadabc89fc6258d03c4a1a70f763f43a/dom/webidl/KeyframeEffect.webidl#64

    
    

    
  
(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> Does KeyframeEffectReadOnly.getProperties() help it?
> http://searchfox.org/mozilla-central/rev/
> ae78ab94fadabc89fc6258d03c4a1a70f763f43a/dom/webidl/KeyframeEffect.webidl#64

In part 1, I only sort pacedValues, which is a local variable [1], and what we need is the cumulative distances [2], which is a double array indexed by the keyframe index. So the sorting doesn't affect the property-value pairs of each keyframe. It only affects the results of cumulative distances, and so the computed offsets of keyframes. Therefore, I think testing the computed offsets may be enough.

[1] http://searchfox.org/mozilla-central/rev/ae78ab94fadabc89fc6258d03c4a1a70f763f43a/dom/animation/KeyframeUtils.cpp#1532
[2] http://searchfox.org/mozilla-central/rev/ae78ab94fadabc89fc6258d03c4a1a70f763f43a/dom/animation/KeyframeUtils.cpp#1524

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8782797 [details]
Bug 1290535 - Part 2: Add test.

I added one more test. Could you please re-review this patch? Thanks.

        Attachment #8782797 -
        Flags: review+ → review?(hiikezoe)

    
    

    
  
(In reply to Boris Chiou [:boris] from comment #11)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> > Does KeyframeEffectReadOnly.getProperties() help it?
> > http://searchfox.org/mozilla-central/rev/
> > ae78ab94fadabc89fc6258d03c4a1a70f763f43a/dom/webidl/KeyframeEffect.webidl#64
> 
> In part 1, I only sort pacedValues, which is a local variable [1], and what
> we need is the cumulative distances [2], which is a double array indexed by
> the keyframe index. So the sorting doesn't affect the property-value pairs
> of each keyframe. It only affects the results of cumulative distances, and
> so the computed offsets of keyframes. Therefore, I think testing the
> computed offsets may be enough.

Ah, thanks.  I was misunderstanding.

    
    

    
  
Comment on attachment 8782797 [details]
Bug 1290535 - Part 2: Add test.

https://reviewboard.mozilla.org/r/72842/#review70590

::: dom/animation/test/mozilla/file_spacing_property_order.html:21
(Diff revision 2)
> +                           { borderRadius: "50%" } ],
> +                         { spacing:"paced(border-radius)" });
> +
> +  var frames = anim.effect.getKeyframes();
> +  var dist = [ 0,
> +               Math.sqrt(20 * 20 + 20 * 20 + 40 * 40 + 50 * 50),

It would be more clear like this;

20 * 20 + (30 - 10) * (30 - 10) + 40 * 40 + 50 * 50

::: dom/animation/test/mozilla/file_spacing_property_order.html:22
(Diff revision 2)
> +                         { spacing:"paced(border-radius)" });
> +
> +  var frames = anim.effect.getKeyframes();
> +  var dist = [ 0,
> +               Math.sqrt(20 * 20 + 20 * 20 + 40 * 40 + 50 * 50),
> +               Math.sqrt(30 * 30 + 20 * 20 + 10 * 10 + 0) ];

And this 
(50 - 20) * (50 - 20) ...

        Attachment #8782797 -
        Flags: review?(hiikezoe) → review+

    
    

    
  
Comment on attachment 8782796 [details]
Bug 1290535 - Part 1: Sort paced subproperties before calculation.

https://reviewboard.mozilla.org/r/72840/#review70594

<p>Nice!</p>

        Attachment #8782796 -
        Flags: review?(hiikezoe) → review+

    
    

    
  
(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)
> Comment on attachment 8782796 [details]
> Bug 1290535 - Part 1: Sort paced subproperties before calculation.
> 
> https://reviewboard.mozilla.org/r/72840/#review70594
> 
> <p>Nice!</p>

It's hard to me to use MozReview.  Where did the <p> come from?

    
    

    
  
(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #17)
> > Comment on attachment 8782796 [details]
> > Bug 1290535 - Part 1: Sort paced subproperties before calculation.
> > 
> > https://reviewboard.mozilla.org/r/72840/#review70594
> > 
> > <p>Nice!</p>
> 
> It's hard to me to use MozReview.  Where did the <p> come from?

Haha. I don't know. It sometimes adds some weird characters. Thanks for your review.

    
    

    
      
  

    
    

    
  
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a3e0d775ec1
Part 1: Sort paced subproperties before calculation. r=hiro
https://hg.mozilla.org/integration/autoland/rev/fb9dbacb7f8b
Part 2: Add test. r=hiro

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/9a3e0d775ec1
https://hg.mozilla.org/mozilla-central/rev/fb9dbacb7f8b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox51:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

          You need to log in
          before you can comment on or make changes to this bug.