Closed Bug 817343 Opened 7 years ago Closed 7 years ago

GC validation isn't happening

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: billm, Assigned: jonco)

Details

Attachments

(2 files, 3 obsolete files)

Attached patch patchSplinter Review
We're not actually calling ValidateIncrementalMarking right now. It only runs if gcCompartmentGroupIndex == 0. However, the first group to be swept has that set to 1, since we call GetNextCompartmentGroup in BeginSweepPhase.

When I fixed this, the checks fail. I noticed that we're no longer in a good position to check gray marking anymore, since at best we'd only be checking the first compartment group anyway. So I took these checks out.

It's also a bit troublesome to check weak marking, since that's now done on a per-compartment group basis. This has never found any bugs, and it requires some annoying code to deal with saving and restoring the weakmap list. So I took that out too.

With these changes, the checks seem to pass.
Attachment #687458 - Flags: review?(jcoppeard)
Comment on attachment 687458 [details] [diff] [review]
patch

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

Sorry, I realise I broke this.  Fix looks good, it's a shame we can't assert the gray marking worked though.
Attachment #687458 - Flags: review?(jcoppeard) → review+
Jon, if you have some extra time, these assertions are interesting. They also might be related to bug 817342.
This is really inefficient because it does a full non-incremental mark for every compartment group, although this could be improved.  

Also not heavily tested.

Try push is here: https://tbpl.mozilla.org/?tree=Try&rev=a5dd2304aafb
Here's an updated version of this patch.  Still expecting some failures but should not be so inefficient as the marking's only done once.
Attachment #688855 - Attachment is obsolete: true
Attached patch Validate every compartment group (obsolete) — Splinter Review
I've removed all gray marking validation from this one, and I'm going to try and address that in a separate patch.
Attachment #689287 - Attachment is obsolete: true
Attachment #690444 - Flags: review?(wmccloskey)
Comment on attachment 690444 [details] [diff] [review]
Validate every compartment group

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

This looks great, but I'd like to see how you handle the MarkWeakReferences issue.

::: js/src/jsgc.cpp
@@ +2754,5 @@
> +
> +namespace js {
> +namespace gc {
> +
> +class MarkingValidator

As long as this has been forward-declared, I think you can do |class js::gc::MarkingValidator| and remove the namespace scopes.

@@ +2780,5 @@
> +{}
> +
> +js::gc::MarkingValidator::~MarkingValidator()
> +{
> +    if (map.initialized()) {

Could you do this as:
  if (!map.initialized())
      return;

@@ +2781,5 @@
> +
> +js::gc::MarkingValidator::~MarkingValidator()
> +{
> +    if (map.initialized()) {
> +        for (BitmapMap::Range r(map.all()); !r.empty(); r.popFront()) {

No braces needed.

@@ +2808,2 @@
>          ChunkBitmap *bitmap = &r.front()->bitmap;
> +	ChunkBitmap *entry = new ChunkBitmap();

Please use |js_new<ChunkBitmap>()| here.

@@ +2822,5 @@
>      WeakMapVector weakmaps;
> +    ArrayBufferVector arrayBuffers;
> +    for (GCCompartmentsIter c(runtime); !c.done(); c.next()) {
> +        if (!WeakMapBase::saveCompartmentWeakMapList(c, weakmaps) ||
> +            !ArrayBufferObject::saveArrayBufferList(c, arrayBuffers)) {

The brace should go on its own line since the condition takes up two lines.

@@ +2864,5 @@
> +    runtime->gcMarker.drainMarkStack(budget);
> +
> +    {
> +        gcstats::AutoPhase ap(runtime->gcStats, gcstats::PHASE_SWEEP);
> +        MarkWeakReferences(runtime, gcstats::PHASE_SWEEP_MARK_WEAK);

This function uses CompartmentGroupIter, so I don't think it's doing what we need here. You need a special version that iterates over all compartments.

One terrifying suggestion would be to templatize MarkWeakReferences on the iterator class :-).

@@ +2875,5 @@
> +        ChunkBitmap *entry = map.lookup(chunk)->value;
> +
> +	ChunkBitmap tmp = *entry;
> +        *entry = *bitmap;
> +        *bitmap = tmp;

I think you can use js::Swap (from jsutil.h) to make this a little clearer.

@@ +2900,5 @@
> +
> +    if (!initialized)
> +        return;
> +
> +    unsigned count = 0;

This looks dead.

@@ +2944,4 @@
>  #endif
> +
> +static void
> +ComputeNonIncrementalMarking(JSRuntime *rt)

Maybe call this ComputeNonIncrementalMarkingForValidation? It's long, but we only call it once.

@@ +2948,5 @@
> +{
> +#ifdef DEBUG
> +    JS_ASSERT(!rt->gcMarkingValidator);
> +    if (rt->gcIsIncremental && rt->gcValidate)
> +        rt->gcMarkingValidator = new MarkingValidator(rt);

We're not allowed to use |new| directly because of exception safety or something. Please replace with |js_new<MarkingValidator>(rt)|.

@@ +3516,5 @@
>  static bool
>  SweepPhase(JSRuntime *rt, SliceBudget &sliceBudget)
>  {
> +#ifdef DEBUG
> +    MarkingValidator validator(rt);

What's this for?

::: js/src/jstypedarray.h
@@ +135,5 @@
>      static void sweep(JSCompartment *rt);
>  
>      static void resetArrayBufferList(JSCompartment *rt);
>  
> +    static bool saveArrayBufferList(JSCompartment *c, ArrayBufferVector &vector);

Could you remove the blank lines between reset, save, and restore?
Attachment #690444 - Flags: review?(wmccloskey)
New patch with review comments addressed.

Thanks for the idea about templating on the iterator class, I would not have thought of that ;)  But actually it expresses the intent, and I can't think of a cleaner alternative.
Attachment #690444 - Attachment is obsolete: true
Attachment #690851 - Flags: review?(wmccloskey)
Comment on attachment 690851 [details] [diff] [review]
Validate every compartment group

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

Looks great. Thanks!

::: js/src/jsgc.cpp
@@ +2697,5 @@
>  
>      return true;
>  }
>  
> +template <class CompartmentIter>

This name is a little confusing given the existing CompartmentsIter. How about calling it CompartmentIterT? That will make people notice it more.
Attachment #690851 - Flags: review?(wmccloskey) → review+
Assignee: wmccloskey → jcoppeard
https://hg.mozilla.org/mozilla-central/rev/421ea4f8b050
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.