Closed Bug 1028358 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript: GC, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
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

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

(Keywords: sec-high, Whiteboard: [qa-][adv-main31+])

Attachments

(3 files)

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.
I hate to duplicate this much code, but I don't see another way.
Attachment #8443689 - Flags: review?(jcoppeard)
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+
Attached patch add-heap-testSplinter Review
Here's some test code for this.
Attachment #8444452 - Flags: review?(terrence)
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+
(In reply to Steve Fink [:sfink] from comment #6)
Ugh, thanks for fixing that.
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+
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+
I suspect this is not affecting ESR24 because Generational GC is newer than that, but can someone confirm?
Flags: needinfo?(terrence)
(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.
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
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.