605452 - Crash at [@ js::mjit::ic::GetProp ] when switching tabs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[(no longer active)]

I got this crash after updating to today's nightly build twice when switching tabs:

bp-eb07cb8e-084d-4e7f-a873-7eaf12101019
bp-4b6d5987-b979-4a48-93b0-7a0862101019

I have BarTab installed, and the crash happened as soon as I clicked a tab to load it for the first time after a restart.

Comment 1

[(no longer active)]

I could reproduce this very easily by just restarting, and trying to load 2 or 3 tabs.

Comment 2

[Alex]

I get this crash when I try to load Gmail.

Comment 3

[Makoto Kato [:m_kato]]

http://crash-stats.mozilla.com/query/query?product=Firefox&version=ALL%3AALL&range_value=1&range_unit=weeks&date=10%2F19%2F2010+07%3A53%3A00&query_search=signature&query_type=exact&query=js%3A%3Amjit%3A%3Aic%3A%3AGetProp&build_id=&process_type=any&hang_type=any&do_query=1

crash data says 64-bit only?

Comment 4

[:Margaret Leibovic]

(In reply to comment #2)
> I get this crash when I try to load Gmail.

I'm experiencing the same problem. 
http://crash-stats.mozilla.com/report/index/bp-b74d280e-9082-4e26-a8ca-982772101019
http://crash-stats.mozilla.com/report/index/bp-cc8d405b-245f-4f99-96c5-608052101019

Gmail was really slow to load, causing the "This is taking longer than usual..." page to display. When I clicked the "Try reloading the page" link, it crashed. This was reproducable.

Comment 5

[Tony Chung [:tchung]]

also confirming on the 10/19 build 64-bit.  For me, i was trying to do session restore

Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101019 Firefox/4.0b8pre

http://crash-stats.mozilla.com/report/index/bp-23d7ebe5-935b-4919-ab4f-9020c2101019

Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	XUL 	js::mjit::ic::GetProp 	js/src/jsobj.h:1046
1 		@0x1302cb67e

Comment 6

[Frederic Bezies]

Seems to crash firefox when trying to open firefox...

See http://crash-stats.mozilla.com/report/index/bp-c795fd86-b0b5-4df7-b6ac-778832101019

Comment 7

[Frederic Bezies]

I meant when opening gmail ;)

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

I can reproduce this, investigating.

Comment 9

[Robert Sayre]

Can reproduce with a tinderbox build of

http://hg.mozilla.org/tracemonkey/rev/759533cf3c38

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

This looks like some kind of register allocation bug. I'm not yet sure of the details - I can only reproduce this in optimized builds. But I think I can see where we go wrong.

  ; JSOP_GETGNAME, Load global slot into (t=R12, d=R10)
  b42b: mov  0x750(%r12), %r12    ; Load global variable
  b433: mov  %r14, %r10           ; R10 = payload mask
  b436: and  %r12, %r10           ; Convert R10 to R12's payload
  b439: xor  %r10, %r12           ; Convert R12 to R12's type

  ; Store value (t=R12, d=R10) into slot 0xA8.
  b43c: mov  %r12, %r15           ; ValueReg = type tag in R12
  b43f: or   %r10, %r15           ; Give ValueReg R10's payload
  b442: mov  %r15, 0x18(%rbx)     ; Store value into slot

  ; JSOP_GETARGPROP
  ; Load value from formal argument vector.
  b449: mov -0x8(%rbx), %r12      ; Load formal argument 
  b44d: and  %r14, %r12           ; Convert R12 to a payload
! b450: mov  %r10, %r15           ; Set ValueReg = type tag in R10
  b453: or   %r12, %r15           ; Tag R12 payload with R10
  b456: mov  %r15, 0xb0(%rbx)     ; Store value into stack slot

Something bogus happened on the ! line. R10 is a payload from the previous instruction, but it's expecting a type tag. The new value becomes something invalid, which I can remat with ($rbx - 8) | $r10. However, the stack slot at 0xb0 has a payload, which is confusing. Maybe it got added somewhere else.

Still investigating.

Comment 11

[juan becerra [:juanb]]

Happens on Linux as well (I was using 64bit), when loading gmail, on latest nightly.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Revised analysis.

  ; JSOP_GETGNAME, Load global slot into (t=R12, d=R10)
  b42b: mov  0x750(%r12), %r12    ; Load global variable
  b433: mov  %r14, %r10           ; R10 = payload mask
  b436: and  %r12, %r10           ; Convert R10 to R12's payload
  b439: xor  %r10, %r12           ; Convert R12 to R12's type

  ; JSOP_GETARGPROP calls FrameState::push(Address)
  ; 1. allocReg(DATA) evicts R12.
  b43c: mov  %r12, %r15           ; ValueReg = type tag in R12
  b43f: or   %r10, %r15           ; Give ValueReg R10's payload
  b442: mov  %r15, 0x18(%rbx)     ; Store value into slot

  ; 2. Load arg payload into R12.
  b449: mov -0x8(%rbx), %r12      ; Load formal argument 
  b44d: and  %r14, %r12           ; Convert R12 to a payload

  ; 3. allocReg(TYPE) evicts R10.
! b450: mov  %r10, %r15           ; Set ValueReg = type tag in R10
  b453: or   %r12, %r15           ; Tag R12 payload with R10
  b456: mov  %r15, 0xb0(%rbx)     ; Store value into stack slot

This is push(Address) being wonky and mostly wrong. I'll try a fix, and a test case.

Comment 13

[Takanori MATSUURA]

(In reply to comment #11)

If this issue is the same as what occurs on Linux, here is the regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1d7cd2d8b830&tochange=0a8ccb9ecdce

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

This bug has existed since the beginning of JM. The problem is that FrameState::push(Address) creates an uninitialized FrameEntry, and fills it in incrementally by allocating registers one at a time. When allocating the second register, it can evict the first.

On x64, this is really bad, since FEs are always synced in whole, and half of the FE is uninitialized. On x86, there *may* be some bug here, but I suspect it's okay (if not sketchy) because FEs are synced in halves.

Anyway, this patch fixes the logic for both platforms. I've verified that gmail no longer crashes on my machine.

Comment 15

[Sean Stangl [:sstangl]]

Comment on attachment 484581 [details] [diff] [review]
fix

breakValueIntoComponents() does not have to exist, and x64 does not need to pin address.base:

the code within FrameState::push(Address) in the #ifdef JSPUNBOX_64 section can merely be:

> RegisterID typeReg = allocReg();
> RegisterID dataReg = allocReg();
> // Note that address.base is still valid, since eviction does not clobber.
> masm.loadValueAsComponents(address, typeReg, dataReg);

r+ with changes.

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix v2

patch to checkin

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/5cfd89cf9da3

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Is it possible that this could also be showing up as a crash sans symbols, like: http://crash-stats.mozilla.com/report/index/1aeae141-c5d0-4a74-b4be-c17a72101020

Comment 19

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/5cfd89cf9da3

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

How are we tracking these bugs which have been FIXED on mozilla-central but haven't yet landed on the relbranch?

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #18)
Yes, this is most likely the same crash. It could also show up under the "EnterMethodJIT" signature, or just about anything else, but keep in mind it was x64 only.