858566 - Builds using MSVC 9 crash in js::MutableValueOperations (BaselineCompiler) on startup

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Bill Gianopoulos [:WG9s]]

Building using MSVC version 9 crash upon startup except in safe-mode.  This happens even with a new profile.

So far, I have narrowed it down to http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=97cfc16ba5dc&tochange=c232bec6974d

I am current doing an hg bisect on that range.

I will move this to the proper component once the checkin causing the regression is identified.

Comment 1

[Bill Gianopoulos [:WG9s]]

I should have mentioned that the crash occurs in mozjs.dll.

Comment 2

[Bill Gianopoulos [:WG9s]]

The defective version of hg bisect that comes with Windows mozilla build came up with this:

The first bad revision is:
changeset:   127458:ca457da3604a
parent:      127457:9baa31b30db4
parent:      127039:e60919ded783
user:        Jan de Mooij <[e-mail]>
date:        Wed Apr 03 10:25:36 2013 +0200
summary:     Merge from mozilla-central.

Which I find not to be that usefull.

Comment 3

[Bill Gianopoulos [:WG9s]]

OK I think I managed to do a relevent --extend.  The linux version just does this. ... 
sigh.

Comment 4

[Bill Gianopoulos [:WG9s]]

This started with the basecompiler merge.  I am doing a bisect on the project branch to try to isolate it further.

Comment 6

[Philip Chee]

> This started with the basecompiler merge.
From Bug 858923 Comment 1:
> P.S. turning off turning off javascript.options.baselinejit.chrome and
> javascript.options.baselinejit.content stops SeaMonkey from crashing

From Bug 858923 Comment 0:

I'm building SeaMonkey from mozilla-central + comm-central daily. The last few days I've been having a startup crash:

Disassembly:

6E49D0E5  jge         js::MutableValueOperations<JS::Rooted<JS::Value> >::setNumber+1Dh (6E49D0EDh) 
6E49D0E7  fadd        qword ptr [__real@41f0000000000000 (6E66FF10h)] 
6E49D0ED  fstp        qword ptr [ecx] 
6E49D0EF  xor         al,al 
6E49D0F1  ret         4    
6E49D0F4  mov         edx,0FFFFFF81h 
6E49D0F9  mov         dword ptr [ecx],eax 
6E49D0FB  mov         dword ptr [ecx+4],edx 
6E49D0FE  mov         al,1 
6E49D100  ret         4

Stack is:

>	mozjs.dll!js::MutableValueOperations<JS::Rooted<JS::Value> >::setNumber(unsigned int ui=0)  Line 1481 + 0x29 bytes	C++
 	mozjs.dll!js::ion::DoBinaryArithFallback(JSContext * cx=0x07ac6fc0, js::ion::BaselineFrame * frame=0x003dde3c, js::ion::ICBinaryArith_Fallback * stub=0x0a1aca80, JS::Handle<JS::Value> lhs={...}, JS::Handle<JS::Value> rhs={...}, JS::MutableHandle<JS::Value> ret={...})  Line 2395 + 0x1a3 bytes	C++
 	007d95dc()	
 	mozjs.dll!EnterBaseline(JSContext * cx=0x00000000, js::StackFrame * fp=0x00000000, void * jitcode=0x007df500, bool osr=false)  Line 154 + 0x31 bytes	C++
 	mozjs.dll!js::ion::EnterBaselineMethod(JSContext * cx=0x07ac6fc0, js::StackFrame * fp=0x047a0128)  Line 182 + 0xc bytes	C++
 	mozjs.dll!js::RunScript(JSContext * cx=0x07ac6fc0, js::StackFrame * fp=0x047a0128)  Line 341 + 0x7 bytes	C++
 	mozjs.dll!js::InvokeKernel(JSContext * cx=0x07ac6fc0, JS::CallArgs args={...}, js::MaybeConstruct construct=NO_CONSTRUCT)  Line 425	C++
 	mozjs.dll!js::Interpret(JSContext * cx=0x07ac6fc0, js::StackFrame * entryFrame=0x047a0058, js::InterpMode interpMode=JSINTERP_NORMAL, bool useNewType=false)  Line 2393 + 0x11 bytes	C++
 	mozjs.dll!js::RunScript(JSContext * cx=0x07ac6fc0, js::StackFrame * fp=0x047a0058)  Line 365 + 0x9 bytes	C++
 	mozjs.dll!js::InvokeKernel(JSContext * cx=0x07ac6fc0, JS::CallArgs args={...}, js::MaybeConstruct construct=NO_CONSTRUCT)  Line 425	C++
 	mozjs.dll!js::Invoke(JSContext * cx=0x0958bc40, const JS::Value & thisv={...}, const JS::Value & fval={...}, unsigned int argc=6, JS::Value * argv=0x003dea68, JS::Value * rval=0x003de94c)  Line 455 + 0x12 bytes	C++
 	mozjs.dll!JS_CallFunctionValue(JSContext * cx=0x07ac6fc0, JSObject * objArg=0x0958bc40, JS::Value fval={...}, unsigned int argc=6, JS::Value * argv=0x003dea68, JS::Value * rval=0x003de94c)  Line 5854 + 0x46 bytes	C++
 	xul.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper=0x0a8c7bc0, unsigned short methodIndex=4, const XPTMethodDescriptor * info_=0x021ff0bc, nsXPTCMiniVariant * nativeParams=0x003deb34)  Line 1435	C++
 	xul.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex=4, const XPTMethodDescriptor * info=0x021ff0bc, nsXPTCMiniVariant * params=0x003deb34)  Line 578 + 0x13 bytes	C++
 	xul.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x0a7da5f0, unsigned int methodIndex=4, unsigned int * args=0x003debec, unsigned int * stackBytesToPop=0x003debdc)  Line 85 + 0x15 bytes	C++
 	xul.dll!SharedStub()  Line 113	C++
 	xul.dll!nsBrowserStatusFilter::MaybeSendProgress()  Line 312 + 0x16 bytes	C++
 	xul.dll!nsBrowserStatusFilter::OnProgressChange(nsIWebProgress * aWebProgress=0x00000000, nsIRequest * aRequest=0x00000000, int aCurSelfProgress=0, int aMaxSelfProgress=0, int aCurTotalProgress=1, int aMaxTotalProgress=2)  Line 182	C++
 	xul.dll!nsBrowserStatusFilter::OnStateChange(nsIWebProgress * aWebProgress=0x0a796c14, nsIRequest * aRequest=0x0a82de28, unsigned int aStateFlags=65552, tag_nsresult aStatus=NS_OK)  Line 143	C++
 	xul.dll!nsDocLoader::DoFireOnStateChange(nsIWebProgress * const aProgress=0x0a796c14, nsIRequest * const aRequest=0x0a82de28, int & aStateFlags=65552, tag_nsresult aStatus=NS_OK)  Line 1290 + 0x14 bytes	C++
 	xul.dll!nsDocLoader::FireOnStateChange(nsIWebProgress * aProgress=0x0a796c14, nsIRequest * aRequest=0x0a82de28, int aStateFlags=65552, tag_nsresult aStatus=NS_OK)  Line 1227 + 0x15 bytes	C++
 	xul.dll!nsDocLoader::doStopURLLoad(nsIRequest * request=0x0a82de28, tag_nsresult aStatus=NS_OK)  Line 835	C++
 	xul.dll!nsDocLoader::OnStopRequest(nsIRequest * aRequest=0x0a82de28, nsISupports * aCtxt=0x00000000, tag_nsresult aStatus=NS_OK)  Line 637	C++
 	xul.dll!nsLoadGroup::RemoveRequest(nsIRequest * request=0x0a82de28, nsISupports * ctxt=0x00000000, tag_nsresult aStatus=NS_OK)  Line 676 + 0xf bytes	C++
 	xul.dll!nsDocument::SetScriptGlobalObject(nsIScriptGlobalObject * aScriptGlobalObject=0x00000000)  Line 4096	C++
 	xul.dll!nsDocumentViewer::Close(nsISHEntry * aSHEntry=0x00000000)  Line 1449	C++
 	xul.dll!nsDocShell::SetupNewViewer(nsIContentViewer * aNewViewer=0x0940a050)  Line 8265	C++
 	xul.dll!nsDocShell::Embed(nsIContentViewer * aContentViewer=0x0940a050, const char * aCommand=0x6b38614b, nsISupports * aExtraInfo=0x00000000)  Line 6354	C++
 	xul.dll!nsDocShell::CreateContentViewer(const char * aContentType=0x00000000, nsIRequest * request=0x09628c2c, nsIStreamListener * * aContentHandler=0x01667b5c)  Line 8076 + 0x15 bytes	C++
 	xul.dll!nsDSURIContentListener::DoContent(const char * aContentType=0x09667af8, bool aIsContentPreferred=false, nsIRequest * request=0x00710000, nsIStreamListener * * aContentHandler=0x09667b5c, bool * aAbortProcess=0x003df067)  Line 125	C++
 	xul.dll!nsDocumentOpenInfo::TryContentListener(nsIURIContentListener * aListener=0x008367e0, nsIChannel * aChannel=0x09667b5c)  Line 661	C++
 	xul.dll!nsDocumentOpenInfo::DispatchContent(nsIRequest * request=0x09628c2c, nsISupports * aCtxt=0x00000000)  Line 360 + 0x12 bytes	C++
 	xul.dll!nsDocumentOpenInfo::OnStartRequest(nsIRequest * request=0x00000000, nsISupports * aCtxt=0x00000000)  Line 252 + 0xe bytes	C++
 	xul.dll!nsBaseChannel::OnStartRequest(nsIRequest * request=0x09668330, nsISupports * ctxt=0x00000000)  Line 720 + 0x19 bytes	C++
 	xul.dll!nsInputStreamPump::OnStateStart()  Line 422	C++
 	xul.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream=0x094ae6a8)  Line 383	C++
 	xul.dll!nsOutputStreamReadyEvent::Run()  Line 83	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=true, bool * result=0x003df217)  Line 627 + 0x6 bytes	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * thread=0x01119160, bool mayWait=true)  Line 238 + 0xd bytes	C++
 	xul.dll!nsThread::Shutdown()  Line 474 + 0xa bytes	C++
 	xul.dll!nsRunnableMethodImpl<enum tag_nsresult (__stdcall nsIUrlClassifierDBServiceWorker::*)(void),1>::Run()  Line 351	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=true, bool * result=0x003df287)  Line 627 + 0x6 bytes	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * thread=0x01119160, bool mayWait=true)  Line 238 + 0xd bytes	C++
 	xul.dll!nsThread::Shutdown()  Line 474 + 0xa bytes	C++
 	xul.dll!nsRunnableMethodImpl<enum tag_nsresult (__stdcall nsIUrlClassifierDBServiceWorker::*)(void),1>::Run()  Line 351	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=true, bool * result=0x003df2f7)  Line 627 + 0x6 bytes	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * thread=0x01119160, bool mayWait=true)  Line 238 + 0xd bytes	C++
 	xul.dll!nsThread::Shutdown()  Line 474 + 0xa bytes	C++
 	xul.dll!nsRunnableMethodImpl<enum tag_nsresult (__stdcall nsIUrlClassifierDBServiceWorker::*)(void),1>::Run()  Line 351	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=true, bool * result=0x003df367)  Line 627 + 0x6 bytes	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * thread=0x01119160, bool mayWait=true)  Line 238 + 0xd bytes	C++
 	xul.dll!nsThread::Shutdown()  Line 474 + 0xa bytes	C++
 	xul.dll!nsRunnableMethodImpl<enum tag_nsresult (__stdcall nsIUrlClassifierDBServiceWorker::*)(void),1>::Run()  Line 351	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=true, bool * result=0x003df3d7)  Line 627 + 0x6 bytes	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * thread=0x01119160, bool mayWait=true)  Line 238 + 0xd bytes	C++
 	xul.dll!nsThread::Shutdown()  Line 474 + 0xa bytes	C++
 	xul.dll!nsRunnableMethodImpl<enum tag_nsresult (__stdcall nsIUrlClassifierDBServiceWorker::*)(void),1>::Run()  Line 351	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=false, bool * result=0x003df447)  Line 627 + 0x6 bytes	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * thread=0x01119160, bool mayWait=false)  Line 238 + 0xd bytes	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate=0x01126030)  Line 82 + 0xa bytes	C++
 	xul.dll!MessageLoop::RunInternal()  Line 217	C++
 	xul.dll!MessageLoop::RunHandler()  Line 210	C++
 	xul.dll!MessageLoop::Run()  Line 184	C++
 	xul.dll!nsBaseAppShell::Run()  Line 165	C++
 	xul.dll!nsAppShell::Run()  Line 113 + 0x9 bytes	C++
 	xul.dll!nsAppStartup::Run()  Line 289	C++
 	xul.dll!XREMain::XRE_mainRun()  Line 3881	C++
 	xul.dll!XREMain::XRE_main(int argc=4, char * * argv=0x01e81b40, const nsXREAppData * aAppData=0x001b32c8)  Line 3947 + 0x7 bytes	C++
 	xul.dll!XRE_main(int argc=4, char * * argv=0x01e81b40, const nsXREAppData * aAppData=0x001b32c8, unsigned int aFlags=0)  Line 4152 + 0x12 bytes	C++
 	seamonkey.exe!do_main(const char * exePath=0x003df8a0, int argc=4, char * * argv=0x00000000)  Line 184 + 0x13 bytes	C++
 	seamonkey.exe!NS_internal_main(int argc=4, char * * argv=0x01e81b40)  Line 272 + 0x12 bytes	C++
 	seamonkey.exe!wmain(int argc=31988544, wchar_t * * argv=0x01e81a88)  Line 112	C++
 	seamonkey.exe!__tmainCRTStartup()  Line 583 + 0x17 bytes	C
 	kernel32.dll!76913677() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	
 	ntdll.dll!76fe9f42() 	
 	ntdll.dll!76fe9f15()

Comment 7

[Bill Gianopoulos [:WG9s]]

I'm glad someone with better debugging facilities was able to duplicate this.  I am still trying to bisect on the ionmonkey branch.

Comment 8

[Bill Gianopoulos [:WG9s]]

This is taking longer than it should because things were renamed back and forth and moving around so much on the branch that it is too much for hg update's little mind.  I have had to resort with manual bisecting doing fresh clones so no dep builds.  It is slow but I am still doing it.

Comment 9

[Bill Gianopoulos [:WG9s]]

reduced regression window to this.  Still working on it.

http://hg.mozilla.org/projects/ionmonkey/pushloghtml?fromchange=3c5779365494&tochange=d8b068c9dbc1

Comment 10

[Robert Kaiser]

Philip, how do you get to believe that the crash signature "EnterBaseline" is being sent for this? Do we have MSVC9 builds out there that end up sending that? From comment #6 I'd expect "js::MutableValueOperations<JS::Rooted<JS::Value> >::setNumber(unsigned int ui)" or similar as the signature.

Comment 11

[Philip Chee]

> I'd expect "js::MutableValueOperations<JS::Rooted<JS::Value> >::setNumber(unsigned int
> ui)" or similar as the signature.
Oops I think you're correct!

Comment 12

[Robert Kaiser]

Scoobidiver, please read the comments. The EnterBaseline signature doesn't belong here.

Comment 13

[Scoobidiver (away)]

(In reply to Robert Kaiser (:[e-mail]) from comment #12)
> Scoobidiver, please read the comments. The EnterBaseline signature doesn't
> belong here.
I read it but bug 595351 was for any crashes with EnterMethodJIT in the stack trace so I assumed bug 858032 is the same for any crashes with EnterBaseline in the stack trace.
If I am wrong, please remove bug 858032 as a blocking bug.

Comment 14

[Bill Gianopoulos [:WG9s]]

My bisect identified this:

The first bad revision is:
changeset:   125404:5f25ee4c94ab
user:        Jan de Mooij <[e-mail]>
date:        Tue Mar 19 10:32:07 2013 +0100
summary:     Bug 851490 - Disable PGO for some functions. r=dvander

Which seems odd because I don't do PGO builds.

I will try seeing if I can do a build with this backed out and see what happens.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

That patch turned off optimizations on that function, so I guess that's still conceivably causing problems.

Comment 16

[Bill Gianopoulos [:WG9s]]

That patch is incorrect.  As far as I know, the "# pragma optimize("g", on)" lines should be "# pragma optimize("", on)", which is the correct way to revert to the optimization specified on the command line.  I believe this patch is turning on unwanted optimization.

See http://msdn.microsoft.com/en-us/library/chh3fb0k.aspx

Comment 17

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos [:WG9s] from comment #16)
> That patch is incorrect.  As far as I know, the "# pragma optimize("g", on)"
> lines should be "# pragma optimize("", on)", which is the correct way to
> revert to the optimization specified on the command line.  I believe this
> patch is turning on unwanted optimization.
> 
> See http://msdn.microsoft.com/en-us/library/chh3fb0k.aspx

That all said, changing it made no difference.  I think I need to re-check some of my work on bisecting this.

Comment 18

[Bill Gianopoulos [:WG9s]]

I also tried changing both the off and on to specify "" instead of "g", which is more in line with what we do elsewhere and it still crashes.  However, backing out the patch altogether seems to avoid the crash.

Comment 19

[Kannan Vijayan [:djvj]]

I've been eyeing this bug.  Haven't had time to take a look at it yet, but I will once my immediate plate is a little bit less full, and that might be in a week or so.

This seems to be happening in a pretty old and untouched piece of Ion code that's been working for a while (trampoline VM calls), so I'm not sure what could be failing.

Does this throw an ASSERT or is it a straightforward SIGSEGV?

Comment 20

[Bill Gianopoulos [:WG9s]]

(In reply to Kannan Vijayan [:djvj] from comment #19)
> This seems to be happening in a pretty old and untouched piece of Ion code
> that's been working for a while (trampoline VM calls), so I'm not sure what
> could be failing.

But was anyone doing ionmonkey branch builds using MSVS2008/VC9?

Comment 21

[Bill Gianopoulos [:WG9s]]

Attachment: Workaround

This reverts 1/2 of the patch for bug 851490 if the compiler is VC9 or older.  This is unlikely to be the correct fix.  I suspect it might also re-introduce the issues bug 851490 was trying to fix.

That said, this does resolve my original issue.

Comment 22

[Bill Gianopoulos [:WG9s]]

Attachment: Workaround v2

This has much more promise.  From re-reading the comments in bug 851490, it seems the issue was with over-aggressive in-lining.  This patch just disables automatic in-lining rather than all optimizations.  I like this because it sounds still likely to fix the original issue, does not cause the regression seen here and allows at least some optimizations to take place on this code section that PGO though was hit often enough to require extra optimization.

Comment 23

[Philip Chee]

Comment on attachment 735980 [details] [diff] [review]
Workaround v2

This patch is WFM with VS2008SP1 / VC9 Thanks Bill!

Comment 24

[Bill Gianopoulos [:WG9s]]

(In reply to Philip Chee from comment #23)
> Comment on attachment 735980 [details] [diff] [review]
> Workaround v2
> 
> This patch is WFM with VS2008SP1 / VC9 Thanks Bill!

Great!  I would like to have some feedback on if it still avoids the original issue in bug 851490.  Not sure who to ask though.

Comment 25

[Bill Gianopoulos [:WG9s]]

Once my request for level-1 check in access gets approved, I will submit this to try and do 6 Windows PGO builds with tests.  If those prove successful, I will ask for review on the patch.

Comment 26

[Jan de Mooij [:jandem]]

Comment on attachment 735980 [details] [diff] [review]
Workaround v2

Review of attachment 735980 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking into this! PGO builds didn't fail reliably, so turning PGO back on is a bit scary. Like you said, it would be good to have a number of different try builds with green jsreftests.

Please also make sure these builds do not regress perf on SS/Kraken/Octane.

Comment 27

[Bill Gianopoulos [:WG9s]]

I fail to understand how disabling only auto in-lining could possibly regress performance more than disabling all optimization.

Comment 28

[Jan de Mooij [:jandem]]

(In reply to Bill Gianopoulos [:WG9s] from comment #27)
> I fail to understand how disabling only auto in-lining could possibly
> regress performance more than disabling all optimization.

As far as I can see, "#pragma optimize("g", off)" only disables global optimizations (link time code generation?), I don't think it disables inlining altogether.

In any case, compilers are complicated and it's always good to verify such changes don't affect performance.

Comment 29

[Bill Gianopoulos [:WG9s]]

According to the MSDN site turning off g disables optimization globally.

Comment 30

[Bill Gianopoulos [:WG9s]]

refer to the link I referenced in comment #16.

Comment 31

[Jan de Mooij [:jandem]]

(In reply to Bill Gianopoulos [:WG9s] from comment #29)
> According to the MSDN site turning off g disables optimization globally.

I read it as disabling "global optimizations" (but still allowing "local optimizations"). See also http://msdn.microsoft.com/en-us/library/1yk3ydd7.aspx

Comment 32

[Bill Gianopoulos [:WG9s]]

OIC. I read it a globally disabling optimizations.  But that its not really what it says.

Comment 33

[Bill Gianopoulos [:WG9s]]

While waiting for try access to be approved, I thought more about :jandems's concerns and decided that going back to my original approach of just fixing the issue that it does not build, and if there is still a remaining issue with PGO builds using VC9 another bug should be filed.  I did take the extra step of fixing the issue I raised in comment 16.  The issue is that the '# pragma optimize("g", on)' ends up turning on optimizations even in a --disable-optimize build.  We should instead use '# pragma optimize("", on)' like we do elsewhere in the build.

Comment 34

[Bill Gianopoulos [:WG9s]]

Attachment: Fix

Comment 35

[Bill Gianopoulos [:WG9s]]

I should mention.  I also filed bug 866425 to just drop support for building using VC9.

Comment 36

[Bill Gianopoulos [:WG9s]]

It seems bug 866425 has a lot of support.  If we decide to drop support for VC9, I will WONTFIX this and file a new bug to fix the issue of the patch for bug  	851490 inadvertently enabling optimization on --disable-optimize builds.

Comment 37

[Bill Gianopoulos [:WG9s]]

Now that the fix for bug 866425 has landed, the affected compilers are no longer supported.

--> WONTFIX