Closed Bug 633069 Opened 13 years ago Closed 13 years ago

"Assertion failure: JS_THREAD_DATA(cx)->profilingCompartment == NULL", including in test_form_submission_cap2.html

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- .x+

People

(Reporter: jruderman, Assigned: billm)

References

Details

(4 keywords, Whiteboard: [fixed-in-tracemonkey])

Attachments

(3 files)

testcase (asserts fatally when loaded)
683 bytes, text/html
Details
stack trace
54.11 KB, text/plain
Details
fix
12.10 KB, patch
gal
: review+
Details | Diff | Splinter Review 
This testcase triggers an assertion in MonitorTracePoint:

Assertion failure: JS_THREAD_DATA(cx)->profilingCompartment == NULL, at js/src/jstracer.cpp:17038

This assertion was added as part of a large patch:
changeset:   ee3a4f429942
user:        Bill McCloskey
date:        Tue Feb 01 10:18:06 2011 -0800
summary:     Bug 623297 - Make JS_TRACE_MONITOR more robust by distinguishing callers (r=gal)

Security-sensitive for now because bug 623297 was security-sensitive.
Attached file stack trace — 
blocking2.0: --- → ?

    
    

    
  
Thanks, Jesse! I think I know what this is.
Assignee: general → wmccloskey
blocking2.0: ? → .x

    
    

    
  
Bill, we take a patch for this I think if you can fix is quickly.

    
    

    
  
What's the security implications of this assert? Compartment leakage? exploitable crash?

    
    

    
  
I'm 90% sure that this is not security sensitive. I don't think it will lead to a crash or any sort of compartment problems. We are breaking an invariant that I had hoped to preserve (that we don't do nested profiling), but I don't think we depend on this invariant for correctness.

I'd rather not take out the assertion, since it's much harder to reason about control flow in the tracer if we can't rely on the invariant. There's a simple one-line fix that causes the invariant to be preserved. However, the fix regresses SunSpider. I'm working on figuring out how to fix it without messing up SS.

Bottom line: shouldn't block; probably not security sensitive.

    
    

    
  
Opening this up based on the above comment, and discussion with Bill.
Group: core-security

    
    

    
  
http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1297477875.1297478604.6242.gz

Pretty much more or less similarish stack leading up to the same assertion, I'm hoping it'll manage to stay a very rarely intermittent while running test_form_submission_cap2.html, so I won't have to take over your bug and fill it with tbplbot spam :)

    
    

    
  
Sorry, looks like someone made it pretty common starting yesterday, here comes the spam.

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1297556299.1297556886.31142.gz
Rev3 MacOSX Snow Leopard 10.6.2 tracemonkey debug test mochitests-5/5 on 2011/02/12 16:18:19
s: talos-r3-snow-026

7691 INFO TEST-PASS | /tests/toolkit/components/satchel/test/test_form_submission_cap2.html | checking unsaved value 60
Assertion failure: JS_THREAD_DATA(cx)->profilingCompartment == NULL, at /builds/slave/tm-osx64-dbg/build/js/src/jstracer.cpp:6500
Blocks: 438871
Summary: "Assertion failure: JS_THREAD_DATA(cx)->profilingCompartment == NULL" → "Assertion failure: JS_THREAD_DATA(cx)->profilingCompartment == NULL", including in test_form_submission_cap2.html
Whiteboard: [orange]

    
    

    
      
  

    
    

    
  
And that first one being

Rev3 MacOSX Leopard 10.5.8 tracemonkey debug test mochitests-5/5 on 2011/02/11 18:31:15
s: talos-r3-leopard-011

apparently made it pretty common only on OS X.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix
        — 
          — Splinter Review
      

    


  
This fixes the fuzzbug. I'm pretty sure it should also fix the orange.

The problem is that we were asserting that we don't profile while we're recording or on trace. However, the lack of a profiler abort in JaegerShot causes us to violate this property. The easy fix would be to stick the abort in JaegerShot. However, that regresses our SunSpider score.

It seems like we get the best performance if we allow outer profiler invocations to proceed and to abort inner profiler invocations (the opposite of what we do for recording). This is what this patch does. I've eliminated the profiler abort in Interpret. Instead, when we're about to run the profiler, the patch checks if we're already profiling and doesn't profile if we are.

I wasn't sure how to deal with the case of recording or running on trace while profiling. These seem safe, but to be conservative, the patch aborts profiling whenever we're about to do either of these things. We can loosen these restrictions later if they hurt performance. (They are already looser than what is happening now on trunk.)

Performance on all the usual benchmarks seems to be unaffected.

        Attachment #512214 -
        Flags: review?(dvander)

        Attachment #512214 -
        Flags: review?(dvander) → review?(gal)

    
  

        Attachment #512214 -
        Flags: review?(gal) → review+

    
    

    
  
http://hg.mozilla.org/tracemonkey/rev/0d78f2804085
Whiteboard: [orange] → [orange][fixed-in-tracemonkey]

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Those were all from earlier in the day, we must have been backed up and suddenly a whole lot of run finished.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED

    
  
Depends on: 634436
Whiteboard: [orange][fixed-in-tracemonkey] → [fixed-in-tracemonkey]

    
    

    
  
Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite-

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: