Closed Bug 1019810 Opened 6 years ago Closed 6 years ago

Consider just crashing in InlineFrameIterator::findNextFrame() when numActualArgs_ fails to get initialized

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

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)

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.
mccr8: adding you as mentor, farming it out.
Whiteboard: [mentor=mccr8] [good first bug]
(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
Whiteboard: [mentor=mccr8] [good first bug] → [mentor=mccr8] [good first bug][CID 1221251]
Mentor: continuation
Whiteboard: [mentor=mccr8] [good first bug][CID 1221251] → [good first bug][CID 1221251]
Attached patch bug-1019810-fix.patch (obsolete) — Splinter Review
Attached the Patch. Crashes in occurs when numActualArgs_ fails to get initialized InlineFrameIterator::findNextFrame() .
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.
Removed the #ifdef DEBUG condition.
Attachment #8477957 - Attachment is obsolete: true
Assignee: nobody → mail2amit19
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 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)
Attached patch bug-1019810.patch (obsolete) — Splinter Review
Added the reason parameter in MOZ_CRASH .
Style nit corrected.
Attachment #8477984 - Attachment is obsolete: true
Attachment #8478226 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/427a731c9c75
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.