Closed Bug 946243 Opened 10 years ago Closed 10 years ago

Crash with SIGTRAP and prototype

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla29
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 --- fixed
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed

People

(Reporter: decoder, Assigned: h4writer)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [jsbugmon:update][qa-])

Attachments

(1 file)

The following testcase crashes on mozilla-central revision 9688476c1544 (run with --fuzzing-safe --ion-eager):


function $ERROR(message) {}
function Foo() {
  var x = this.property;
  glob = x;
}
Foo.prototype.property = 10;
for (var i = 0; i < 10; i++) {
  new Foo();
  Foo ($ERROR, 10);
}
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, failed due to error (try manually).
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/39bfcadd6492
user:        Hannes Verschore
date:        Tue Nov 26 23:21:18 2013 +0100
summary:     Bug 942105 - IonMonkey: Remove the inlineUseCountRatio option, r=jandem

Hannes, is bug 942105 a likely regressor?
Blocks: 942105
Flags: needinfo?(hv1989)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
> Hannes, is bug 942105 a likely regressor?

There is a big chance it isn't the regressor. We are inlining more cases by removing that. Could be an edgecase that already existed from before. Gonna have a quick look.
Flags: needinfo?(hv1989)
Using bug 947188, I see:
jit/BaselineIC.cpp:8287 Failed to return object in constructing call.

So I'll add djvj since this breakpoint is in baseline.
Flags: needinfo?(kvijayan)
During "EliminateDeadResumePointOperands" we eliminate the value of "this", if the value is MComputeThis.
In other cases "this" could only be MParameter or MTypeBarrier (I think). That is covered.

I now did the easy fix, by just adding MComputeThis to the ignore list of "EliminateDeadResumePointOperands".
Not sure if we want more extensive and less error prone code. I'm ok with that. Let's discuss!
(But plz in other bug. As I want to backport this 'easy' fix)
Blocks: 913424
No longer blocks: 942105
Flags: needinfo?(kvijayan)
Attached patch PatchSplinter Review
Assignee: general → hv1989
Attachment #8344412 - Flags: review?(jdemooij)
Comment on attachment 8344412 [details] [diff] [review]
Patch

Review of attachment 8344412 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh, thanks for fixing this.
Attachment #8344412 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/6683ed2feb75

Given that this doesn't have a sec rating and affects multiple branches, it shouldn't have landed without security approval.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
This landed prior to the merge (from m-c to aurora), but at landing, there was not enough time for a inbound to m-c merge to make the aurora merge.

(It seemed to only affect version 28, 29 didn't "exist" upon landing to inbound, if this makes more sense)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #11)
> (It seemed to only affect version 28, 29 didn't "exist" upon landing to
> inbound, if this makes more sense)

According to Hannes, this affects 26 and 27 too. See the status flags as set after comment 8.
Oh, you're right. I thought the regressor was bug 942105 as opposed to bug 913424.
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
According to Hannes, this will always cause an int3, nothing else, so this is not a security bug.
Group: core-security
(In reply to Christian Holler (:decoder) from comment #15)
> According to Hannes, this will always cause an int3, nothing else, so this
> is not a security bug.

Oh, just for completeness, in optimized builds this won't cause an int3. It will only give undefined as return value to "new Foo();" in some edge-cases that should be hard to reproduce. But also in that case not a security bug.
Comment on attachment 8344412 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
bug 913424

User impact if declined:
In some edge edge edge cases returning "undefined" instead of "object" with "new Foo();".

Testing completed (on m-c, etc.):
m-i for a day

Risk to taking this patch (and alternatives if risky):
Very low risk. The original code did some optimizations. We now don't do that optimization and take the regular path.

String or IDL/UUID changes made by this patch:
/
Attachment #8344412 - Flags: approval-mozilla-beta?
Attachment #8344412 - Flags: approval-mozilla-aurora?
Attachment #8344412 - Flags: approval-mozilla-beta?
Attachment #8344412 - Flags: approval-mozilla-beta+
Attachment #8344412 - Flags: approval-mozilla-aurora?
Attachment #8344412 - Flags: approval-mozilla-aurora+
I assume b2g26 and b2g28 right? If still possible that would be great to get fixed there too.
Flags: needinfo?(hv1989)
(In reply to Hannes Verschore [:h4writer] from comment #19)
> I assume b2g26 and b2g28 right? If still possible that would be great to get
> fixed there too.

b2g28 = Aurora for this cycle, so that's already taken care of. Can you nominate the patch for b2g26?
Comment on attachment 8344412 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
bug 913424

User impact if declined:
In some edge edge edge cases returning "undefined" instead of "object" with "new Foo();".

Testing completed (on m-c, etc.):
m-i for a day

Risk to taking this patch (and alternatives if risky):
Very low risk. The original code did some optimizations. We now don't do that optimization and take the regular path.

String or IDL/UUID changes made by this patch:
/
Attachment #8344412 - Flags: approval-mozilla-b2g26?
I don't think this needs QA verification. If anyone thinks that's a mistake please remove the [qa-] whiteboard tag and add the verifyme keyword.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][qa-]
Who do I need to poke for approval-mozila-b2g26 ??
Trying nominating for koi? - some B2G folk might look at this.
blocking-b2g: --- → koi?
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #24)
> Trying nominating for koi? - some B2G folk might look at this.

What's the critical need to block on this? I am seeing edge of edge case, which implies a non-blocker.
> What's the critical need to block on this? I am seeing edge of edge case,
> which implies a non-blocker.

(In reply to Hannes Verschore [:h4writer] from comment #19)
> I assume b2g26 and b2g28 right? If still possible that would be great to get
> fixed there too.

Hannes would be the right person for this.
Flags: needinfo?(hv1989)
(In reply to Jason Smith [:jsmith] from comment #25)
> What's the critical need to block on this? I am seeing edge of edge case,
> which implies a non-blocker.

I'm not sure what the policy is for b2g26. I know it is an edgecase, but than again, b2g is only executing JS. Making it use it extensively. So could potentially cause strange behaviour for b2g. Now the fix is very straightforward and definitely without side-effects or potential regressions. It's a one-line patch too. 
So if we still take correction patches for b2g26, this would be a easy fix for potential very strange and hard to debug issues.
Flags: needinfo?(hv1989)
Historically for branches (even ESR) we've taken small fixes for fuzzing bugs no matter how edge-case they are, because should they pop out in real life eventually (which they have), it takes up an immense amount of developer resources to triage, reduce and debug the testcase only to find that they're the same issue. (which is a waste of time)

Not sure if this applies to b2g26 (or any other b2g branch), but I really don't see a difference.
Fair enough - let's take the patch then.
blocking-b2g: koi? → koi+
Attachment #8344412 - Flags: approval-mozilla-b2g26?
You need to log in before you can comment on or make changes to this bug.