1406220 - stylo: Add canaries and journaling to HashMaps

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P4)

Description

[Bobby Holley (:bholley)]

In our ongoing quest to figure out what the heck is going on with HashMaps, I wrote up some more aggressive diagnostics to land on nightly.

First, we insert a canary between each key and value in the map. After insertions, we iterate the map, verifying all the canaries. If we ever find a bad canary, we panic.

Second, we keep a log of all the hashes inserted/removed, and include this log in the crash report. If we assume there are no collisions of full hashes (which is possible, but much less likely than collisions of hashes masked down to the table size), this will allow us to replay the conditions that led to the crash, which will hopefully allow is to reproduce it.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Diagnostic map semantics. v1

MozReview-Commit-ID: C0a5g6xMPY0

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add canary and journaling. v1

MozReview-Commit-ID: 582ZiTmcvgs

Comment 3

[Nicholas Nethercote [inactive]]

How big is the log?

Comment 4

[Bobby Holley (:bholley)]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> How big is the log?

2 words per hashmap entry. remove() and overwriting insert() calls could technically push us over the size of the hashmap, but that seems unlikely.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa1b93d1cd3635f0d9e0414d8c6ee91ffcdd0843

Comment 5

[(Away)]

Comment on attachment 8915789 [details] [diff] [review]
Part 2 - Add canary and journaling. v1

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

::: layout/style/ServoBindings.cpp
@@ +24,5 @@
>  #include "nsContentUtils.h"
>  #include "nsDOMTokenList.h"
>  #include "nsDeviceContext.h"
>  #include "nsIContentInlines.h"
> +#include "nsICrashReporter.h"

This probably needs an ifdef MOZ_CRASHREPORTER.

Comment 6

[Nicholas Nethercote [inactive]]

My question was unclear. How much will this increase the size of crash reports?

Comment 7

[Bobby Holley (:bholley)]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> My question was unclear. How much will this increase the size of crash
> reports?

The only time we append the data is when we detect corruption via the canary, so it should only increase the size of a handful of crash reports.

Comment 8

[Bobby Holley (:bholley)]

https://github.com/servo/servo/pull/18779

Comment 9

[Bobby Holley (:bholley)]

the gecko-side changes are trivial and not interdependent, so I'm just landing them now instead of doing the coordiland dance:

https://hg.mozilla.org/integration/autoland/rev/7b16f542139d0d12722cb8dd51568ddedf81bf1e

Comment 10

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/autoland/rev/4a7092139098

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b16f542139d
https://hg.mozilla.org/mozilla-central/rev/7d02772fb885