TM: possible GC rooting bug with Google Docs

RESOLVED FIXED

Status

()

defect
P1
normal
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: dmandelin, Assigned: mrbkap)

Tracking

({testcase, verified1.9.1})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
wanted1.9.0.x -
in-testsuite -

Firefox Tracking Flags

(status1.9.2 beta1-fixed, blocking1.9.1 .6+, status1.9.1 .6-fixed, fennec1.0+)

Details

(Whiteboard: fixed-in-tracemonkey [sg:critical], )

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

10 years ago
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.
Group: core-security

Updated

10 years ago
Duplicate of this bug: 514766
Posted patch Patch (obsolete) — Splinter Review
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.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #398787 - Flags: review?(gal)

Updated

10 years ago
Attachment #398787 - Flags: review?(gal)
Attachment #398787 - Flags: review+
Attachment #398787 - Flags: approval1.9.1.3?

Comment 3

10 years ago
Great initial debugging by dmandelin proving the source of the bug.

Updated

10 years ago
Attachment #398787 - Flags: review+
Attachment #398787 - Flags: approval1.9.1.3?

Comment 4

10 years ago
Posted patch patchSplinter Review
Make sure native calls immediately unroot nativeVp as well, not just setter/getter invocations.
Attachment #398787 - Attachment is obsolete: true

Comment 5

10 years ago
http://hg.mozilla.org/tracemonkey/rev/18ec7c6f7fb2
Whiteboard: fixed-in-tracemonkey

Updated

10 years ago
Attachment #398793 - Flags: approval1.9.2?
Attachment #398793 - Flags: approval1.9.1.3?

Updated

10 years ago
tracking-fennec: --- → ?
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey [sg:critical?]
Attachment #398793 - Flags: approval1.9.1.3? → approval1.9.1.4?
blocking1.9.1: --- → ?

Updated

10 years ago
Flags: blocking1.9.2?

Updated

10 years ago
Flags: blocking1.9.2? → blocking1.9.2+
Depends on: 514999
We'll switch this to blocking the next specific 1.9.1.x after it lands on trunk and 1.9.2 successfully.
blocking1.9.1: ? → needed

Comment 7

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

Updated

10 years ago
Whiteboard: fixed-in-tracemonkey [sg:critical?] → fixed-in-tracemonkey [sg:critical]

Updated

10 years ago
tracking-fennec: ? → 1.0+
Can we get this landed on trunk? Google Docs is currently almost impossible to use. :/
http://hg.mozilla.org/mozilla-central/rev/18ec7c6f7fb2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(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?
Reporter

Comment 11

10 years ago
(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.
Attachment #398793 - Flags: approval1.9.1.4? → approval1.9.1.5?

Comment 12

10 years ago
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.

Updated

10 years ago
Priority: -- → P1
Where are we with getting this on 1.9.2, especially since it's a beta blocker.
blocking1.9.1: needed → .5+
Comment on attachment 398793 [details] [diff] [review]
patch

Approved for 1.9.1.5, a=dveditz for release-drivers
Attachment #398793 - Flags: approval1.9.2?
Attachment #398793 - Flags: approval1.9.1.5?
Attachment #398793 - Flags: approval1.9.1.5+
Flags: wanted1.9.0.x-

Comment 16

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

Comment 17

10 years ago
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

10 years ago
Posted patch patchSplinter Review
Attachment #410346 - Flags: review?(mrbkap)

Comment 19

10 years ago
1.9.1 patch attached. Passes trace tests.

Updated

10 years ago
Blocks: 514999
No longer depends on: 514999
Attachment #410346 - Flags: review?(mrbkap) → review+
Attachment #398793 - Flags: approval1.9.1.6+
Comment on attachment 410346 [details] [diff] [review]
patch

Moving 1.9.1.6 approval to the correct branch patch
Attachment #410346 - Attachment filename: patch → patch for 1.9.1 branch
Attachment #410346 - Flags: approval1.9.1.6+
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).
Keywords: verified1.9.1
Group: core-security
Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.