Closed Bug 900890 Opened 7 years ago Closed 7 years ago

crash in js::ion::LinearScanAllocator::assign

Categories

(Core :: JavaScript Engine, defect, critical)

25 Branch
x86
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla26
Tracking Status
firefox23 --- unaffected
firefox24 --- verified
firefox25 --- verified
firefox26 --- verified

People

(Reporter: scoobidiver, Assigned: nbp)

References

()

Details

(Keywords: crash, regression, reproducible)

Crash Data

Attachments

(2 files)

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
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
(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.
Gecko Profiler 1.12.5 was released on Jul 18 and my first crash happened on Jul 20 nightly.
It's a regression from bug 892426.
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..
Flags: needinfo?(nicolas.b.pierron)
(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.
Flags: needinfo?(nicolas.b.pierron)
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.
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
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.
Attached patch bug900890.patchSplinter Review
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.
Attachment #787866 - Flags: review?(jdemooij)
Comment on attachment 787866 [details] [diff] [review]
bug900890.patch

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

Nice fix!
Attachment #787866 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/2045c5f41bee
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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
Attachment #787866 - Flags: approval-mozilla-aurora?
(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 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
Attachment #787866 - Flags: approval-mozilla-beta?
Attachment #787866 - Flags: approval-mozilla-beta?
Attachment #787866 - Flags: approval-mozilla-beta+
Attachment #787866 - Flags: approval-mozilla-aurora?
Attachment #787866 - Flags: approval-mozilla-aurora+
I still see post fix crashes on FF 24 beta. Xavier, can you please confirm this is fixed ?
Flags: needinfo?(xavier114fch)
I use Nightly and cannot see any crash on the link posted in comment 1.
Flags: needinfo?(xavier114fch)
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.
This is safe, there are no crashes on FF > 24.0b5 in the last month.
Depends on: 905993
You need to log in before you can comment on or make changes to this bug.