Closed
Bug 1019810
Opened 10 years ago
Closed 10 years ago
Consider just crashing in InlineFrameIterator::findNextFrame() when numActualArgs_ fails to get initialized
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: mccr8, Assigned: amit001, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: coverity, Whiteboard: [good first bug][CID 1221251])
Attachments
(1 file, 3 obsolete files)
2.03 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
This gets checked in debug builds, so I doubt it is very likely, but I can't imagine there's any great advantage to rely on asserts here. It might make more sense to just crash if we fail to initialize.
Comment 1•10 years ago
|
||
mccr8: adding you as mentor, farming it out.
Whiteboard: [mentor=mccr8] [good first bug]
Comment 2•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #0) > This gets checked in debug builds, so I doubt it is very likely, but I can't > imagine there's any great advantage to rely on asserts here. It might make > more sense to just crash if we fail to initialize. I guess coverity is not aware of the inlining rules of IonMonkey and assume that the assertion can fail? This code is not performance critical, so checking this condition in release builds sounds fine to me. The idea of this bug is to change the assertion[1] such as we also verify it during release builds. [1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonFrames.cpp?from=findNextFrame#1824
Reporter | ||
Updated•10 years ago
|
Whiteboard: [mentor=mccr8] [good first bug] → [mentor=mccr8] [good first bug][CID 1221251]
Updated•10 years ago
|
Mentor: continuation
Whiteboard: [mentor=mccr8] [good first bug][CID 1221251] → [good first bug][CID 1221251]
Assignee | ||
Comment 3•10 years ago
|
||
Attached the Patch. Crashes in occurs when numActualArgs_ fails to get initialized InlineFrameIterator::findNextFrame() .
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8477957 [details] [diff] [review] bug-1019810-fix.patch Review of attachment 8477957 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonFrames.cpp @@ +1879,2 @@ > #ifdef DEBUG > numActualArgs_ = 0xbadbad; You need to remove the ifdef DEBUG from here or it won't get set in regular builds.
Assignee | ||
Comment 5•10 years ago
|
||
Removed the #ifdef DEBUG condition.
Attachment #8477957 -
Attachment is obsolete: true
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → mail2amit19
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8477984 [details] [diff] [review] bug-1019810.patch Fixed the DEBUG version condition Review of attachment 8477984 [details] [diff] [review]: ----------------------------------------------------------------- That looks reasonable to me. I'll ask Hannes for review because he is a JS JIT peer and has reviewed patches for this file before.
Attachment #8477984 -
Flags: review?(hv1989)
Comment 7•10 years ago
|
||
Comment on attachment 8477984 [details] [diff] [review] bug-1019810.patch Fixed the DEBUG version condition Review of attachment 8477984 [details] [diff] [review]: ----------------------------------------------------------------- Looks good ;). I'm mostly nitpicking. ::: js/src/jit/IonFrames.cpp @@ +1902,5 @@ > } else if (IsSetPropPC(pc_)) { > numActualArgs_ = 1; > } > > + if(numActualArgs_ == 0xbadbad) Style nit: there needs to be a space between the "if" and "(" @@ +1903,5 @@ > numActualArgs_ = 1; > } > > + if(numActualArgs_ == 0xbadbad) > + MOZ_CRASH(); MOZ_CRASH usually contains a reason for the crash as first parameter. Can you also add one?
Attachment #8477984 -
Flags: review?(hv1989)
Assignee | ||
Comment 8•10 years ago
|
||
Added the reason parameter in MOZ_CRASH . Style nit corrected.
Attachment #8477984 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8478226 -
Flags: review+
Assignee | ||
Comment 9•10 years ago
|
||
Updated•10 years ago
|
Attachment #8478226 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8478233 -
Flags: review+
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/427a731c9c75
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•6 years ago
|
Blocks: coverity-analysis
You need to log in
before you can comment on or make changes to this bug.
Description
•