1407080 - stylo: more HashMap diagnostics

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

Here's another round that came out of the wishlist from today's investigation.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Stuff HashMap journal into a string and verify invariants in a few more places. v1

The RegisterAppMemory approach isn't working for some reason.

MozReview-Commit-ID: GjGaq6GALI5

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[(Away)]

Comment on attachment 8916829 [details] [diff] [review]
Stuff HashMap journal into a string and verify invariants in a few more places. v1

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

::: servo/components/hashglobe/src/diagnostic.rs
@@ +152,5 @@
>          position: usize
>      ) {
> +        use ::std::ffi::CString;
> +        let key = CString::new("HashMapJournal").unwrap();
> +        let value = CString::new(format!("{:?}", self.journal)).unwrap();

Historically we've avoided allocating in a MOZ_CRASH-like path because we can't trust our state, especially when corruption is involved. `key` here should probably be a static thing (do you have an nsLiteralString equivalent?) and as for `value`, maybe consider printing into a static buffer?

Maybe this is pointless because we already have tons of code using `panic!` which is going to printf-allocate anyway, whether I like it or not.

@@ +157,2 @@
>          unsafe {
> +            Gecko_AnnotateCrashReport(

ted and/or njn should sign off on this use of annotations. I vaguely recall some restrictions on not stuffing too much data into Socorro but my knowledge in this area is old.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8916829 [details] [diff] [review]
Stuff HashMap journal into a string and verify invariants in a few more places. v1

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

::: layout/style/ServoBindings.cpp
@@ +2822,5 @@
>  }
> +
> +void Gecko_AnnotateCrashReport(const char* key_str, const char* value_str)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());

Just curious... There's nothing that guarantees this, actually, right? Is it just to silence the heap write analysis?

We have crash reporting for all DiagnosticHashMap, which can get dropped off-main-thread (the ones for custom props at least). I changed this today to not have temporary OrderedMaps, so I believe in that case should hold as long as style contexts are 100% always dropped on the main thread.

I wouldn't be so sure this holds for all the objects that may contain a hash map without that assertion in Servo_ComputedValues_Drop...

::: servo/components/hashglobe/src/diagnostic.rs
@@ +152,5 @@
>          position: usize
>      ) {
> +        use ::std::ffi::CString;
> +        let key = CString::new("HashMapJournal").unwrap();
> +        let value = CString::new(format!("{:?}", self.journal)).unwrap();

b"HashMapJournal\0" should work, but yeah, the panic allocations can be hard to avoid...

Comment 5

[(not currently active) Ted Mielczarek]

(In reply to David Major [:dmajor] from comment #3)
> Historically we've avoided allocating in a MOZ_CRASH-like path because we
> can't trust our state, especially when corruption is involved. `key` here
> should probably be a static thing (do you have an nsLiteralString
> equivalent?) and as for `value`, maybe consider printing into a static
> buffer?
> 
> Maybe this is pointless because we already have tons of code using `panic!`
> which is going to printf-allocate anyway, whether I like it or not.

It would be nice to not allocate here since we think we have memory corruption, but we're not in an exception handler or anything when this code runs, so I don't think it will be a real issue (and as you point out panic! is going to allocate while formatting anyway).

> ted and/or njn should sign off on this use of annotations. I vaguely recall
> some restrictions on not stuffing too much data into Socorro but my
> knowledge in this area is old.

I asked, and it doesn't look like Socorro has specific limits on the size of crash annotations. Since we're only adding this in a very specific case I think it should be fine.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> Comment on attachment 8916829 [details] [diff] [review]
> Stuff HashMap journal into a string and verify invariants in a few more
> places. v1
> 
> Review of attachment 8916829 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/ServoBindings.cpp
> @@ +2822,5 @@
> >  }
> > +
> > +void Gecko_AnnotateCrashReport(const char* key_str, const char* value_str)
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> 
> Just curious... There's nothing that guarantees this, actually, right? Is it
> just to silence the heap write analysis?

That's right. With e10s enabled, the crash handler is actually threadsafe, but this whole situation is going to be rare enough (a few times a day over our entire nightly population) that I'm not worried about races.

> ::: servo/components/hashglobe/src/diagnostic.rs
> @@ +152,5 @@
> >          position: usize
> >      ) {
> > +        use ::std::ffi::CString;
> > +        let key = CString::new("HashMapJournal").unwrap();
> > +        let value = CString::new(format!("{:?}", self.journal)).unwrap();
> 
> b"HashMapJournal\0" should work, but yeah, the panic allocations can be hard
> to avoid...

Will do. And yeah, I'm not going to worry about allocations during stringification.

Comment 7

[Bobby Holley (:bholley)]

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

Comment 8

[Bobby Holley (:bholley)]

gecko-side: https://hg.mozilla.org/integration/autoland/rev/6a17fec90855

Comment 9

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (parental leave - send mail for anything urgent) from comment #8)
> gecko-side: https://hg.mozilla.org/integration/autoland/rev/6a17fec90855

https://hg.mozilla.org/integration/autoland/rev/a7a0a2f1c89d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a17fec90855

Comment 11

[(not currently active) Ted Mielczarek]

(In reply to Bobby Holley (parental leave - send mail for anything urgent) from comment #1)
> The RegisterAppMemory approach isn't working for some reason.

I think it doesn't work in child processes. I filed bug 1408473.

Comment 12

[Bobby Holley (:bholley)]

Meant to post this a few days ago, it mid-aired and I didn't notice until now.

> I tacked on some diagnostics as
>
> https://github.com/servo/servo/pull/18829
> https://hg.mozilla.org/integration/autoland/rev/28a041a33fec