496824 - JS_REQUIRES_STACK violation in JS_EvaluateUCInStackFrame

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1244403638.1244403731.21809.gz

jsdbgapi.cpp: In function 'JSBool JS_EvaluateUCInStackFrame(JSContext*, JSStackFrame*, const jschar*, uintN, const char*, uintN, jsval*)':
jsdbgapi.cpp:1268: error: cannot access JS_REQUIRES_STACK variable JSContext::fp
jsdbgapi.cpp:1272: error: cannot access JS_REQUIRES_STACK variable JSContext::fp

Blame for these lines points to bug 494235.

Comment 1

[Michael Kraft [:morac]]

Attachment: Crash stack

I'm guessing this is the same problem I'm seeing even though the line number is off by one.  I'm seeing it when using Venkman.

>  /* This API requires an active fp on cx, so fp2 can't go null here. */
I'm guessing it can go null.


FAULTING_IP: 
js3250!JS_EvaluateUCInStackFrame+6f [e:\builds\moz2_slave\mozilla-central-win32-nightly\build\js\src\jsdbgapi.cpp @ 1273]
003d5d2c 8b4858          mov     ecx,dword ptr [eax+58h]

EXCEPTION_RECORD:  ffffffff -- (.exr 0xffffffffffffffff)
.exr 0xffffffffffffffff
ExceptionAddress: 003d5d2c (js3250!JS_EvaluateUCInStackFrame+0x0000006f)
   ExceptionCode: c0000005 (Access violation)
  ExceptionFlags: 00000000
NumberParameters: 2
   Parameter[0]: 00000000
   Parameter[1]: 00000058
Attempt to read from address 00000058

Comment 2

[Brendan Eich [:brendan]]

(In reply to comment #1)
> Created an attachment (id=382074) [details]
> Crash stack

Separate bug, please file it (nominate blocking), with a debug build's backtrace if possible. Thanks,

/be

Comment 3

[Benjamin Smedberg]

I'm not sure about the jsdbgapi invariants here. As far as I can tell, obtaining a JSStackFrame* can only happen through the JS_FrameIterator API. This leaves the trace. Then all the remaining dbgapi methods take a JSStackFrame* parameter. Can all these methods assume that we're no longer on trace?

It's simple enough to js_LeaveTrace, but I'm not sure whether that's necessary.

Comment 4

[Brendan Eich [:brendan]]

We shouldn't add a js_LeaveTrace. What's the qualifier to use that says "we promise to be off-trace here, please don't turn red, oh dear static-analysis tbox"?

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: fix

Thanks to bsmedberg for irc advice.

/be

Comment 6

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/2d8dd0494e64

/be

Comment 7

[Benjamin Smedberg]

http://hg.mozilla.org/tracemonkey/rev/27739a61e313 is a followup, reversing the order of the asserts so the analysis knows it's safe to assert cx->fp

Comment 8

[Benjamin Smedberg]

I think this should be a blocker for reopening mozilla-central, but does not need to block 1.9.1

Comment 9

[Rob Campbell [:rc] (:robcee)]

I've found a crasher filed at bug 496981. Could these be related?

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> I've found a crasher filed at bug 496981. Could these be related?

No, this bug is just about a false positive in the static analysis due to failure to annotate, which is now fixed in tracemonkey.

Rob, if you could test Firebug on tracemonkey hourly builds that would be great.

/be

Comment 11

[Rob Campbell [:rc] (:robcee)]

wilco

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/2d8dd0494e64

Comment 13

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fc298bb83af7

Comment 14

[Karsten Düsterloh]

This 
  JS_ASSERT(cx->fp);
makes Venkman crash in 1.9.1 SM debug builds for every breakpoint.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Why was this checked into the 1.9.1 branch? It isn't blocking, wanted, or approved...

Comment 16

[Robert Sayre]

(In reply to comment #15)
> Why was this checked into the 1.9.1 branch? It isn't blocking, wanted, or
> approved...

ug. that must be mistake...

Comment 17

[Robert Sayre]

(In reply to comment #16)
> (In reply to comment #15)
> > Why was this checked into the 1.9.1 branch? It isn't blocking, wanted, or
> > approved...
> 
> ug. that must be mistake...

must be *my* mistake, I meant to write.

Comment 18

[Benjamin Smedberg]

The fix is debug-only and therefore mostly exempt from the normal rules... no harm done.

Comment 19

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Harm definitely done -- we need to be able to debug 1.9.1-stream builds, which is why we take assertion and other debug-only fixes on maintenance stream.  Can we get a fix for this?

Comment 20

[Benjamin Smedberg]

Shaver, I don't understand. This *did* land on 1.9.1, even though it technically didn't have the proper approvals.

Comment 21

[Brendan Eich [:brendan]]

Fixed by patch in bug 497998.

/be

Comment 22

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yes, I mean that harm was done by the landing, since it took us from "static analysis tinderbox is red" to "can't use a JS debugger in a 1.9.1 debug build".  We'll have to take bug 497998's patch somewhere to remedy that, hopefully after FF3.5 rc1 becomes FF3.5 :)