900890 - crash in js::ion::LinearScanAllocator::assign

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Scoobidiver (away)]

It started spiking in 25.0a1/20130720 and has been hit mainly by a single user but not only. The regression range might be:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=af4e3ce8c487&tochange=bf73e10f5e54

Signature 	js::ion::LinearScanAllocator::assign(js::ion::LAllocation) More Reports Search
UUID 	25c17c97-1134-4372-bc97-e7b922130802
Date Processed	2013-08-02 00:51:03.441835
Uptime	3932
Last Crash	10269 seconds before submission
Install Age 	37176 since version was first installed.
Install Time 	2013-08-01 14:01:46
Product 	Firefox
Version 	25.0a1
Build ID 	20130801030223
Release Channel 	nightly
OS 	Windows NT
OS Version 	6.3.9431
Build Architecture 	x86
Build Architecture Info 	GenuineIntel family 6 model 37 stepping 2 | 4
Crash Reason 	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address 	0x0
App Notes 	
AdapterVendorID: 0x1002, AdapterDeviceID: 0x68c1, AdapterSubsysID: 00000000, AdapterDriverVersion: 13.150.100.1000
D2D! D2D+ DWrite? DWrite+ D3D10 Layers! D3D10 Layers+ 

Frame 	Module 	Signature 	Source
0 	mozjs.dll 	js::ion::LinearScanAllocator::assign(js::ion::LAllocation) 	js/src/ion/LinearScan.cpp
1 	mozjs.dll 	js::ion::LinearScanAllocator::spill() 	js/src/ion/LinearScan.cpp
2 	mozjs.dll 	js::ion::LinearScanAllocator::allocateRegisters() 	js/src/ion/LinearScan.cpp
3 	mozjs.dll 	js::ion::LinearScanAllocator::go() 	js/src/ion/LinearScan.cpp
4 	mozjs.dll 	js::ion::GenerateLIR(js::ion::MIRGenerator *) 	js/src/ion/Ion.cpp
5 	mozjs.dll 	js::ion::CompileBackEnd(js::ion::MIRGenerator *,js::ion::MacroAssembler *) 	js/src/ion/Ion.cpp
6 	mozjs.dll 	js::ion::IonCompile 	js/src/ion/Ion.cpp
7 	mozjs.dll 	js::ion::Compile 	js/src/ion/Ion.cpp
8 	mozjs.dll 	js::ion::CanEnter(JSContext *,js::RunState &) 	js/src/ion/Ion.cpp
9 	mozjs.dll 	js::RunScript(JSContext *,js::RunState &) 	js/src/vm/Interpreter.cpp
10 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
11 	mozjs.dll 	js::CallOrConstructBoundFunction(JSContext *,unsigned int,JS::Value *) 	js/src/jsfun.cpp

More reports at:
https://crash-stats.mozilla.com/report/list?product=Firefox&signature=js%3A%3Aion%3A%3ALinearScanAllocator%3A%3Aassign%28js%3A%3Aion%3A%3ALAllocation%29

Comment 1

[Xavier Fung]

I am the very user who has submitted these reports. It appears that the Gecko Profiler extension (1.12.5) causes these crashes. This happens randomly on Facebook, often when I clicked for comments from posts. I can reliably hit the crash on this very page.

http://www.mtr-shatincentrallink.hk/tc/public-consultation/consultation-meeting.html

Comment 2

[Scoobidiver (away)]

(In reply to Xavier Fung from comment #1)
> I can reliably hit the crash on this very page.
> http://www.mtr-shatincentrallink.hk/tc/public-consultation/consultation-
> meeting.html
Me too with Gecko Profiler.

Comment 3

[Xavier Fung]

Gecko Profiler 1.12.5 was released on Jul 18 and my first crash happened on Jul 20 nightly.

Comment 4

[Alice0775 White]

Built in Profiler also cause the crash.

Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=97a092648a62&tochange=76e7ef98a9d1

Comment 5

[Scoobidiver (away)]

It's a regression from bug 892426.

Comment 6

[Jan de Mooij [:jandem]]

LSetElementCacheV now requires 7 registers on x86 (1 for the object, 2 for the boxed index, 2 for the boxed value, 2 temps) and with the profiler enabled we only have 6 registers (ebp is not available) so regalloc can't satisfy this..

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> LSetElementCacheV now requires 7 registers on x86 (1 for the object, 2 for
> the boxed index, 2 for the boxed value, 2 temps) and with the profiler
> enabled we only have 6 registers (ebp is not available) so regalloc can't
> satisfy this..

2 things:
 1/ If we cannot allocate more registers, then we should properly fail the register allocation and not crash.

 2/ And fix this issue by either:
   a. Adding support for stack allocation as argument of our ICs.
   b. Replacing the setElementIC by a call, if we are profiling.

Comment 8

[Jan de Mooij [:jandem]]

nbp can you take this one? Thanks!

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
>    b. Replacing the setElementIC by a call, if we are profiling.

This would be unfortunate.. The call will probably be a lot slower than an IC stub so it would make the profiler less reliable.

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: Testing: Add register pressure.

Based on comment 6, this patch should fake the activation of the profiler in terms of register pressure, and thus should be visible in the test suite.

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: bug900890.patch

Hannes pointed that masm.extractInt32 is reusing the payload register, and thus instead of having a special allocation for unboxing the value on x86 (as needed on x64), we can just use a bogus as a temp value, as long as the temp is dedicated to extractInt32.

So I've renamed the temp to make one of them dedicated to handle the int value, and reuse the logic added by Hannes as part as Bug 880471.

js1_5/Regress/regress-192414.js was failing if we remove ebp from the list of registers, and pass with this patch applied.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 787866 [details] [diff] [review]
bug900890.patch

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

Nice fix!

Comment 12

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2045c5f41bee

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2045c5f41bee

Comment 14

[Nicolas B. Pierron [:nbp]]

Comment on attachment 787866 [details] [diff] [review]
bug900890.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 892426

User impact if declined: Crash on x86 when using the sampling profiler of Gecko. (not only the addon)

Testing completed (on m-c, etc.): yes.

Risk to taking this patch (and alternatives if risky): This patch use a function which has been introduced in Bug 880471, which has not yet proved to be faulty since Gecko 24.  So I estimate the risk to be minimal.

String or IDL/UUID changes made by this patch: N/A

Comment 15

[Nicolas B. Pierron [:nbp]]

(In reply to Scoobidiver from comment #0)
> It started spiking in 25.0a1/20130720 and has been hit mainly by a single
> user but not only. The regression range might be:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=af4e3ce8c487&tochange=bf73e10f5e54

Knowing that this bug is caused by Bug 892426, I am flagging previous versions as affected too.

Comment 16

[Nicolas B. Pierron [:nbp]]

Comment on attachment 787866 [details] [diff] [review]
bug900890.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 892426

User impact if declined: Crash on x86 when using the sampling profiler of Gecko. (not only the addon)

Testing completed (on m-c, etc.): yes.

Risk to taking this patch (and alternatives if risky): This patch use a function which has been introduced in Bug 880471, which has not yet proved to be faulty since Gecko 24.  So I estimate the risk to be minimal.

String or IDL/UUID changes made by this patch: N/A

Comment 17

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b9e3bd42389
https://hg.mozilla.org/releases/mozilla-beta/rev/23b7a978c567

Comment 18

[Paul Silaghi, QA [:pauly]]

I still see post fix crashes on FF 24 beta. Xavier, can you please confirm this is fixed ?

Comment 19

[Xavier Fung]

I use Nightly and cannot see any crash on the link posted in comment 1.

Comment 20

[u279076]

Assuming this is verified fixed based on Xavier's comment 19. 

Paul, if you're seeing a high volume of these signature please provide URLs so we can investigate if a follow-up bug should be reported.

Comment 21

[Paul Silaghi, QA [:pauly]]

This is safe, there are no crashes on FF > 24.0b5 in the last month.