1403397 - stylo: Land Nightly diagnostic code to mprotect stylist hashtables outside of rebuilds in order to determine what's corrupting them

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

This is the approach discussed in bug 1385925 comment 26. The hope is that this will convert those crash reports into new reports whose callstack indicates the source of corruption.

My basic plan is as follows:
(1) Introduce a HashMap wrapper that must be explicitly unlocked before mutation, and add lock/unlock calls around stylist rebuilds. 
(2) Round all hashglobe allocations up to the nearest page. This should regress memory somewhat, but it should be acceptable for a few days of diagnostics on Nightly 58.
(3) In {begin,end}_mutation, do an FFI call with the buffer to invoke MemoryProtectionExceptionHandler::{add,remove}Region and {MakePagesReadOnly,UnprotectPages}.

If anyone has thoughts/suggestions here, please speak up.

This try push is for step (1):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d46d80220c328017d60d4d3df269d35e8d39a95e

Comment 1

[Emilio Cobos Álvarez (:emilio)]

So, there were a bunch of stacks in that bug that doesn't necessarily involve HashMap.

Is the idea here just that HashMaps are just a bit more likely to hit this because they use a ton of contiguous memory?

Comment 2

[Emanuel Hoogeveen [:ehoogeveen]]

I'd be really happy if MemoryProtectionExceptionHandler helped track down an issue like this :)

One implementation detail of the class that might be worth keeping an eye on is its use of js::SplayTree [1]. IIUC splay trees work best when a small subset of elements are accessed frequently, or if it's used as a LIFO buffer, and I don't know if that's likely for this. If it ends up acting like a linked list and insertion or removal start showing up in profiles, it might be worth switching it over to another data structure.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/ds/SplayTree.h

Comment 3

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> So, there were a bunch of stacks in that bug that doesn't necessarily
> involve HashMap.
> 
> Is the idea here just that HashMaps are just a bit more likely to hit this
> because they use a ton of contiguous memory?

Potentially, or they're distinct crashes. Either way these diagnostics are intended to diagnose the hashmap crashes described in bug 1385925 comment 12.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #2)
> I'd be really happy if MemoryProtectionExceptionHandler helped track down an
> issue like this :)
> 
> One implementation detail of the class that might be worth keeping an eye on
> is its use of js::SplayTree [1]. IIUC splay trees work best when a small
> subset of elements are accessed frequently, or if it's used as a LIFO
> buffer, and I don't know if that's likely for this. If it ends up acting
> like a linked list and insertion or removal start showing up in profiles, it
> might be worth switching it over to another data structure.
> 
> [1] https://dxr.mozilla.org/mozilla-central/source/js/src/ds/SplayTree.h

The plan is to only leave this code active for a few days, so as long as the performance isn't terrible it's probably ok. Will be on the lookout though.

Comment 5

[Bobby Holley (:bholley)]

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

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 1 - Semantics for ProtectedHashMap. v1

MozReview-Commit-ID: K0m65uZi7iw

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 2 - Round hashglobe allocations up to the nearest page size. v1

MozReview-Commit-ID: 34KFtcwCkBB

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 3 - Protect the hashmaps outside of rebuilds. v1

MozReview-Commit-ID: KACmfw4pZY2

Comment 9

[Bobby Holley (:bholley)]

Did some local measurements. Memory impact is surprisingly small, which I guess indicates that we don't have lots and lots of barely-populated hashmaps. There's some performance overhead during stylist rebuilds, but it should be acceptable for a few days of diagnostics on Nightly.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 3 - Protect the hashmaps outside of rebuilds. v2

Updated to handle asan.

MozReview-Commit-ID: KACmfw4pZY2

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 4 - Add a testing API. v1

This will allow us to verify the entire detection pipeline in real nightly
builds, which will give us confidence that real heap corruption will be
detected and reported properly.

MozReview-Commit-ID: 43Fp2HT8RYy

Comment 12

[Bobby Holley (:bholley)]

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

Comment 13

[Bobby Holley (:bholley)]

And to be clear: The plan is to land these patches on nightly, and back them out a few days later once we have sufficient data from crash-stats.

Comment 14

[(Away)]

Comment on attachment 8913017 [details] [diff] [review]
Part 2 - Round hashglobe allocations up to the nearest page size. v1

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

::: layout/style/ServoBindings.cpp
@@ +2634,5 @@
>  
> +size_t
> +Gecko_GetSystemPageSize()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());

This operation is probably safe on any thread, but I guess there's no harm in being more restrictive.

::: servo/components/hashglobe/src/table.rs
@@ +1212,5 @@
> +    let remainder = size % page_size;
> +    if remainder != 0 {
> +        result += page_size - remainder;
> +    }
> +    debug_assert!(result % page_size == 0);

In case this function survives past your backout and the math above gets refactored, could you please add:
debug_assert!(result - size < page_size);
We've had some off-by-ones in the past where e.g. 4096 got rounded up to 8192. :-/

Comment 15

[(Away)]

Comment on attachment 8913071 [details] [diff] [review]
Part 3 - Protect the hashmaps outside of rebuilds. v2

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

I assume this handles cases of the buffer changing on resize? (I'm not familiar with the hashmap code)

Comment 16

[(Away)]

I want to test this out by introducing a bad write in my debugger and checking that the report shows up correctly in crash-stats. (Don't block on me, and clear my flag if you've already done this)

Comment 17

[(Away)]

Looks good:
MOZ_CRASH Reason 	MOZ_CRASH(Tried to access a protected region!)

Comment 18

[Bobby Holley (:bholley)]

(In reply to David Major [:dmajor] from comment #17)
> Looks good:
> MOZ_CRASH Reason 	MOZ_CRASH(Tried to access a protected region!)

I assume this was you? https://crash-stats.mozilla.com/report/index/d1354951-339d-4be4-b725-819da0170928

Note that I was also planning to land the diagnostics in Part 4, which would allow us to trigger a crash with a known stack from a privileged scratchpad in a nightly build.

Comment 19

[Manish Goregaokar [:manishearth]]

Comment on attachment 8913017 [details] [diff] [review]
Part 2 - Round hashglobe allocations up to the nearest page size. v1

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

::: servo/components/hashglobe/src/table.rs
@@ +776,5 @@
>  
>  
>  
>          // FORK NOTE: Uses alloc shim instead of Heap.alloc
> +        let buffer = alloc(round_up_to_page_size(size), alignment);

kinda feel like we could round up cap as well but not necessary since this is just an experiment

Comment 20

[Manish Goregaokar [:manishearth]]

Comment on attachment 8913071 [details] [diff] [review]
Part 3 - Protect the hashmaps outside of rebuilds. v2

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

::: servo/components/hashglobe/src/protected.rs
@@ +169,5 @@
>  {
>      pub fn new() -> Self {
>          Self {
>              map: HashMap::new(),
>              readonly: true,

it's a bit non-obvious why we don't protect() here too, might be worth mentioning in a comment, but no big deal if ignored since this is temporary

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8913072 [details] [diff] [review]
Part 4 - Add a testing API. v1

r=me

Comment 22

[Bobby Holley (:bholley)]

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

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/820174e66b18
Round hashglobe allocations up to the nearest page size. r=Manishearth,r=dmajor
https://hg.mozilla.org/integration/autoland/rev/17044f9cf2a3
Protect the hashmaps outside of rebuilds. r=Manishearth,r=dmajor,r=ehoogeveen
https://hg.mozilla.org/integration/autoland/rev/600792598d42
Add a testing API. r=bz,r=Manishearth

Comment 24

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #22)
> https://github.com/servo/servo/pull/18668

Landed as:

https://hg.mozilla.org/integration/autoland/rev/9793aaf42699
https://hg.mozilla.org/integration/autoland/rev/9d688ebb802b

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/820174e66b18
https://hg.mozilla.org/mozilla-central/rev/17044f9cf2a3
https://hg.mozilla.org/mozilla-central/rev/600792598d42

Comment 26

[Bobby Holley (:bholley)]

So, we got some mixed and inconclusive results here.

First, the mprotect stuff didn't turn up anything all that interesting. We got exactly one interesting crash when trying to write to the protected region: https://crash-stats.mozilla.com/report/index/39b2cd4a-7bed-40ec-8b63-598080171002 . That seems to indicate the JS heap was corrupted and GCing it is causing us to clobber memory elsewhere in the system. That's pretty bad, but I'm not sure if there's anything we can do about it. NI some GC people to make sure they're aware, but one instances of extern corruption doesn't account for the crash volume we were seeing.

Additionally, the crashes continued. The data is a bit hard to read, because the crash volume (in 58) decreased substantially a few weeks ago. Furthermore, the EXCEPTION_BREAKPOINT crashes (where we get killed by the parent process) are less explainable, so I've filtered those out as well in my query [1].

Anyway, there are definitely some HashMap crashes in the range where this stuff was landed.
* https://crash-stats.mozilla.com/report/index/15be700d-7b16-4880-9b1f-3da270171001
* https://crash-stats.mozilla.com/report/index/e8ab82ce-adc0-4f14-adf7-4fd5f0171003
* https://crash-stats.mozilla.com/report/index/7d56ffe7-006f-4a0a-94f9-ced150171003


Removing the diagnostics in https://github.com/servo/servo/pull/18732 . Gankro and I are doing some audits of HashMap.

[1] https://crash-stats.mozilla.com/signature/?version=58.0a1&reason=EXCEPTION_ACCESS_VIOLATION_WRITE&reason=EXCEPTION_ACCESS_VIOLATION_READ&signature=core%3A%3Aptr%3A%3Adrop_in_place%3CT%3E&date=%3E%3D2017-09-19T19%3A33%3A24.000Z&date=%3C2017-10-03T19%3A33%3A24.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf6cec51bf0d
Revert: Add a testing API. r=bholley
https://hg.mozilla.org/integration/autoland/rev/2a4030b05c35
Revert: Protect the hashmaps outside of rebuilds. r=bholley
https://hg.mozilla.org/integration/autoland/rev/93de74a2302a
Revert: Round hashglobe allocations up to the nearest page size. r=bholley

Comment 28

[Paul Bone [:pbone]]

Thanks, I have filed bug 1405525 for the GC team.

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/93de74a2302a