Closed Bug 526449 Opened 11 years ago Closed 11 years ago

FinishSharingTitle should skip fixed slots for slow array

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- alpha1+
blocking1.9.2 --- needed
status1.9.2 --- .4-fixed
blocking1.9.1 --- needed
status1.9.1 --- .10-fixed

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: crash, testcase, Whiteboard: [sg:vector-critical addons?] fixed-in-tracemonkey)

Attachments

(3 files, 2 obsolete files)

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.
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Whiteboard: [sg:vector-critical addons?][3.6.x]
Flags: wanted1.9.2?
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Attached patch v1 (obsolete) — Splinter Review
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.
I nominate this bug for 1.9.0 as it exists on all branches with fast/slow array implementation.
Flags: wanted1.9.0.x?
Moving this from the 1.9.1 needed list to ? again so we can decide if we want it in 1.9.1.7.
blocking1.9.1: needed → ?
Flags: blocking1.9.0.17?
Attached patch v2 (obsolete) — Splinter Review
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.
Attachment #415948 - Attachment is obsolete: true
Attachment #416052 - Flags: review?(brendan)
Attached patch v3Splinter Review
sync with tm tip
Attachment #416052 - Attachment is obsolete: true
Attachment #417332 - Flags: review?(brendan)
Attachment #416052 - Flags: review?(brendan)
Comment on attachment 417332 [details] [diff] [review]
v3

r=me, thanks.

/be
Attachment #417332 - Flags: review?(brendan) → review+
https://hg.mozilla.org/tracemonkey/rev/c9dc5cdf3d8e
Whiteboard: [sg:vector-critical addons?][3.6.x] → [sg:vector-critical addons?][3.6.x] fixed-in-tracemonkey
Attached patch v3 for 192Splinter Review
The version for 1.9.2 required a trivial backporting.
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.
blocking1.9.1: ? → .7+
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.17?
Attachment #417370 - Flags: approval1.9.2?
http://hg.mozilla.org/mozilla-central/rev/c9dc5cdf3d8e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
blocking2.0: ? → alpha1
blocking1.9.1: .8+ → needed
blocking1.9.2: --- → ?
Whiteboard: [sg:vector-critical addons?][3.6.x] fixed-in-tracemonkey → [sg:vector-critical addons?] fixed-in-tracemonkey
Attachment #417370 - Flags: approval1.9.2? → approval1.9.2.2?
Will be back to do the approval requests shortly!
blocking1.9.2: ? → needed
Comment on attachment 417370 [details] [diff] [review]
v3 for 192

a1922=beltzner, after an embarassingly long time
Attachment #417370 - Flags: approval1.9.2.2? → approval1.9.2.2+
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.
Attachment #417370 - Flags: approval1.9.2.3?
Attachment #417370 - Flags: approval1.9.2.2-
Attachment #417370 - Flags: approval1.9.2.2+
Flags: in-testsuite?
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?
I don't have anything. Comment 0 appears to be it.
Comment on attachment 417370 [details] [diff] [review]
v3 for 192

a=beltzner
Attachment #417370 - Flags: approval1.9.2.3? → approval1.9.2.3+
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.
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.
Status: RESOLVED → VERIFIED
(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.
(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
The assert on 1.9.2 is a regression from the bug 534051 that have not yet landed on branches, see bug 534308.
We need a 1.9.1 patch for this, pronto. Sorry, we should have noticed that when we approved it for 1.9.2.
Define pronto. Igor is probably offline (Europe time). Is this needed today?
It is the last or next to last bug waiting for code complete.
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.
Attached patch fix for 191Splinter Review
The patch just skips the slots in question.
Attachment #442377 - Flags: review?(brendan)
Comment on attachment 442377 [details] [diff] [review]
fix for 191

Safe spot-fix. r=me.

/be
Attachment #442377 - Flags: review?(brendan) → review+
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.
Attachment #442377 - Flags: approval1.9.1.10+
Bob, can you check this on 1.9.1?
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.
Group: core-security
You need to log in before you can comment on or make changes to this bug.