1381389 - stylo: Crash in nsCSSValue::nsCSSValue

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

This bug was filed from the Socorro interface and is 
report bp-f32fcbef-4fe5-4cff-86ed-2bdc50170716.
=============================================================

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

eCSSUnit_GridTemplateAreas is set to nsCSSValue::mUnit, but mValue.mGridTemplateAreas is null.

canaltinova may have some ideas. IIRC, he mentioned that there were still some crashes in our grid mochitests, so fixes to them may fix this as well.

Comment 2

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

The crash I mentioned doesn't seem related to that. I'll look into that next.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Some of reports have PresShell::HandleEvent(), so I guess they are the same cause of bug 1381431 (or bug 1371450)

But this one bp-f87121ee-1c68-4ca7-9c9e-1caaf0170712 doesn't have HandleEvent().
Anyway it's related to animation.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

The thing that worries me about this is that 'grid-template-areas' is not yet animatable with Stylo (bug 1379922), yet this crash appears to happen when we clone the array of keyframes (mKeyframes) suggesting we're somehow getting grid-template-areas properties in mKeyframes despite it not being animatable. Either that, or we're setting the mUnit member incorrectly (or this block of memory is just completely bogus).

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Yeah, initially I thought the crash report was sent by daisuke, actually I was about to leave such comment, but it's not Mac.

(In reply to Brian Birtles (:birtles) from comment #4)

> we're setting the mUnit member incorrectly (or
> this block of memory is just completely bogus).

I guess so.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

This seems to mostly happen when building CSS animations using Stylo and we end up having trouble with grid-template-areas:

  https://crash-stats.mozilla.com/report/index/f32fcbef-4fe5-4cff-86ed-2bdc50170716
  https://crash-stats.mozilla.com/report/index/e9109e53-960c-4cab-9de8-3262f0170716
  https://crash-stats.mozilla.com/report/index/48dfd3e5-5743-4de6-a794-25f090170716
  https://crash-stats.mozilla.com/report/index/65591043-bfd0-4baf-8283-821430170716

In one case, however, with a similar stack we ran into trouble with a string value:

  https://hg.mozilla.org/mozilla-central/annotate/09a4282d1172/layout/style/nsCSSValue.cpp#l150

Curiously, we can hit the same problem line with grid-template-areas in non-animation code on non-Stylo builds:

  https://crash-stats.mozilla.com/report/index/013cfe1e-d090-4f3f-ba85-4c1410170718
  https://crash-stats.mozilla.com/report/index/55ed57ee-164e-4450-8110-f266f0170712

So perhaps this is just a common place to land when memory is corrupt?

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Come to think of it, I guess we shouldn't be setting PropertyValuePair.mValue at all when using Stylo. Perhaps we are overwriting that memory when we set mProperty? Or we're doing something else odd when allocating that mPropertyValues array?

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Oh, I didn't realize that set_len only extends the length without initializing the space![1] That would explain the problem. I suppose we need to zero it out or somehow ensure that mValue doesn't end up with random junk.

[1] http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/servo/components/style/gecko_bindings/sugar/ns_t_array.rs#89

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Stealing this since it seems straightforward.

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1381389 - Append PropertyValuePair objects on Gecko side so they are initialized correctly;

Without this mValue may end up looking like a valid nsCSSValue object and bad
things will happen when we try to clone it.

We could just assign mem:zeroed() to mValue but this array hacking on the Servo
side is already pretty nasty and mSimulateComputeValuesFailure would still
remain unassigned (and if we did try to assign it on the Servo side we'd need to
only assign it in debug builds). Unless this proves performance-critical, it's
probably best to just do this on the Gecko side.

Review commit: https://reviewboard.mozilla.org/r/159066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159066/

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8888150 [details]
Bug 1381389 - Append PropertyValuePair objects on Gecko side so they are initialized correctly;

https://reviewboard.mozilla.org/r/159066/#review164454

::: layout/style/ServoBindings.cpp:1900
(Diff revision 1)
> +  PropertyValuePair* result =
> +    aProperties->AppendElement(PropertyValuePair {aProperty});
> +  return result;

Oops, this should just be:

  return aProperties->AppendElement(...);

(Originally I set mProperty on |result| as a separate step but I hard trouble persuading hazard analysis that this was ok.)

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8888150 [details]
Bug 1381389 - Append PropertyValuePair objects on Gecko side so they are initialized correctly;

https://reviewboard.mozilla.org/r/159066/#review164456

Thanks for fixing this! This will emliminate weird crash reports!

::: dom/animation/Keyframe.h:28
(Diff revision 1)
>  /**
>   * A property-value pair specified on a keyframe.
>   */
>  struct PropertyValuePair
>  {
> +  PropertyValuePair(nsCSSPropertyID aProperty)

Nit: explicit.

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Servo PR #17794

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8888150 [details]
Bug 1381389 - Append PropertyValuePair objects on Gecko side so they are initialized correctly;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159066/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d506550be89
Append PropertyValuePair objects on Gecko side so they are initialized correctly; r=hiro

Comment 16

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

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