Closed Bug 1509923 Opened 11 months ago Closed 10 months ago

Add a zeal mode to check weak map marking state

Categories

(Core :: JavaScript: GC, enhancement)

61 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: jonco, Assigned: jonco, NeedInfo)

References

Details

Attachments

(1 file)

I'm still getting crashes when enabling incremental gray marking.  To help track them down, I'm adding a zeal mode to check the weak map marking state is correct.
Patch to add a zeal mode to check weak map marking.  This happens when we finish marking a sweep group.

One thing I'm not sure about is the provision for debugger weakmaps.  We do run into the case where one of these has a key in another compartment that ends up unmarked even though its delegate is marked.  This seems suspicious, but I'm not sure why it's happening (or what to do about it).

Also, maybe this should run all the time in debug builds.  What do you think?

This is green on try when enabled.
Attachment #9027846 - Flags: review?(sphink)
Comment on attachment 9027846 [details] [diff] [review]
bug1509923-weak-map-marking-zeal

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

Wow, this is really awesome. Why didn't I do this, again?

I think this makes sense to be on all the time in DEBUG. Unless it's expensive, but it seems like it shouldn't be -- it just scans through all entries of live weakmaps, which ought to almost always be way faster than scanning the entire heap.

I guess before r+, I want to be convinced of the need to early-exit if the key's compartment is not being collected.

::: js/src/gc/Verifier.cpp
@@ +864,5 @@
> +    // no longer collecting. We can't make guarantees about the mark
> +    // state of these keys.
> +    if (map->allowKeysInOtherZones() && !(keyZone->isGCMarking() || keyZone->isGCSweeping())) {
> +        return true;
> +    }

Debugger weakmap keys are always in another compartment, I believe. Debugger maps debuggee objects to their Debugger.Object wrappers (for instance) which will be in the same compartment as the weakmap. (Well, the weakmaps are not owned by/memberOf objects, so they're not exactly "in" a compartment, but I consider them to be in the Debugger object's compartment.)

If the other compartment (probably a debuggee compartment, or at least it was once) is not being marked, then I would expect those keys to be treated as if they were marked. As for their delegates... hm, I guess that's the case where we're debugging a compartment that has a CCW to yet another compartment. In that case, I guess I would think that the delegate would be regarded as marked, which means the key should be marked if the map is alive.

I guess I'm wondering why we need to early-exit in this case (map->allowKeysInOtherZones() && !(keyZone->isGCMarking() || keyZone->isGCSweeping())). Shouldn't this just make keyColor = CellColor::Black? Oh wait, the color is a little.

Ok, let me map this out. Say we have 3 compartments: DC (Debugger compartment), KC (key compartment), and OC (delegate object compartment.) DC and KC are being marked, but OC is not. Then we should assume everything in OC is black. Let's say KC is not marked for any other reason. Then because the map is live, the delegate's black should propagate to the weakmap entry, which needs its key's identity to persist so it marks it (black). That then propagates to the entry's value in the usual way.

Shouldn't this then work if keyColor is set to GetCellColor(key) if KC is marking/sweeping, otherwise black? And without the early exit?
Attachment #9027846 - Flags: review?(sphink)
Comment on attachment 9027846 [details] [diff] [review]
bug1509923-weak-map-marking-zeal

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

It sounds like it will require investigation to get the tighter assertion working, so we'd like to land as-is.
Attachment #9027846 - Flags: review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98f8e4e44c10
Check weak map marking state in debug builds and when enabled with a zeal mode r=sfink
Backed out changeset 98f8e4e44c10 (Bug 1509923) for Verifier.cpp failures on Linux builds.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=98f8e4e44c103044d3a6d9a27bd2e8586fe4f05e&selectedJob=216065734

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/387f770bf58c175b69b5803b3c0d4a7ea55d18c4

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=216068007&repo=mozilla-inbound&lineNumber=7388

[task 2018-12-08T20:53:06.773Z] Found 1 hazards and 36 unsafe references
[task 2018-12-08T20:53:06.774Z] Running heapwrites to generate heapWriteHazards.txt
[task 2018-12-08T20:53:06.774Z] PATH="/builds/worker/workspace/sixgill/usr/bin:${PATH}" SOURCE='/builds/worker/checkouts/gecko' ANALYZED_OBJDIR='/builds/worker/workspace/obj-analyzed' LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:/builds/worker/workspace/obj-haz-shell/dist/bin" XDB='/builds/worker/workspace/sixgill/usr/bin/xdb.so' /builds/worker/workspace/obj-haz-shell/dist/bin/js /builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/analyzeHeapWrites.js > heapWriteHazards.txt
[task 2018-12-08T20:53:08.954Z] + check_hazards /builds/worker/workspace/analysis
[task 2018-12-08T20:53:08.954Z] + set +e
[task 2018-12-08T20:53:08.954Z] ++ grep -c 'Function.*has unrooted.*live across GC call' /builds/worker/workspace/analysis/rootingHazards.txt
[task 2018-12-08T20:53:08.955Z] + NUM_HAZARDS=1
[task 2018-12-08T20:53:08.956Z] ++ grep -c '^Function.*takes unsafe address of unrooted' /builds/worker/workspace/analysis/refs.txt
[task 2018-12-08T20:53:08.957Z] + NUM_UNSAFE=36
[task 2018-12-08T20:53:08.957Z] ++ grep -c '^Function.* has unnecessary root' /builds/worker/workspace/analysis/unnecessary.txt
[task 2018-12-08T20:53:08.959Z] + NUM_UNNECESSARY=1015
[task 2018-12-08T20:53:08.959Z] ++ grep -c '^Dropped CFG' /builds/worker/workspace/analysis/build_xgill.log
[task 2018-12-08T20:53:08.961Z] + NUM_DROPPED=0
[task 2018-12-08T20:53:08.961Z] ++ perl -lne 'print $1 if m!found (\d+)/\d+ allowed errors!' /builds/worker/workspace/analysis/heapWriteHazards.txt
[task 2018-12-08T20:53:08.963Z] + NUM_WRITE_HAZARDS=0
[task 2018-12-08T20:53:08.963Z] + set +x
[task 2018-12-08T20:53:08.963Z] TinderboxPrint: rooting hazards<br/>1
[task 2018-12-08T20:53:08.963Z] TinderboxPrint: (unsafe references to unrooted GC pointers)<br/>36
[task 2018-12-08T20:53:08.963Z] TinderboxPrint: (unnecessary roots)<br/>1015
[task 2018-12-08T20:53:08.963Z] TinderboxPrint: heap write hazards<br/>0
[task 2018-12-08T20:53:08.966Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'key' of type 'js::gc::Cell*' live across GC call at js/src/gc/Verifier.cpp:802
[task 2018-12-08T20:53:08.966Z] TEST-UNEXPECTED-FAIL | hazards | 1 rooting hazards detected
[task 2018-12-08T20:53:08.966Z] TinderboxPrint: documentation<br/><a href='https://wiki.mozilla.org/Javascript:Hazard_Builds#Diagnosing_a_rooting_hazards_failure'>static rooting hazard analysis failures</a>, visit "Inspect Task" link for hazard details
[task 2018-12-08T20:53:08.966Z] + onexit
Flags: needinfo?(jcoppeard)
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca4f360d2517
Check weak map marking state in debug builds and when enabled with a zeal mode r=sfink
https://hg.mozilla.org/mozilla-central/rev/ca4f360d2517
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
This broke fuzzing because the jsshell doesn't build anymore with --disable-debug --enable-gczeal now.

In file included from js/src/gc/Unified_cpp_js_src_gc2.cpp:2:
js/src/gc/Verifier.cpp:799:16: error: member reference type 'DebugOnly<JS::Zone *>' is not a pointer; did you mean to use '.'?
      !(keyZone->isGCMarking() || keyZone->isGCSweeping())) {
        ~~~~~~~^~
               .
js/src/gc/Verifier.cpp:799:18: error: no member named 'isGCMarking' in 'mozilla::DebugOnly<JS::Zone *>'
      !(keyZone->isGCMarking() || keyZone->isGCSweeping())) {
        ~~~~~~~  ^

and so on. This build flag combination is one of the most common to break that we don't have in CI, we should change that.


jonco, can you please land an urgent fix for this directly on m-c or backout this patch so we can resume fuzzing? Thanks.
Depends on: 1513433
(In reply to Christian Holler (:decoder) from comment #8)
> This broke fuzzing because the jsshell doesn't build anymore with
> --disable-debug --enable-gczeal now.
> 
> This build flag combination is one of the most common to break
> that we don't have in CI, we should change that.

Done in bug 1514346.
Depends on: 1513991
Regressions: 1542387
You need to log in before you can comment on or make changes to this bug.