Closed Bug 1340604 Opened 3 years ago Closed 3 years ago

Add more test code for weak map gray marking behaviour

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file)

Attached patch test-weakmaps-2Splinter Review
The CC gray marking assertions still complain about black->gray weak map edges, for weak maps not associated with a JSObject, so I added some more test code to exercise this.  It's simpler that the previous case because the map is always marked black.  It didn't catch any issues though.
Attachment #8838627 - Flags: review?(sphink)
Comment on attachment 8838627 [details] [diff] [review]
test-weakmaps-2

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

::: js/src/jsapi-tests/testGCGrayMarking.cpp
@@ +318,5 @@
> +          : CustomAutoRooter(cx), map_(map) {}
> +        void trace(JSTracer* trc) override {
> +            map_->trace(trc);
> +        }
> +    };

There is a *different* way of doing this. I'm not sure if "better" is the right adjective.

Inside of your namespace JS, just after specializing DeletePolicy, you could do:

template <>
struct MapTypeToRootKind<js::ObjectWeakMap*> {
    static const JS::RootKind kind = JS::RootKind::Traceable;
};

template <>
struct GCPolicy<js::ObjectWeakMap*> {
    static void trace(JSTracer* trc, js::ObjectWeakMap** tp, const char* name) {
        (*tp)->trace(trc);
    }
};

and then change RootMap to Rooted<js::ObjectWeakMap*>. It's a bit weird, since you're still doing Rooted management of an external pointer, but not really -- it's basically using Rooted to manage a pointer to a pointer (well, a pointer to a struct containing a pointer), and that's as legit as Rooted management of some random struct.

Alternatively, you could try to tie more directly into the Rooted protocol stuff by not doing any template specialization at all, and instead of auto weakMap = ..., do

  Rooted<ObjectWeakMap> weakMap(cx, cx);
  CHECK(weakMap.get().init());
  ...
  CHECK(weakMap.get().add(cx, key, value));
  ...

with the minor problem that weakmaps only expect to be destroyed by being swept during a GC, so you'll get an assertion failure. :(

Though that raises the question of why your version didn't explode when the UniquePtr destroyed the OWM. And that's because the GCManagedDeletePolicy is very clever; it transfers ownership to a sweepActions_ queue to be processed during the next minor GC. I didn't know about that; must be some terrence magic that you reviewed. Sadly, I see no way to apply it here, since we really are stack-allocating and thus stack-destroying the OWM here.
Attachment #8838627 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #1)
> There is a *different* way of doing this. I'm not sure if "better" is the
> right adjective.

Thanks for the suggestion.  I think this is better, even if it feels slightly strange.  I want to get rid of CustomAutoRooter anyway.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd05e24fefff
Add more test code for weak map gray marking behaviour r=sfink
https://hg.mozilla.org/mozilla-central/rev/cd05e24fefff
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.