The default bug view has changed. See this FAQ.

FinishSharingTitle should skip fixed slots for slow array

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({crash, testcase})

Trunk
crash, testcase
Points:
---
Bug Flags:
blocking1.9.2 -
wanted1.9.2 +
wanted1.9.0.x +
in-testsuite ?

Firefox Tracking Flags

(blocking2.0 alpha1+, blocking1.9.2 needed, status1.9.2 .4-fixed, blocking1.9.1 needed, status1.9.1 .10-fixed)

Details

(Whiteboard: [sg:vector-critical addons?] fixed-in-tracemonkey)

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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]

Updated

8 years ago
Flags: wanted1.9.2?
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
(Assignee)

Comment 1

7 years ago
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.
(Assignee)

Comment 2

7 years ago
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?
(Assignee)

Comment 4

7 years ago
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.
Attachment #415948 - Attachment is obsolete: true
Attachment #416052 - Flags: review?(brendan)
(Assignee)

Comment 5

7 years ago
Created attachment 417332 [details] [diff] [review]
v3

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+
(Assignee)

Comment 7

7 years ago
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
(Assignee)

Comment 8

7 years ago
Created attachment 417370 [details] [diff] [review]
v3 for 192

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?

Comment 10

7 years ago
http://hg.mozilla.org/mozilla-central/rev/c9dc5cdf3d8e
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
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
status1.9.2: --- → wanted
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?

Comment 15

7 years ago
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+
(Assignee)

Comment 17

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a55dfc2a5475
status1.9.2: wanted → .4-fixed

Comment 18

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

7 years ago
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
(Assignee)

Comment 20

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

7 years ago
(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
(Assignee)

Comment 22

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

Comment 24

7 years ago
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.
(Assignee)

Comment 26

7 years ago
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.
(Assignee)

Comment 27

7 years ago
Created attachment 442377 [details] [diff] [review]
fix for 191

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 29

7 years ago
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+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/58f03b5d4931
status1.9.1: wanted → .10-fixed
Bob, can you check this on 1.9.1?

Comment 32

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