js shell: scatter sets wrong thread stack limit

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({fixed1.9.1})

unspecified
fixed1.9.1
Points:
---
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

v2
4.27 KB, patch
Igor Bukanov
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
Currently SetContextOptions from shell/js.cpp sets the stack limit for the context based on the address of the gStackBase global variable. But this is valid only for the main thread, for threads created through the scatter function this generates the wrong answer meaning that code either miss the proper stack limit or would generate the spurious too-much-recursion errors. The following example demonstrates the first case on Linux:

@watson-32~> cat ~/s/x1.js
function f()
{
    var obj = {};
    obj.toString = function() { return ""+obj; }
    return ""+obj;
}

scatter([Math.sin, f]);
@watson-32~> ~/build/js32.tm.dbg/js ~/s/x1.js
Segmentation fault

I nominate the bug for 1.9.1 as those incorrect recursion errors makes debugging multi-threading problems harder.
Flags: wanted1.9.1?
(Assignee)

Comment 1

9 years ago
Created attachment 369274 [details] [diff] [review]
v1

The patch uses thread-private to store the address of a variable in the top native stack frame and use it to calculate the limit.
Attachment #369274 - Flags: review?(mrbkap)

Updated

9 years ago
Attachment #369274 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 2

9 years ago
landed to TM - http://hg.mozilla.org/tracemonkey/rev/0b36bddcefe4
Whiteboard: fixed-in-tracemonkey
Looks like this broke Windows:

e:/builds/moz2_slave/tracemonkey-win32-nightly/build/js/src/shell/js.cpp(4600) : error C2664: 'PR_NewThreadPrivateIndex' : cannot convert parameter 1 from 'uint32 *' to 'PRUintn *'
        Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1238061720.1238063080.2204.gz#err1

Can someone back it out? I won't get a chance for a couple of hours.
(Assignee)

Comment 4

9 years ago
I backed out the landed patch. I assumed that uint32 is the same as PRUintn. But this is only so under Linux.
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 5

9 years ago
Created attachment 369509 [details] [diff] [review]
v2

The new patch uses the proper type, PRUintn, not uint32, for thread-private index type.
Attachment #369274 - Attachment is obsolete: true
Attachment #369509 - Flags: review+
(Assignee)

Comment 6

9 years ago
re-landed to TM - http://hg.mozilla.org/tracemonkey/rev/72ec438d1934
Whiteboard: fixed-in-tracemonkey

Comment 7

9 years ago
http://hg.mozilla.org/mozilla-central/rev/72ec438d1934
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: wanted1.9.1? → wanted1.9.1+
Resolution: --- → FIXED

Comment 8

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9a0957485ff7
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.