Last Comment Bug 526449 - FinishSharingTitle should skip fixed slots for slow array
: FinishSharingTitle should skip fixed slots for slow array
Status: VERIFIED FIXED
[sg:vector-critical addons?] fixed-in...
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks: 500096
  Show dependency treegraph
 
Reported: 2009-11-04 03:13 PST by Igor Bukanov
Modified: 2010-06-22 19:51 PDT (History)
16 users (show)
sayrer: blocking1.9.2-
sayrer: wanted1.9.2+
dveditz: wanted1.9.0.x+
dveditz: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
alpha1+
needed
.4-fixed
needed
.10-fixed


Attachments
v1 (3.77 KB, patch)
2009-12-03 14:09 PST, Igor Bukanov
no flags Details | Diff | Review
v2 (8.46 KB, patch)
2009-12-04 01:01 PST, Igor Bukanov
no flags Details | Diff | Review
v3 (8.46 KB, patch)
2009-12-13 03:15 PST, Igor Bukanov
brendan: review+
Details | Diff | Review
v3 for 192 (8.68 KB, patch)
2009-12-13 11:51 PST, Igor Bukanov
mbeltzner: approval1.9.2.2-
mbeltzner: approval1.9.2.4+
Details | Diff | Review
fix for 191 (1.53 KB, patch)
2010-04-29 05:43 PDT, Igor Bukanov
brendan: review+
christian: approval1.9.1.10+
Details | Diff | Review

Description Igor Bukanov 2009-11-04 03:13:23 PST
The following example crashes in multi-threaded shell in FinishSharingTitle:

function test() {
    function makeWorkerFn(id, bar) {
        return function() {
            bar[id] = 0;
        };
    }

    for (let i = 0; i < 10; i++) {
        var bar = [];
        bar.x = 0;
        var a = [];
        for (var j = 0; j != 100; ++j)
            a.push(makeWorkerFn(j, bar));
        scatter(a);
    }
}

test();

The reason for the crash is that for the slow array FinishSharingTitle mistreats JSSLOT_ARRAY_LENGTH slot as a slot containing GC things and crashes when reinterpreting the array length, 100, as JSString.

I am not sure about severity of this bug. It cannot be triggered via the browser since even thread workers does not allow to share anything between threads. On other hand there are extensions that use threads with shared objects. For them the bug is relevant so I mark it as restricted.
Comment 1 Igor Bukanov 2009-12-03 14:09:58 PST
Created attachment 415948 [details] [diff] [review]
v1

The patch marks the slow array class with the JSCLASS_HAS_PRIVATE class taking advantage of the changes from the bug 513190 to fix this bug and eliminate slow_array_trace hack.

I will ask for a review later after some testing.
Comment 2 Igor Bukanov 2009-12-03 14:16:28 PST
I nominate this bug for 1.9.0 as it exists on all branches with fast/slow array implementation.
Comment 3 Samuel Sidler (old account; do not CC) 2009-12-03 14:27:02 PST
Moving this from the 1.9.1 needed list to ? again so we can decide if we want it in 1.9.1.7.
Comment 4 Igor Bukanov 2009-12-04 01:01:44 PST
Created attachment 416052 [details] [diff] [review]
v2

The new version is v1 + jsscript.cpp cleanup. The latter comes from a big initial patch involving JSCLASS_START refactoring. I had been writing until I realized that marking the slow array class with JSCLASS_HAS_PRIVATE is enough to fix this.
Comment 5 Igor Bukanov 2009-12-13 03:15:56 PST
Created attachment 417332 [details] [diff] [review]
v3

sync with tm tip
Comment 6 Brendan Eich [:brendan] 2009-12-13 10:01:24 PST
Comment on attachment 417332 [details] [diff] [review]
v3

r=me, thanks.

/be
Comment 8 Igor Bukanov 2009-12-13 11:51:55 PST
Created attachment 417370 [details] [diff] [review]
v3 for 192

The version for 1.9.2 required a trivial backporting.
Comment 9 Daniel Veditz [:dveditz] 2009-12-14 14:15:55 PST
Blocking 1.9.1.7, and when we're ready to land we'll take it opportunistically on the 1.9.0 branch as well.
Comment 11 Mike Beltzner [:beltzner, not reading bugmail] 2010-02-11 08:34:35 PST
Will be back to do the approval requests shortly!
Comment 12 Mike Beltzner [:beltzner, not reading bugmail] 2010-02-24 12:37:23 PST
Comment on attachment 417370 [details] [diff] [review]
v3 for 192

a1922=beltzner, after an embarassingly long time
Comment 13 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-10 12:44:26 PST
Comment on attachment 417370 [details] [diff] [review]
v3 for 192

Sad, but this missed the 1.9.2.2 train. Let's make sure to get it landed in time, next time.
Comment 14 Daniel Veditz [:dveditz] 2010-03-19 13:44:38 PDT
Comment on attachment 417370 [details] [diff] [review]
v3 for 192

Is there a testcase to check in with this patch, or one in bc's regression suite?
Comment 15 Bob Clary [:bc:] 2010-03-19 13:52:41 PDT
I don't have anything. Comment 0 appears to be it.
Comment 16 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-24 12:53:34 PDT
Comment on attachment 417370 [details] [diff] [review]
v3 for 192

a=beltzner
Comment 18 Bob Clary [:bc:] 2010-04-13 00:41:25 PDT
Igor, 1.9.2 firefox-debug/dist/bin/js gives me:

Assertion failure: (cx)->requestDepth || (cx)->thread == (cx)->runtime->gcThread, at /work/mozilla/builds/1.9.2/mozilla/js/src/jsapi.cpp:1209

on mac and 64bit linux.

1.9.3 appears ok, but i'm stilling building win32 and linux32, so I'm not sure there.
Comment 19 Bob Clary [:bc:] 2010-04-13 03:23:22 PDT
v fixed 1.9.3 win32/mac32/linux32|64.

For 1.9.2 I get the same assertion for win32/mac32/linux32|64. Can't verify there.
Comment 20 Igor Bukanov 2010-04-16 14:49:10 PDT
(In reply to comment #18)
> Igor, 1.9.2 firefox-debug/dist/bin/js gives me:
> 
> Assertion failure: (cx)->requestDepth || (cx)->thread ==
> (cx)->runtime->gcThread, at
> /work/mozilla/builds/1.9.2/mozilla/js/src/jsapi.cpp:1209
> 
> on mac and 64bit linux.


Do you mean you got the assert just by running the shell?

> 
> 1.9.3 appears ok, but i'm stilling building win32 and linux32, so I'm not sure
> there.
Comment 21 Bob Clary [:bc:] 2010-04-16 23:53:06 PDT
(In reply to comment #20)
> 
> Do you mean you got the assert just by running the shell?

yes, running the js shell built as part of firefox'

$ cd /work/mozilla/builds/1.9.2/mozilla/firefox-debug/dist/bin
$ ./js
<paste testcase>
Assertion failure: (cx)->requestDepth || (cx)->thread == (cx)->runtime->gcThread, at /work/mozilla/builds/1.9.2/mozilla/js/src/jsapi.cpp:1209
Trace/BPT trap
Comment 22 Igor Bukanov 2010-04-18 15:43:50 PDT
The assert on 1.9.2 is a regression from the bug 534051 that have not yet landed on branches, see bug 534308.
Comment 23 Mike Beltzner [:beltzner, not reading bugmail] 2010-04-28 17:46:07 PDT
We need a 1.9.1 patch for this, pronto. Sorry, we should have noticed that when we approved it for 1.9.2.
Comment 24 Andreas Gal :gal 2010-04-28 17:50:24 PDT
Define pronto. Igor is probably offline (Europe time). Is this needed today?
Comment 25 Al Billings [:abillings] 2010-04-28 17:55:11 PDT
It is the last or next to last bug waiting for code complete.
Comment 26 Igor Bukanov 2010-04-29 03:35:15 PDT
The fix for 191 needs another patch. On that branch the private slot is treated as jsval so to fix this we just need to skip the slots in  FinishSharingTitle with an explicit class check.
Comment 27 Igor Bukanov 2010-04-29 05:43:58 PDT
Created attachment 442377 [details] [diff] [review]
fix for 191

The patch just skips the slots in question.
Comment 28 Brendan Eich [:brendan] 2010-04-29 08:23:37 PDT
Comment on attachment 442377 [details] [diff] [review]
fix for 191

Safe spot-fix. r=me.

/be
Comment 29 christian 2010-05-03 14:08:31 PDT
Comment on attachment 442377 [details] [diff] [review]
fix for 191

a=LegNeato for 1.9.1.10. Please land this asap. We intend to spin a build early Tuesday PST (tomorrow) and would like to get this fix in.
Comment 30 Reed Loden [:reed] (use needinfo?) 2010-05-03 16:53:19 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/58f03b5d4931
Comment 31 Al Billings [:abillings] 2010-05-07 16:41:06 PDT
Bob, can you check this on 1.9.1?
Comment 32 Bob Clary [:bc:] 2010-05-09 09:13:47 PDT
On Mac.

$ 1.9.1/mozilla/firefox-debug/dist/bin/js -f tmp/bug-526449.js 
Assertion failure: (cx)->requestDepth || (cx)->thread == (cx)->runtime->gcThread, at /work/mozilla/builds/1.9.1/mozilla/js/src/jsapi.cpp:1194
Trace/BPT trap

ditto 1.9.2 still.

bug 534308 states that the regression from bug 534051 prevents tests involving scatter from running on 1.9.1 or 1.9.2. Can't verify.

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