Closed Bug 1523558 Opened 1 year ago Closed 1 year ago

VMFunction::returnsData() tests for incorrect return-type

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 66+ fixed
firefox65 --- wontfix
firefox66 + fixed
firefox67 + fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(1 file)

The returnType == Type_Pointer condition in this test can never be true, because returnType is required to be either Type_Void, Type_Bool, or Type_Object. I assume the correct test is to check for Type_Object in VMFunction::returnsData().

Attached patch bug1523558.patchSplinter Review

I hope I get a carte blanche for any performance regressions which might happen here, just like happened for the initial issue at bug 1444856 comment 2.

Attachment #9039751 - Flags: review?(nicolas.b.pierron)
Comment on attachment 9039751 [details] [diff] [review]
bug1523558.patch

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

(In reply to André Bargull [:anba] from comment #1)
> I hope I get a carte blanche for any performance regressions which might happen here, just like happened for the initial issue at bug 1444856 comment 2.

Yes. We should probably backport these changes too.
Attachment #9039751 - Flags: review?(nicolas.b.pierron) → review+
Keywords: checkin-needed

Comment on attachment 9039751 [details] [diff] [review]
bug1523558.patch

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1438886

User impact if declined

Incomplete Spectre mitigation after execution of C++ calls, cf. bug 1438886.

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

No

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

The change is non-risky, because it only ensures a speculation barrier (lfence on x86, csdb on ARM) is emitted. It may lead to a performance regression, though.

String changes made/needed

None.

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration

Incomplete Spectre mitigation after execution of C++ calls, cf. bug 1438886.

User impact if declined

See Beta approval.

Fix Landed on Version

66/67

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

See Beta approval.

String or UUID changes made by this patch

None.

Attachment #9039751 - Flags: approval-mozilla-esr60?
Attachment #9039751 - Flags: approval-mozilla-beta?

Andre, do you need this landed on inbound also?

Flags: needinfo?(andrebargull)

(In reply to Cosmin Sabou [:CosminS] from comment #4)

Andre, do you need this landed on inbound also?

Yes, first on inbound, please. (And after the Beta/ESR requests are approved also there.)

Flags: needinfo?(andrebargull)

Pushed by opoprus@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74d7d41da9e4
Fix test condition in VMFunction::returnsData(). r=nbp

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Comment on attachment 9039751 [details] [diff] [review]
bug1523558.patch

Spectre mitigation, ok for uplift to beta 4.
Attachment #9039751 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1524482
Comment on attachment 9039751 [details] [diff] [review]
bug1523558.patch

Spectre mitigation improvement. Approved for 60.6esr.
Attachment #9039751 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.