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

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

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.

Comment 1

[Terrence Cole [:terrence]]

Attachment: fire_barriers_on_heap_function-v0.diff

I hate to duplicate this much code, but I don't see another way.

Comment 2

[Jon Coppeard (:jonco)]

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

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

Ah, good catch.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: add-heap-test

Here's some test code for this.

Comment 4

[Terrence Cole [: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.

Comment 5

[Terrence Cole [:terrence]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1fbc93b30f16
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dd07ffb3af82

Comment 6

[Steve Fink [:sfink] [:s:]]

Namespacing problems hidden by unified build:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0abbe272076

Comment 7

[Ed Morley [:emorley]]

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

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] from comment #6)
Ugh, thanks for fixing that.

Comment 9

[Terrence Cole [:terrence]]

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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Aurora bustage follow-up:
https://hg.mozilla.org/releases/mozilla-aurora/rev/248da3015a51

Backed out from beta for other bustage:
https://hg.mozilla.org/releases/mozilla-beta/rev/23c915c46d6e

https://tbpl.mozilla.org/php/getParsedLog.php?id=42554294&tree=Mozilla-Beta

Comment 12

[Terrence Cole [:terrence]]

Attachment: 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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Terrence landed the beta patch:
https://hg.mozilla.org/releases/mozilla-beta/rev/efbf65d0d3a0

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

I suspect this is not affecting ESR24 because Generational GC is newer than that, but can someone confirm?

Comment 15

[Terrence Cole [: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.

Comment 16

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking [qa-] for verification purposes. Feel free to add STR or other info if you feel you need QA to test this. Thank you.

Comment 17

[Daniel Veditz [:dveditz]]

If GGC landed in Firefox 31 (bug 619558) then b2g28 and b2g30 (1.3/1.4) don't need this fix