888117 - GC: properly barrier the JSON stringifier cycle detection set

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

Christian found a non-reducable testcase when fuzzing GGC that stems from lack of a post barrier on the JSON stringifier's cycle detection HashSet. The specific GGC hazard here was triggered through a minor GC through the recursion check. I don't see any reason that a normal full GC would be prevented here, so I think this is an incremental GC hazard as well, thus the sec-sensitive flag. I'll let Bill comment further on whether this is an actual issue and, if so, how severe it is.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The pre-barrier is not needed here. Only data structures that are traced by the GC need pre-barriers, and this one never gets traced. (The stack doesn't count because it's traced entirely in the first slice.)

Why did you split the HashTable barrier code into separate pieces for HashSet and HashMap? Is that needed for this patch?

Also, I don't totally understand why this would cause a failure. As far as I can tell, we never dereference the objects in this HashSet. Was it a crash you were seeing, or a JS error? I could understand if we were incorrectly getting JSMSG_CYCLIC_VALUE. I don't see how we would get a crash, though.

Comment 2

[Terrence Cole [:terrence]]

(In reply to Bill McCloskey (:billm) [away until July 1] from comment #1)
> The pre-barrier is not needed here. Only data structures that are traced by
> the GC need pre-barriers, and this one never gets traced. (The stack doesn't
> count because it's traced entirely in the first slice.)

Ah, it was not clear to me that this structure is traced through MarkRuntime.
 
> Why did you split the HashTable barrier code into separate pieces for
> HashSet and HashMap? Is that needed for this patch?

Yes. The HashMap needs to know the map's Value type so that it can put the value on the stack while we change the key. Now that Jon has added a rekey method to HashMap, I think we can get around this. I'll get a new version up with this change.

> Also, I don't totally understand why this would cause a failure. As far as I
> can tell, we never dereference the objects in this HashSet. Was it a crash
> you were seeing, or a JS error? I could understand if we were incorrectly
> getting JSMSG_CYCLIC_VALUE. I don't see how we would get a crash, though.

We were hitting an assertion in debug builds. We move some of the items the set is referring to in the middle of the recursion. The removals do not find the objects in the table and we assert at the end because the table is not empty.

Comment 3

[Terrence Cole [:terrence]]

Attachment: v1

Sorry for the lack of context on the previous patch. The reworked patch is vastly simpler. This new patch:

  - Simplifies and makes HashTableKeyRef fully generic using the new HashTable::rekey method. This includes implementing rekey for HashSet: a direct copy of HashMap::rekey.

  - Improves the documentation on HashKeyRef and HashTablePostWriteBarrier.

  - Removes IsRelocatableHeapType. If it didn't work to catch this bug, it is totally useless.

  - Replaces the duplicate CycleDetector class in json.cpp with the common and well-tested one in jscntxt.h. Now that this uses the rooting from AutoCycleDetector, the original crash is gone without having to add any new rooting primitives.

Comment 4

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7813225999af

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7813225999af