Note: There are a few cases of duplicates in user autocompletion which are being worked on.

stylo: Crash in nsCSSValue::nsCSSValue

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P2
critical
RESOLVED FIXED
5 days ago
7 hours ago

People

(Reporter: cpeterson, Assigned: birtles)

Tracking

(Blocks: 1 bug, {crash})

unspecified
mozilla56
Unspecified
Windows 10
crash
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

5 days ago
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.
(Assignee)

Comment 4

3 days ago
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.
(Assignee)

Comment 6

3 days ago
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?
(Assignee)

Comment 7

3 days ago
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?
(Assignee)

Comment 8

3 days ago
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
(Assignee)

Comment 9

3 days ago
Stealing this since it seems straightforward.
Assignee: canaltinova → bbirtles
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 days ago
mozreview-review
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

2 days ago
mozreview-review
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+
(Assignee)

Comment 13

2 days ago
Created attachment 8888164 [details] [review]
Servo PR #17794
Comment hidden (mozreview-request)

Comment 15

2 days ago
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
Last Resolved: 7 hours ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.