Closed Bug 1401992 Opened 7 years ago Closed 7 years ago

stylo: Assertion failure: !mInStyleRefresh, at /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:1228

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: jkratzer, Assigned: bholley)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

trigger.html
650 bytes, text/html
Details
Minidump stack trace
95.67 KB, text/plain
Details
Don't cross anonymous boundaries when finding the fieldset. v1
2.01 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
revisions for uplift (same as comment 22)
226 bytes, text/plain
Sylvestre
: approval-mozilla-beta+
Details
Attached file trigger.html — 
Testcase found while fuzzing mozilla-central rev 47f7b6c64265.  Please note that this testcase must be served up by a web server in order to reproduce.
Flags: in-testsuite?
Attached file Minidump stack trace — 

    
  
Priority: -- → P2
Summary: Assertion failure: !mInStyleRefresh, at /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:1228 → stylo: Assertion failure: !mInStyleRefresh, at /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:1228

    
    

    
  
This looks like anonymous content unbinding stuff, I suspect bug 1397983.
Flags: needinfo?(emilio)

    
  
Assignee: nobody → emilio

    
    

    
  
Requesting tracking on all outstanding p2 stylo bugs.

          tracking-firefox57:
          --- → ?

    
    

    
  
I haven't been able to repro this, neither loading the test-case from bugzilla, nor manually serving from python -m http.server.

Also, I'm not sure how this is possible, given this UpdateFieldSet call[1] isn't supposed to notify.

[1]: http://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/dom/html/nsGenericHTMLElement.cpp#1942

    
    

    
  
Any chance to get some help reproducing this?
Flags: needinfo?(emilio) → needinfo?(jkratzer)

    
    

    
  
(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> Any chance to get some help reproducing this?

All of my testing was done on a linux64 system using the latest mc-debug build.  I've just tested and confirmed that this reproduces using the default prefs.  Also, as you mentioned, I'm starting a web server via python -m SimpleHTTPServer in the testcase directory and navigating to http://localhost:8000/trigger.html.
Flags: needinfo?(jkratzer)

    
    

    
  
(In reply to Jason Kratzer [:jkratzer] from comment #6)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> > Any chance to get some help reproducing this?
> 
> All of my testing was done on a linux64 system using the latest mc-debug
> build.  I've just tested and confirmed that this reproduces using the
> default prefs.  Also, as you mentioned, I'm starting a web server via python
> -m SimpleHTTPServer in the testcase directory and navigating to
> http://localhost:8000/trigger.html.

I'm using linux64 too, and no extra prefs either (tried stylo disabled and enabled, just in case). Not sure what the difference could be.

    
    

    
  
I don't see why there would be a difference but have you tried a TC build?  You can retrieve the latest using fuzzfetch:
https://github.com/MozillaSecurity/fuzzfetch

fuzzfetch -d

    
    

    
  
I can reproduce this. Emilio and I are discussing a fix. Thanks Jason!

    
  
Assignee: emilio → bobbyholley

    
    

    
  


  
MozReview-Commit-ID: DiywRognpqx

        Attachment #8911318 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 8911318 [details] [diff] [review]
Don't cross anonymous boundaries when finding the fieldset. v1

Please add a crashtest?  r=me

        Attachment #8911318 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #13)
> Comment on attachment 8911318 [details] [diff] [review]
> Don't cross anonymous boundaries when finding the fieldset. v1
> 
> Please add a crashtest?  r=me

I think a reftest with fieldset:valid / :invalid would be even better, since that doesn't just test the assertion.

    
    

    
  
Yes, something along the lines of Emilio's testcase:

  <!doctype html>
  <style>
  video {
    width: 100px;
    height: 100px;
    border: 10px solid purple;
  }
  fieldset {
    border: 10px solid blue;
  }
  fieldset:valid {
    border-color: green;
  }
  fieldset:invalid {
    border-color: red;
  }
  </style>
  <fieldset><video onclick="this.currentTime = -128; this.controls = !this.controls;"></fieldset>

would be good.
Status: NEW → ASSIGNED

    
    

    
  
Yes, I have both a crashtest and a reftest, I just didn't upload them. Still fiddling with them a little bit to make them work for reasons unrelated to what's being tested.

    
    

    
  
I can't get the crashtest to work properly without triggering navigation, which seems to screw up the crashtest harness in the case where we don't crash.

I'm going to call the reftest good enough here.

    
    

    
  
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/581b6ab2826a
Don't cross anonymous boundaries when finding the fieldset. r=bz
https://hg.mozilla.org/integration/autoland/rev/5bfa23832767
Reftest. r=me

          status-firefox55:
          --- → unaffected

          status-firefox56:
          --- → unaffected

          status-firefox57:
          --- → affected

          status-firefox58:
          --- → affected

          status-firefox-esr52:
          --- → unaffected
Flags: in-testsuite? → in-testsuite+

    
    

    
  
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c191b4e631f
Mark some tests as passing on android. r=me

    
    

    
  
Please request Beta approval on this when you get a chance.
Flags: needinfo?(bobbyholley)

    
    

    
  


  
Approval Request Comment
[Feature/Bug causing the regression]: Pre-existing, fuzz bug.
[User impact if declined]: Potentially incorrect rendering, potential crashes.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not really.
[Why is the change risky/not risky?]: Just adds a missing check of the sort that we have in other analogous code. The fact that it leads to various unexpected-passes on android is a good sign.
[String changes made/needed]: None
Flags: needinfo?(bobbyholley)

        Attachment #8911876 -
        Flags: approval-mozilla-beta?

    
    

    
  
Comment on attachment 8911876 [details]
revisions for uplift (same as comment 22)


Fix a crash, taking it.
Should be in 57b4

        Attachment #8911876 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

    
    

    
  
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #24)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: No
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Bobby 's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: