If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Consider replacing abort() in AvmAssertFail

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: gkw, Assigned: gkw)

Tracking

Trunk
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tamarin)

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
The abort() call in AvmAssertFail could be replaced by the components in JS_Assert. Currently abort() calls cause testcase reduction issues (in Lithium) in Windows when a LIR assertion is hit.

http://mxr.mozilla.org/mozilla-central/source/js/src/nanojit/avmplus.cpp#65

http://mxr.mozilla.org/mozilla-central/source/js/src/jsutil.cpp#67

Comment 1

7 years ago
We own that file, so we can patch it at will. Go for it :)
Assignee: general → gary
(Assignee)

Comment 2

7 years ago
Created attachment 450039 [details] [diff] [review]
take one
Attachment #450039 - Flags: review?(gal)
(Assignee)

Comment 3

7 years ago
Created attachment 450040 [details] [diff] [review]
ignore whitespace nits
Attachment #450039 - Attachment is obsolete: true
Attachment #450040 - Flags: review?(gal)
Attachment #450039 - Flags: review?(gal)
(Assignee)

Comment 4

7 years ago
Created attachment 450042 [details] [diff] [review]
side patch

Does this side patch make sense? I'd thought to synchronize the "Assertion failed" vs "Assertion failure" message, but this doesn't seem important except from a cosmetic point-of-view, and I'm not sure if there was any reason for calling it the former rather than the latter.
Attachment #450042 - Flags: feedback?(gal)

Comment 5

7 years ago
Comment on attachment 450042 [details] [diff] [review]
side patch

nanojit.h is shared with adobe. The patch is fine but there is a certain process to apply it (commit to nanojit-central). njn knows the drill.
Attachment #450042 - Flags: feedback?(gal) → review+

Comment 6

7 years ago
Comment on attachment 450040 [details] [diff] [review]
ignore whitespace nits

Looks good.
Attachment #450040 - Flags: review?(gal) → review+
(Assignee)

Comment 7

7 years ago
(In reply to comment #6)
> (From update of attachment 450040 [details] [diff] [review])
> Looks good.

http://hg.mozilla.org/tracemonkey/rev/2ae7cb9510b3
Whiteboard: fixed-in-tracemonkey
I pushed these for Gary:

http://hg.mozilla.org/projects/nanojit-central/rev/c7e84fb8ce71
http://hg.mozilla.org/projects/nanojit-central/rev/49982fe529dd
Whiteboard: fixed-in-tracemonkey → fixed-in-nanojit
Created attachment 450070 [details] [diff] [review]
Quick fix to include signal.h.

The previous patch used "raise" and "SIGABRT", but didn't include signal.h. This broke the nanojit-central build both on ARM/Ubuntu and x64/Ubuntu, so I've pushed a trivial patch to fix it.

http://hg.mozilla.org/projects/nanojit-central/rev/0a8efeddb4d9
(Assignee)

Comment 10

7 years ago
Thanks Jacob!

Comment 11

7 years ago
TR: http://hg.mozilla.org/tamarin-redux/rev/47d0a7afb559
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tamarin

Comment 12

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

Comment 13

7 years ago
http://hg.mozilla.org/mozilla-central/rev/a0eff38ab571
You need to log in before you can comment on or make changes to this bug.