504797 - Crash on Google Docs with jit enabled

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Sylvain Pasche]

If you load or create a new Google Docs document on http://docs.google.com with jit on, you get the unresponsive script dialog with:

Script: http://docs.google.com/js/791001784-EditPage.js:15

with jit off it doesn't happen.

m-c regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b9012a18d2c6&tochange=9b4dd06e1c9d

tracemonkey regression range:
http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=14ae4c00c420&tochange=700866ae679d

Comment 1

[Sylvain Pasche]

in the same regression window (maybe a duplicate of this one): bug 505516

Comment 2

[Sylvain Pasche]

It's now crashing since 2009-08-26-04:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b712ab888d9b&tochange=485a319ce5e6

Comment 3

[Andreas Gal :gal]

We are going to need some bisecting here. There are 132 TM changesets in that window.

Comment 4

[Sylvain Pasche]

It doesn't crash on Linux apparently (there is still the unresponsive script dialog there).

Comment 5

[Sylvain Pasche]

With the tracemonkey builds:

http://hg.mozilla.org/tracemonkey/rev/7b6fbcb0fd97 unresponsive script
http://hg.mozilla.org/tracemonkey/rev/72a50720cd40 crashes

window:
http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=7b6fbcb0fd97&tochange=72a50720cd40

Comment 6

[Andreas Gal :gal]

I am guessing upvar on trace.

Comment 7

[David Mandelin [:dmandelin]]

The first regression, when it started becoming unresponsive, came here:

changeset:   30020:c76558a87dd9
user:        David Mandelin <[e-mail]>
date:        Wed Jul 08 11:16:41 2009 -0700
summary:     Bug 453730: trace JSOP_ARGUMENTS, r=gal

Comment 8

[David Mandelin [:dmandelin]]

The crash, which started at a later time, seems to occur because in array_slice, the new array nobj (in the non-dense path at the bottom of the function) gets gc'd during the loop that sets up its values. That's pretty surprising, but it doesn't seem to be the source of the problem: if I use a JSAutoTempValueRooter on nobj, the crash goes away, and I do eventually get a Google Docs window, but only after a 10+ seconds.

The slowness appears to be related somehow to tracing JSOP_ARGUMENTS, but I haven't figured that out yet.

Comment 9

[David Mandelin [:dmandelin]]

Attachment: Shell test case

This is a shell test case that behaves differently in jit vs. browser. I believe it is due to the same bug. (Bisection showed that the problem started when JSOP_ARGUMENTS started being traced. I modified TM to print out PCs where JSOP_ARGUMENTS was executed. I discovered that the program worked fine if I made a certain one abort. I then looked at the Google code for that PC and wrote some similar shell code. This was also based on a hunch that the problem was related to passing |arguments| objects created on trace to builtins.)

Comment 10

[David Mandelin [:dmandelin]]

Attachment: Stopgap patch

This patch fixes the Google Docs bug, but some related issue remain. I'd like to do those as a separate bug, if that's OK, and push this first to fix things up for nightly users. 

The basic problem is that arguments objects created on trace don't have a frame pointer (because there is no frame), so to the arguments property getter/setter code, they look like arguments objects from returned functions. This is a problem for builtins at least, and perhaps other code too. The stopgap just checks for arguments objects being passed to builtins at record time and aborts.

Comment 11

[David Mandelin [:dmandelin]]

Attachment: Stopgap patch 2

Forgot to reformat the test case.

Comment 12

[Ashley Bischoff (blog at handcoding.com)]

(In reply to comment #10)
> Stopgap patch
> 
> This patch fixes the Google Docs bug, but some related issue remain. I'd like
> to do those as a separate bug, if that's OK, and push this first to fix things
> up for nightly users. 

Out of curiosity, does your patch happen to also fix the issue from sister- bug 505516 (via comment #1) in which Google Spreadsheets' navigation bar is grayed out?

Comment 13

[David Mandelin [:dmandelin]]

(In reply to comment #12)
> (In reply to comment #10)
> > Stopgap patch
> > 
> > This patch fixes the Google Docs bug, but some related issue remain. I'd like
> > to do those as a separate bug, if that's OK, and push this first to fix things
> > up for nightly users. 
> 
> Out of curiosity, does your patch happen to also fix the issue from sister- bug
> 505516 (via comment #1) in which Google Spreadsheets' navigation bar is grayed
> out?

Seems to.

Comment 14

[David Mandelin [:dmandelin]]

Attachment: WIP real fix

This patch (supposedly) works in general and can stay on trace for almost all cases of getting an argument through natives.

The basic idea is to set the |private| member for trace-created arguments objects to a struct that contains a pointer to the arguments area of the native stack and a typemap for said area. The getter for Arguments can then use that to get the value and box it to a jsval.

Currently this is running on try, but my main question about it is whether doing the deep bail as I do in the getter (for the oom-on-alloc-double case) and the setter (for all cases) is correct.

Comment 15

[David Mandelin [:dmandelin]]

Attachment: Patch

This is a full fix: Arguments objects created on trace get a specially tagged private pointer to the arguments area, and the getter is updated to use it.

Comment 16

[David Mandelin [:dmandelin]]

Attachment: Patch 2 (forgot test case last time)

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 397975 [details] [diff] [review]
Stopgap patch 2

Looks pretty sketchy to me. :)

If you still think it's necessary, ping me on IRC and we can land this, but if the complete fix is ready today, I'd rather see that land.

Comment 18

[David Mandelin [:dmandelin]]

(In reply to comment #17)
> (From update of attachment 397975 [details] [diff] [review])
> Looks pretty sketchy to me. :)
> 
> If you still think it's necessary, ping me on IRC and we can land this, but if
> the complete fix is ready today, I'd rather see that land.

Yes, the plan is now just to land the complete fix.

Comment 19

[Andreas Gal :gal]

Comment on attachment 398506 [details] [diff] [review]
Patch 2 (forgot test case last time)


>+
>+// This function uses a type defined only if tracing is enabled.

Not sure we need that comment.


>         if (arg < GetArgsLength(obj)) {
>+// This type is defined only if tracing is enabled.

dito

Love the template hack for the double OOM handling.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 398506 [details] [diff] [review]
Patch 2 (forgot test case last time)

>+struct ReserveDoubleOOMHandler {
>+    static bool handleDoubleOOM(JSContext *cx, double d, jsval& v)
>+    {

Love this template traits stuff, but nit: inline method style puts qualifiers, type, declarator and { all on the same line where they fit. Matches the zero-inheritance { style for the struct head.

/be

Comment 21

[David Mandelin [:dmandelin]]

Pushed to TM with nits fixed as 277d2b2e2384.

Comment 22

[Graydon Hoare :graydon]

Regression in unit tests:

trace-test/tests/basic/inArrayTest.js: trace-test/tests/basic/inArrayTest.js:9: TypeError: Assertion failed: got 10, expected 104

Maybe back this out?

Comment 23

[Andreas Gal :gal]

dave?

Comment 24

[Andreas Gal :gal]

I don't see the regression on any other platform on the waterfall and the patch and the test case seem very unrelated.

Comment 25

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

I couldn't reproduce this on Linux.

Comment 26

[Andreas Gal :gal]

mac tinderbox is also failing now. I can't reproduce this on mac either.

Comment 27

[David Mandelin [:dmandelin]]

(In reply to comment #22)
> Regression in unit tests:
> 
> trace-test/tests/basic/inArrayTest.js: trace-test/tests/basic/inArrayTest.js:9:
> TypeError: Assertion failed: got 10, expected 104
> 
> Maybe back this out?

[Forehead slap.] I changed the 'expect' to 104 to test a minor fix to the tinderbox output for trace-test.py and accidentally committed that as part of this patch. Looks like it got fixed already.

Comment 29

[Reed Loden [:reed]]

Can we get TM merged to trunk soon so I can actually use Google Docs, please? :)

Comment 30

[Damon Sicore (:damons)]

P1 now.

Comment 31

[David Mandelin [:dmandelin]]

sayrer usually does the merge but he's on vacation until Monday. Does anyone know his merge procedure?

Comment 32

[Robert Sayre]

from a tracemonkey tree: 
hg pull -u http://hg.mozilla.org/mozilla-central
hg merge
hg commit

... builds, green tinderboxes ...

from a mozilla-central tree:
hg pull -u http://hg.mozilla.org/tracemonkey
hg merge
hg commit

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/277d2b2e2384

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/ff4821569605

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on trunk with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090920 Minefield/3.7a1pre ID:20090920031129

Comment 36

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/810e4bcfdd94

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixes on 1.9.2 with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b1pre) Gecko/20091004 Namoroka/3.6b1pre ID:20091004033636

Comment 39

[Bob Clary [:bc] (inactive)]

js/src/trace-test/tests/basic/argumentsPassedToBuiltin.js