513981 - TM: possible GC rooting bug with Google Docs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[David Mandelin [:dmandelin]]

This is a secondary bug from bug 504797. To reproduce, use this rev with a debug build:

changeset:   32107:e8a55ae8fb98
tag:         qparent
user:        Andreas Gal <[e-mail]>
date:        Mon Aug 31 17:02:16 2009 -0700
summary:     Compilation fix for bug 513787.

and create a new document in Google Docs. You may get a slow script dialog, but if you continue it will crash eventually inside array_slice called from trace.

The problem is that the array created by js_NewArrayObject at the end of array_slice (the non-dense case) gets GC'd (and its fields overwritten) while it is being populated in the loop. I showed that by printing field values immediately after allocation and then inside SetArrayElement. I also tried adding a new JSAutoTempValueRooter right after the call to js_NewArrayObject and that stopped the problem.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch

We used to do the builtinStatus guard before clearing nativeVp and nativeVplen on trace. That has now switched around, so this patch is safe both ways.

Comment 3

[Andreas Gal :gal]

Great initial debugging by dmandelin proving the source of the bug.

Comment 4

[Andreas Gal :gal]

Attachment: patch

Make sure native calls immediately unroot nativeVp as well, not just setter/getter invocations.

Comment 5

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/18ec7c6f7fb2

Comment 6

[Daniel Veditz [:dveditz]]

We'll switch this to blocking the next specific 1.9.1.x after it lands on trunk and 1.9.2 successfully.

Comment 7

[Andreas Gal :gal]

The dependent bug has to be landed at the same time.

Comment 8

[Reed Loden [:reed]]

Can we get this landed on trunk? Google Docs is currently almost impossible to use. :/

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/18ec7c6f7fb2

Comment 10

[Daniel Veditz [:dveditz]]

(In reply to comment #0)
> This is a secondary bug from bug 504797. To reproduce, use this rev with a
> debug build:

What does this mean? A regression from 504797? something found while you were investigating that? Bug 504797 appears to be a regression itself, but pinned on a checkin that would seem to say the 1.9.1 branch is unaffected.

Is this bug and bug 514999 independent of that regression, and needed on the 1.9.1 branch anyway?

Comment 11

[David Mandelin [:dmandelin]]

(In reply to comment #10)
> (In reply to comment #0)
> > This is a secondary bug from bug 504797. To reproduce, use this rev with a
> > debug build:
> 
> What does this mean? A regression from 504797? something found while you were
> investigating that? Bug 504797 appears to be a regression itself, but pinned on
> a checkin that would seem to say the 1.9.1 branch is unaffected.

Exactly what I meant is: 

- I found this bug while debugging bug 504797.
- This bug was exposed by bug 504797.

> Is this bug and bug 514999 independent of that regression, and needed on the
> 1.9.1 branch anyway?

I don't have access to bug 514999.

This bug is independent of bug 504797, by which I mean that fixing bug 504797 does not necessarily fix this bug. But I don't have a way of reproducing this bug without bug 504797 active.

Comment 12

[Andreas Gal :gal]

marcia crashed with a related stack in this bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=517957

We should get this onto 1.9.2 so QA doesn't spend cycles chasing stuff we fixed.

Comment 13

[Samuel Sidler (old account; do not CC)]

Where are we with getting this on 1.9.2, especially since it's a beta blocker.

Comment 14

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cd81bbde4522

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 398793 [details] [diff] [review]
patch

Approved for 1.9.1.5, a=dveditz for release-drivers

Comment 16

[Robert Sayre]

Andreas, this doesn't apply to 1.9.1. Can you backport?

Comment 17

[Andreas Gal :gal]

Ok. Same warning here. 191 is missing the getters/setters patch. This patch fixes a gc hazard in the getters/setters patch. I have to remove that code from this patch to make it fit 191. If we ever land getters/setters on 191, bad things will happen if we forget to backport the missing piece. Otherwise this applies somewhat sanely. Patch in a sec.

Comment 18

[Andreas Gal :gal]

Attachment: patch

Comment 19

[Andreas Gal :gal]

1.9.1 patch attached. Passes trace tests.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 410346 [details] [diff] [review]
patch

Moving 1.9.1.6 approval to the correct branch patch

Comment 21

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0c83a08adbf6

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.1.6 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091119 Shiretoko/3.5.6pre (.NET CLR 3.5.30729) using Google Docs (with JIT enabled).

Comment 23

[Christian Holler (:decoder)]

Bug in removed tracer code, setting in-testsuite- flag.