526449 - FinishSharingTitle should skip fixed slots for slow array

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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]

Attachment: 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]

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)]

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]

Attachment: 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]

Attachment: v3

sync with tm tip

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 417332 [details] [diff] [review]
v3

r=me, thanks.

/be

Comment 7

[Igor Bukanov]

https://hg.mozilla.org/tracemonkey/rev/c9dc5cdf3d8e

Comment 8

[Igor Bukanov]

Attachment: v3 for 192

The version for 1.9.2 required a trivial backporting.

Comment 9

[Daniel Veditz [:dveditz]]

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 10

[Robert Sayre]

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

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Will be back to do the approval requests shortly!

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

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]]

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]]

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] (inactive)]

I don't have anything. Comment 0 appears to be it.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 417370 [details] [diff] [review]
v3 for 192

a=beltzner

Comment 17

[Igor Bukanov]

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

Comment 18

[Bob Clary [:bc] (inactive)]

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] (inactive)]

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]

(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] (inactive)]

(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]

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]]

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]

Define pronto. Igor is probably offline (Europe time). Is this needed today?

Comment 25

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

It is the last or next to last bug waiting for code complete.

Comment 26

[Igor Bukanov]

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]

Attachment: fix for 191

The patch just skips the slots in question.

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 442377 [details] [diff] [review]
fix for 191

Safe spot-fix. r=me.

/be

Comment 29

[LegNeato]

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]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/58f03b5d4931

Comment 31

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

Bob, can you check this on 1.9.1?

Comment 32

[Bob Clary [:bc] (inactive)]

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.