Closed Bug 1381389 Opened 2 years ago Closed 2 years ago

stylo: Crash in nsCSSValue::nsCSSValue

Categories

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

Unspecified
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: cpeterson, Assigned: birtles)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-f32fcbef-4fe5-4cff-86ed-2bdc50170716.
=============================================================
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.
The crash I mentioned doesn't seem related to that. I'll look into that next.
Assignee: nobody → canaltinova
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.
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).
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.
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?
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?
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
Stealing this since it seems straightforward.
Assignee: canaltinova → bbirtles
Status: NEW → ASSIGNED
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 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.
Attachment #8888150 - Flags: review?(hikezoe) → review+
Attached file Servo PR #17794
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d506550be89
Append PropertyValuePair objects on Gecko side so they are initialized correctly; r=hiro
https://hg.mozilla.org/mozilla-central/rev/6d506550be89
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.