Closed Bug 883490 Opened 7 years ago Closed 7 years ago

OdinMonkey: Crash with FFI call that causes recursion through valueOf

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jruderman, Assigned: h4writer)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Attachments

(1 file)

function coerceForeign(stdlib, foreign)
{
    "use asm";
    var g = foreign.g;
    function f() {
        +g();
    }
    return f;
}
var recursive = {
  valueOf: coerceForeign(this, {g: function() { return recursive; }})
};
"" + recursive;

Assertion failure: v.isObject(), at js/src/jsnum.cpp:1461

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/2810e80e1393
user:        Hannes Verschore
date:        Wed Jun 12 21:58:22 2013 +0200
summary:     Bug 860838: OdinMonkey: Optimize FFI calls to ionmonkey, r=luke
js_ReportOverRecursed called
Assertion failure: v.isObject(), at /var/folders/fy/kwq0h84n5v51zb5llf8mb5b80000gp/T/abtmp-3d16d59c9317-d2U_zv/compilePath/js/src/jsnum.cpp:1461

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
0x00000001001dfe12 in js::ToNumberSlow ()

(gdb) bt
#0  0x00000001001dfe12 in js::ToNumberSlow ()
#1  0x000000010029b923 in ValueToNumber ()
#2  0x00000001014f1291 in ?? ()
#3  0x000000010007b69a in js::CallJSNative ()
#4  0x000000010007552b in js::Invoke ()
#5  0x0000000100075bdc in js::Invoke ()
#6  0x00000001001f1612 in MaybeCallMethod ()
#7  0x00000001001f1109 in js::DefaultValue ()
#8  0x00000001001dfd0b in js::ToNumberSlow ()
#9  0x000000010029b923 in ValueToNumber ()
#10 0x00000001014f1291 in ?? ()
#11 0x000000010007b69a in js::CallJSNative ()
#12 0x000000010007552b in js::Invoke ()
#13 0x0000000100075bdc in js::Invoke ()
#14 0x00000001001f1612 in MaybeCallMethod ()
#15 0x00000001001f1109 in js::DefaultValue ()
#16 0x00000001001dfd0b in js::ToNumberSlow ()
#17 0x000000010029b923 in ValueToNumber ()
#18 0x00000001014f1291 in ?? ()
This is fragile in that adding environment variables can make it go away.  Probably because it changes the amount of stack available and the point at which the recursion limit is hit.
Attached patch PatchSplinter Review
Forgot to check the return value to ion for failures. Noticed that branchTestMagicValue was wrongly implemented for 'Assembler::Equal'. Introduced in FF23. Quick grep shows that this path isn't used yet. So I don't think we should uplift?
Assignee: general → hv1989
Attachment #763256 - Flags: review?(luke)
(In reply to Jesse Ruderman from comment #2)
> This is fragile in that adding environment variables can make it go away. 
> Probably because it changes the amount of stack available and the point at
> which the recursion limit is hit.

(In reply to Hannes Verschore [:h4writer] from comment #3)
> Introduced in FF23. Quick grep shows that this path isn't used yet. So I
> don't think we should uplift?

I'd prefer that this is, if it causes the same intermittent issue in Fx23. As Jesse mentioned, this bug showed the issue intermittently and can confuse our harness infrastructure.
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #4)
> I'd prefer that this is, if it causes the same intermittent issue in Fx23.
> As Jesse mentioned, this bug showed the issue intermittently and can confuse
> our harness infrastructure.

Sorry, I think I didn't make myself clear enough.
1) Issue fixed here is definitely FF24 only. It is indeed intermediate and hard to repro. I could only see it one out of 10 times running it.

2) To fix the issue I reused a function already present in our code. That function was clearly wrongly implemented for a certain case. The function was added in FF23, but that certain case wasn't executed in FF23 or in FF24, until now. So I was talking about not uplifting that change to FF23. Since it isn't exercised.
Okay! That should work, thanks for clarifying.
Comment on attachment 763256 [details] [diff] [review]
Patch

Could you also add a test-case?
Attachment #763256 - Flags: review?(luke) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a42ab5a85e3

(In reply to Luke Wagner [:luke] from comment #7)
> Comment on attachment 763256 [details] [diff] [review]
> Patch
> 
> Could you also add a test-case?

Sure, I didn't add it since the provided testcase is intermediate. Created a new testcase that also fails because of this fault.
Flags: in-testsuite+
Stupidly forgot to annotate script as returning TypeError:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86e5c5133f3f
https://hg.mozilla.org/mozilla-central/rev/2a42ab5a85e3
https://hg.mozilla.org/mozilla-central/rev/86e5c5133f3f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.