Closed Bug 780274 Opened 12 years ago Closed 12 years ago

JM/IonMonkey: Crash [@ js::mjit::EnterMethodJIT] or "Assertion failure: info.isValid(),"

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86_64
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla17
Tracking Status
firefox17 --- affected
firefox-esr10 --- unaffected

People

(Reporter: gkw, Assigned: nbp)

References

Details

(5 keywords, Whiteboard: [ion:p1:fx18])

Crash Data

Attachments

(3 files, 2 obsolete files)

Attached file fragile testcase
The attached testcase asserts js debug shell on IonMonkey changeset b457b592f609 with --no-ion and -a at Assertion failure: info.isValid(),

Pretty sure this is a regression (not bisecting because the fragile testcase won't give accurate results), s-s because there seems to be a schedulegc call in the testcase.

Bug 779124 is similar but the testcase there seems to no longer reproduce.
Attached file stacks
(I assigned it to Nicolas at his request)

These are stacks from debug and opt shells.
Also assuming sec-critical.
Crash Signature: [@ js::mjit::EnterMethodJIT]
Keywords: crash, sec-critical
Summary: IonMonkey: "Assertion failure: info.isValid()," → IonMonkey: Crash [@ js::mjit::EnterMethodJIT] or "Assertion failure: info.isValid(),"
Whiteboard: [ion:p1:fx18]
This patch add forgotten free-rules.
Attachment #649464 - Flags: review?(dvander)
This bug affect both ff17 and IonMonkey.  I will port the current patch on top of mozilla-central.
Depends on: 772509
Attachment #649464 - Flags: review?(dvander) → review?(bhackett1024)
Blocks: 772509
No longer depends on: 772509
Comment on attachment 649464 [details] [diff] [review]
Invalidate & Remove pending compilation when sweeping.

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

What is the problem here, and how does this patch fix it?  This logic looks like it should belong somewhere else; I don't think any direct changes to compartment sweeping should be necessary.
(In reply to Brian Hackett (:bhackett) from comment #6)
> Comment on attachment 649464 [details] [diff] [review]
> Invalidate & Remove pending compilation when sweeping.
> 
> Review of attachment 649464 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What is the problem here, and how does this patch fix it?  This logic looks
> like it should belong somewhere else; I don't think any direct changes to
> compartment sweeping should be necessary.

One of the option would to move that to the discardJitCode function.  And instead of iterating on CellIterUnderGC we can just iterate on the list of CompilerOutput and invalidate all of them at the same time.
± Move the sweeping of the compiler outputs and of pending recompilations to the sweepCompilerOutputs function and call it from the discardJitCode function.
Attachment #649464 - Attachment is obsolete: true
Attachment #649464 - Flags: review?(bhackett1024)
Attachment #649816 - Flags: review?(bhackett1024)
Comment on attachment 649816 [details] [diff] [review]
Invalidate & Remove pending compilation when sweeping.

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

::: js/src/jsinfer.cpp
@@ +5733,5 @@
> +            fop->delete_(constrainedOutputs);
> +            constrainedOutputs = NULL;
> +        } else {
> +            // A Compilation is running and the AutoEnterCompilation class has
> +            // captured an index into the constrainted ouputs vector and

typos
Attachment #649816 - Flags: review?(bhackett1024) → review+
This modifications coming from patch made for Bug 777537 should avoid the error which appear when the script is garbage collected and we assert for the valididty of the script by reading from the JSScript.
Attachment #650676 - Flags: review?(bhackett1024)
Adding the JM to the name so we don't just ignore this in security triage.
Summary: IonMonkey: Crash [@ js::mjit::EnterMethodJIT] or "Assertion failure: info.isValid()," → JM/IonMonkey: Crash [@ js::mjit::EnterMethodJIT] or "Assertion failure: info.isValid(),"
Attachment #650676 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/b6319530d74c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The latest patch cause more additional issues while checking the assertions.  Bug 777537 will bring the necessary fixes to avoid any dangling pointers.

Closing this Bug since it has already landed on central and that the latest patch cannot apply on inbound/central without plenty of issues in the test suite.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Comment on attachment 650676 [details] [diff] [review]
Avoid keeping a danling pointer to a script after a sweep phase.

Should this patch now be obsoleted.
Blocks: 780549
Comment on attachment 650676 [details] [diff] [review]
Avoid keeping a danling pointer to a script after a sweep phase.

This patch is obsolete because Bug 777537 provide a better solution.
Attachment #650676 - Attachment is obsolete: true
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: core-security
You need to log in before you can comment on or make changes to this bug.