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

RESOLVED FIXED in Firefox -esr60

Status

()

defect
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr6066+ fixed, firefox65 wontfix, firefox66+ fixed, firefox67+ fixed)

Details

Attachments

(1 attachment)

Assignee

Description

5 months ago

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().

Assignee

Comment 1

5 months ago

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+
Assignee

Updated

5 months ago
Keywords: checkin-needed
Assignee

Comment 3

5 months ago

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)
Assignee

Comment 5

5 months ago

(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)

Comment 6

5 months ago

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

Comment 7

5 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 5 months 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+
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.