Last Comment Bug 513981 - TM: possible GC rooting bug with Google Docs
: TM: possible GC rooting bug with Google Docs
Status: RESOLVED FIXED
: fixed-in-tracemonkey [sg:critical]
: testcase, verified1.9.1
Product: Core
Classification: Components
Component: JavaScript Engine
: Trunk
: All All
: P1 normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
: general
: https://bugzilla.mozilla.org/attachme...
:
: 514999
  Show dependency treegraph
 
Reported: 2009-09-01 12:22 PDT by David Mandelin
Modified: 2010-01-25 00:06 PST (History)
14 users (show)
sayrer: blocking1.9.2+
dveditz: wanted1.9.0.x-
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  1.0+
  ---
  ---
  ---
  beta1-fixed
  .6+
  .6-fixed


Attachments
Patch (1.70 KB, patch)
2009-09-04 16:41 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
patch (6.33 KB, patch)
2009-09-04 16:58 PDT, Andreas Gal :gal
mrbkap: review+
Details | Diff | Splinter Review
patch (5.39 KB, patch)
2009-11-04 14:20 PST, Andreas Gal :gal
mrbkap: review+
dveditz: approval1.9.1.6+
Details | Diff | Splinter Review

Summon comment box

Description David Mandelin 2009-09-01 12:22:25 PDT
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 <gal@mozilla.com>
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 1 Andreas Gal :gal 2009-09-04 16:39:17 PDT
*** Bug 514766 has been marked as a duplicate of this bug. ***
Comment 2 Blake Kaplan (:mrbkap) 2009-09-04 16:41:12 PDT
Created attachment 398787 [details] [review]
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 2009-09-04 16:46:01 PDT
Great initial debugging by dmandelin proving the source of the bug.
Comment 4 Andreas Gal :gal 2009-09-04 16:58:07 PDT
Created attachment 398793 [details] [review]
patch

Make sure native calls immediately unroot nativeVp as well, not just setter/getter invocations.
Comment 5 Andreas Gal :gal 2009-09-04 17:00:36 PDT
http://hg.mozilla.org/tracemonkey/rev/18ec7c6f7fb2
Comment 6 Daniel Veditz 2009-09-10 11:26:30 PDT
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 2009-09-10 11:38:28 PDT
The dependent bug has to be landed at the same time.
Comment 8 Reed Loden [:reed] (very busy) 2009-09-15 00:04:13 PDT
Can we get this landed on trunk? Google Docs is currently almost impossible to use. :/
Comment 9 Blake Kaplan (:mrbkap) 2009-09-16 17:14:32 PDT
http://hg.mozilla.org/mozilla-central/rev/18ec7c6f7fb2
Comment 10 Daniel Veditz 2009-09-20 09:56:34 PDT
(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 2009-09-21 12:05:16 PDT
(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 2009-09-22 17:37:22 PDT
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) 2009-10-05 09:48:17 PDT
Where are we with getting this on 1.9.2, especially since it's a beta blocker.
Comment 15 Daniel Veditz 2009-10-16 11:09:58 PDT
Comment on attachment 398793 [details] [review]
patch

Approved for 1.9.1.5, a=dveditz for release-drivers
Comment 16 Robert Sayre 2009-11-04 09:24:56 PST
Andreas, this doesn't apply to 1.9.1. Can you backport?
Comment 17 Andreas Gal :gal 2009-11-04 14:14:20 PST
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 2009-11-04 14:20:16 PST
Created attachment 410346 [details] [review]
patch
Comment 19 Andreas Gal :gal 2009-11-04 14:20:56 PST
1.9.1 patch attached. Passes trace tests.
Comment 20 Daniel Veditz 2009-11-08 21:08:13 PST
Comment on attachment 410346 [details] [review]
patch

Moving 1.9.1.6 approval to the correct branch patch
Comment 22 Al Billings [:abillings] 2009-11-20 11:12:41 PST
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).

Note You need to log in before you can comment on or make changes to this bug.