We are not firing any post-barriers on Heap<T> for JSFunction*

RESOLVED FIXED in Firefox 31, Firefox OS v2.0

Status

()

Core
JavaScript: GC
--
major
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug, {sec-high})

Trunk
mozilla33
sec-high
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox31 fixed, firefox32 fixed, firefox33 fixed, firefox-esr24 unaffected, b2g-v1.2 unaffected, b2g-v1.3 unaffected, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [qa-][adv-main31+])

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
JSFunction is a sub-class of JSObject, but gecko users of RootingAPI.h have no way of knowing this. The upshot is that we end up using GCMethods<JSFunction*>::needsPostBarrier (which returns false unconditionally), instead of GCMethods<JSObject*>::needsPostBarrier. We absolutely allocate some JSFUnction in the nursery, so I guess we're just getting mostly lucky right now.

Luckily, this is only a problem for Heap<T>: the internal barrier classes use InternalGCMethods<T>, which dispatches to T::needsPostBarrier, which will follow the class hierarchy and always find JSObject::needsPostBarrier.

The trivial fix is to dup GCMethods<JSObject*> for JSFunction*. If there were other gecko-exposed object users of GCMethods<T>, they would have to be in SpecificRootKind too, so we can be fairly confident that JSFunction is the only subclass we need to worry about.

Will patch after lunch.
(Assignee)

Updated

4 years ago
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox33: --- → affected
(Assignee)

Comment 1

4 years ago
Created attachment 8443689 [details] [diff] [review]
fire_barriers_on_heap_function-v0.diff

I hate to duplicate this much code, but I don't see another way.
Attachment #8443689 - Flags: review?(jcoppeard)
(Assignee)

Updated

4 years ago
Severity: normal → major
Component: JavaScript Engine → JavaScript: GC
Keywords: sec-high
Comment on attachment 8443689 [details] [diff] [review]
fire_barriers_on_heap_function-v0.diff

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

Ah, good catch.
Attachment #8443689 - Flags: review?(jcoppeard) → review+
Created attachment 8444452 [details] [diff] [review]
add-heap-test

Here's some test code for this.
Attachment #8444452 - Flags: review?(terrence)
(Assignee)

Comment 4

4 years ago
Comment on attachment 8444452 [details] [diff] [review]
add-heap-test

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

Great idea! I'll push adjacent to the other.
Attachment #8444452 - Flags: review?(terrence) → review+

Comment 7

4 years ago
https://hg.mozilla.org/mozilla-central/rev/1fbc93b30f16
https://hg.mozilla.org/mozilla-central/rev/dd07ffb3af82
https://hg.mozilla.org/mozilla-central/rev/c0abbe272076
https://hg.mozilla.org/mozilla-central/rev/db3e7febe0a4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox33: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(In reply to Steve Fink [:sfink] from comment #6)
Ugh, thanks for fixing that.
(Assignee)

Comment 9

4 years ago
Comment on attachment 8443689 [details] [diff] [review]
fire_barriers_on_heap_function-v0.diff

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Generational GC.
User impact if declined: Random crashes.
Testing completed (on m-c, etc.): On m-c with complete unit tests.
Risk to taking this patch (and alternatives if risky): Low to none.
String or IDL/UUID changes made by this patch: None.
Attachment #8443689 - Flags: approval-mozilla-beta?
Attachment #8443689 - Flags: approval-mozilla-aurora?
Attachment #8443689 - Flags: approval-mozilla-beta?
Attachment #8443689 - Flags: approval-mozilla-beta+
Attachment #8443689 - Flags: approval-mozilla-aurora?
Attachment #8443689 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/8494ff92590e
https://hg.mozilla.org/releases/mozilla-aurora/rev/7ee79c993016

https://hg.mozilla.org/releases/mozilla-beta/rev/759a69c36dcf
https://hg.mozilla.org/releases/mozilla-beta/rev/be30b037ff07

BTW, this should have had sec-approval before landing since it affects more than just trunk.
status-firefox31: affected → fixed
status-firefox32: affected → fixed
Flags: in-testsuite+
(Assignee)

Comment 12

4 years ago
Created attachment 8448295 [details] [diff] [review]
rebased_for_beta-v0.diff

Beta used a different mechanism for specifying the ThingRootKind and did not have IsInsideNursery(Cell*) exposed yet, so cast a wider net for barriers. The attached patch adjusts for these changes and folds in the test and the 2 followup bustage fixes.

I tested with on jsapi-tests locally with both clang and gcc, so I believe this should be ready for landing.
Attachment #8448295 - Flags: review+
Terrence landed the beta patch:
https://hg.mozilla.org/releases/mozilla-beta/rev/efbf65d0d3a0
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
status-firefox31: affected → fixed
I suspect this is not affecting ESR24 because Generational GC is newer than that, but can someone confirm?
status-firefox-esr24: --- → ?
tracking-firefox-esr24: --- → ?
Flags: needinfo?(terrence)
(Assignee)

Comment 15

4 years ago
(In reply to Lukas Blakk [:lsblakk] from comment #14)
> I suspect this is not affecting ESR24 because Generational GC is newer than
> that, but can someone confirm?

You are correct.
status-firefox-esr24: ? → unaffected
tracking-firefox-esr24: ? → ---
Flags: needinfo?(terrence)
Marking [qa-] for verification purposes. Feel free to add STR or other info if you feel you need QA to test this. Thank you.
Whiteboard: [qa-]
Whiteboard: [qa-] → [qa-][adv-main31+]
If GGC landed in Firefox 31 (bug 619558) then b2g28 and b2g30 (1.3/1.4) don't need this fix
status-b2g-v1.2: --- → unaffected
status-b2g-v1.3: --- → unaffected
status-b2g-v1.4: --- → unaffected

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.