Closed Bug 1330513 Opened 7 years ago Closed 7 years ago

Null-deref in [@ GetUnit] with Animation options composite: add and fill: forwards

Categories

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

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- fixed

People

(Reporter: truber, Assigned: hiro)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-nullptr, testcase)

Attachments

(3 files)

testcase.html
148 bytes, text/html
Details
log.txt
8.32 KB, text/plain
Details
Bug 1330513- Check that mCSSValueList is null in case of filter and shadow in StyleAnimationValue::Add().
59 bytes, text/x-review-board-request
boris
: review+
Details
Attached file testcase.html — 
The attached testcase causes a nullptr dereference in mozilla-central rev 63ad56438630.

==12848==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f9f9b3c2495 bp 0x7ffe5b212630 sp 0x7ffe5b212540 T0)
    #0 0x7f9f9b3c2494 in GetUnit /home/worker/workspace/build/src/layout/style/nsCSSValue.h:629:38
    #1 0x7f9f9b3c2494 in mozilla::StyleAnimationValue::Add(nsCSSPropertyID, mozilla::StyleAnimationValue const&, mozilla::StyleAnimationValue&&) /home/worker/workspace/build/src/layout/style/StyleAnimationValue.cpp:787
    #2 0x7f9f973837c0 in mozilla::dom::KeyframeEffectReadOnly::CompositeValue(nsCSSPropertyID, mozilla::StyleAnimationValue const&, mozilla::StyleAnimationValue const&, mozilla::dom::CompositeOperation) /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:333:14
    #3 0x7f9f97383e51 in mozilla::dom::KeyframeEffectReadOnly::CompositeValue(nsCSSPropertyID, RefPtr<mozilla::AnimValuesStyleRule> const&, mozilla::StyleAnimationValue const&, mozilla::dom::CompositeOperation) /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:395:10
    #4 0x7f9f9735a050 in mozilla::dom::KeyframeEffectReadOnly::ComposeStyle(RefPtr<mozilla::AnimValuesStyleRule>&, nsCSSPropertyIDSet const&) /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:524:7
Attached file log.txt — 

    
    

    
  
Hmm, the StyleAnimationValue for the filter has a valid unit, eUnit_Filter, but mCSSValueList is nullptr.
Flags: needinfo?(hikezoe)
See Also:  → 1330190

    
  
Blocks: 1311620
Flags: needinfo?(hikezoe)

    
  
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED

    
  
Priority: -- → P1

    
    

    
      
  


  

    
    

    
  
Comment on attachment 8826058 [details]
Bug 1330513- Check that mCSSValueList is null in case of filter and shadow in StyleAnimationValue::Add().

https://reviewboard.mozilla.org/r/104102/#review104840

::: dom/animation/test/crashtests/1330513-1.html:5
(Diff revision 1)
> +<!DOCTYPE html>
> +<html>
> +<body id=a></body>
> +<script>
> +a.animate([{"filter": "grayscale(28%)"}], {fill:"forwards", composite:"add"});

Don't we need to call document.getElementById('a') first to get the body element?

::: dom/animation/test/crashtests/crashtests.list:19
(Diff revision 1)
>  pref(dom.animations-api.core.enabled,true) load 1304886-1.html
>  pref(dom.animations-api.core.enabled,true) load 1322382-1.html
>  skip-if(stylo) pref(dom.animations-api.core.enabled,true) load 1322291-1.html # bug 1323733
>  asserts-if(stylo,0-5) pref(dom.animations-api.core.enabled,true) load 1323114-1.html # bug 1324690
>  asserts-if(stylo,0-5) pref(dom.animations-api.core.enabled,true) load 1323114-2.html # bug 1324690
> +skip-if(stylo) pref(dom.animations-api.core.enabled,true) load 1330513-1.html # bug 1311257

r/1311257/1330513

        Attachment #8826058 -
        Flags: review?(boris.chiou) → review+

    
    

    
  
(In reply to Boris Chiou [:boris] from comment #5)
> Comment on attachment 8826058 [details]
> Bug 1330513- Check that mCSSValueList is null in case of filter and shadow
> in StyleAnimationValue::Add().
> 
> https://reviewboard.mozilla.org/r/104102/#review104840
> 
> ::: dom/animation/test/crashtests/1330513-1.html:5
> (Diff revision 1)
> > +<!DOCTYPE html>
> > +<html>
> > +<body id=a></body>
> > +<script>
> > +a.animate([{"filter": "grayscale(28%)"}], {fill:"forwards", composite:"add"});
> 
> Don't we need to call document.getElementById('a') first to get the body
> element?

It's named access on the window object.
https://html.spec.whatwg.org/#named-access-on-the-window-object

But I will replace it with getElementById().

> ::: dom/animation/test/crashtests/crashtests.list:19
> (Diff revision 1)
> >  pref(dom.animations-api.core.enabled,true) load 1304886-1.html
> >  pref(dom.animations-api.core.enabled,true) load 1322382-1.html
> >  skip-if(stylo) pref(dom.animations-api.core.enabled,true) load 1322291-1.html # bug 1323733
> >  asserts-if(stylo,0-5) pref(dom.animations-api.core.enabled,true) load 1323114-1.html # bug 1324690
> >  asserts-if(stylo,0-5) pref(dom.animations-api.core.enabled,true) load 1323114-2.html # bug 1324690
> > +skip-if(stylo) pref(dom.animations-api.core.enabled,true) load 1330513-1.html # bug 1311257
> 
> r/1311257/1330513

The comment is for skip-if(stylo), that means we will drop the skip-if once bug 1311257 is fixed.

Thanks for the quick review!

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by hiikezoe@mozilla-japan.org:
https://hg.mozilla.org/integration/autoland/rev/0a4f47afa494
Check that mCSSValueList is null in case of filter and shadow in StyleAnimationValue::Add(). r=boris

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/0a4f47afa494
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox53:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: