1621773 - Crash in [@ selectors::parser::{{impl}}::to_shmem<T>]

Status: RESOLVED FIXED

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

Description

[Brian Grinstead [:bgrins]]

I'm spinning this out of Bug 1607716 so we can track this better, because of https://bugzilla.mozilla.org/show_bug.cgi?id=1607716#c52:

I think this bug has been tagged up with crashes that have a different root cause from the "outdated libxul and minimal-xul.css" problem we worked around here (as we are still seeing reports on 73/74 where the mozCollapsed atom was re-added).

For instance:

https://crash-stats.mozilla.org/report/index/243592b9-ddd5-467f-a8eb-919a60200311 - ToShmem failed for Atom: must be a static atom: reSet
https://crash-stats.mozilla.org/report/index/40908cd8-1931-4292-9a5f-ad8640200201 - ToShmem failed for Atom: must be a static atom: bmrder.

What are bmrder and reSet? These must be coming from users somehow, which is unexpected as per https://bugzilla.mozilla.org/show_bug.cgi?id=1607716#c16. Cam or Emilio, do you have any ideas what is going on from those crashes?

Emilio mentions in https://bugzilla.mozilla.org/show_bug.cgi?id=1607716#c53 that it's possible we are hitting a corrupted file on disk and we could fallback more gracefully in release builds.

Comment 1

[Brian Grinstead [:bgrins]]

Moving the needinfo from https://bugzilla.mozilla.org/show_bug.cgi?id=1607716#c52 over here

Comment 2

[Brian Grinstead [:bgrins]]

Here's another "off by one character" crash: https://crash-stats.mozilla.org/report/index/9320d196-dd24-407c-b465-2ab4f0200309 ToShmem failed for Atom: must be a static atom: morResizingInfo

Though this one is different: https://crash-stats.mozilla.org/report/index/ee41de6f-9b72-4842-a6ee-a66820200308 EXCEPTION_ACCESS_VIOLATION_EXEC

Comment 3

[Daniel Holbert [:dholbert]]

Observation: around a third of the last week's crashes seem to be reported for nonexistent ESR versions.

FACT: the current Firefox ESR release is Firefox 68.6.0esr, and based on https://wiki.mozilla.org/Release_Management/ESR_Landing_Process , I think the next ESR will be 78. And yet, a bunch of these crashes are reported with other bogus intermediate ESR versions. Some examples:

• 74.0esr : bp-8f44399e-3db5-4829-bdba-821df0200313
• 73.0esr bp-5f96b22a-4076-4fab-9ccb-011c10200319
  (both of those^ under the style::gecko_string_cache::{{impl}}::to_shmem signature, linked here
• 72.0.1esr: bp-e6d89686-5ee5-47f0-bc9a-03ff60200319
  (this one^ under the selectors::parser::{{impl}}::to_shmem<T> signature, linked here

Comment 4

[Daniel Holbert [:dholbert]]

(and this version-number-weirdness probably adds credence to the theory that there's some sort of local file corruption/manipulation/customization that's gone on here)

Comment 5

[Cameron McCormack (:heycam)]

OK, we can fall back more gracefully here by just not doing the shared memory style sheet thing when we detect we don't have the right static atoms available. And maybe we should add some telemetry to record how often it happens. But the single bit differences sounds like the kind of crash reports we get with single bit differences, and so might just point to bad RAM / disk.

Comment 6

[Daniel Holbert [:dholbert]]

A few other interesting MOZ_CRASH reason field values:

https://crash-stats.mozilla.org/report/index/b3c5e785-cded-44f5-a07b-591860200429
ToShmem failed for Atom: must be a static atom: ٠

This looks pretty bogus. ٠ doesn't seem like something that we'd have in a static atom...

https://crash-stats.mozilla.org/report/index/6300a87c-639a-418e-b192-f58f00200429
https://crash-stats.mozilla.org/report/index/3da23a97-7ea8-443e-a76d-8c6d30200429
ToShmem failed for Atom: must be a static atom: anonymous-div
https://crash-stats.mozilla.org/report/index/b3c5e785-cded-44f5-a07b-591860200429
https://crash-stats.mozilla.org/report/index/9aff04dc-8aec-44b2-81c0-a06460200429
https://crash-stats.mozilla.org/report/index/1ff218c6-413c-4088-a5dc-165e40200429
ToShmem failed for Atom: must be a static atom: moz-collapsed

These ones look like real strings (there's no weird character mismatching as there seems to be with e.g. bmder in comment 0). But having said that, searchfox doesn't turn up any usages of these strings in-tree, so I'm not sure where they'd come from...

Comment 7

[Cameron McCormack (:heycam)]

FWIW the reason I made this a panic initially was so that developers would realize when they needed to add new static atoms after editing UA style sheets. But we could make it panic only in debug builds or something.

(In reply to Daniel Holbert [:dholbert] from comment #6)

https://crash-stats.mozilla.org/report/index/6300a87c-639a-418e-b192-f58f00200429
https://crash-stats.mozilla.org/report/index/3da23a97-7ea8-443e-a76d-8c6d30200429
ToShmem failed for Atom: must be a static atom: anonymous-div

That's an atom that was removed in bug 1618260 last month, so feels like the same potential updater / corruption related issue as bug 1607716.

https://crash-stats.mozilla.org/report/index/b3c5e785-cded-44f5-a07b-591860200429
https://crash-stats.mozilla.org/report/index/9aff04dc-8aec-44b2-81c0-a06460200429
https://crash-stats.mozilla.org/report/index/1ff218c6-413c-4088-a5dc-165e40200429
ToShmem failed for Atom: must be a static atom: moz-collapsed

That was the specific one that prompted bug 1607716 and which we added back briefly.

Comment 8

[Cameron McCormack (:heycam)]

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

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Bug 1621773 - Gracefully handle errors creating shared memory UA style sheets.

We still panic in a debug build, so that developers can notice when they
need to add a new static atom after modifying UA sheets.

We also add telemetry to note when this happens, add an app note to a
crash report, in case any crash later on occurs, and re-up the existing,
expired shared memory sheet telemetry probes so we can look at them
again.

Comment 10

[Cameron McCormack (:heycam)]

Attachment: data collection renewal request

Comment 11

[Cameron McCormack (:heycam)]

Attachment: data review request

Comment 12

[Cameron McCormack (:heycam)]

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

Comment 13

[Chris H-C :chutten]

Comment on attachment 9144545 [details]
data collection renewal request

    Is the provided Data Collection Review complete, correct, and data-review+ by a Data Steward?

Yes.

    Is the data collection covered by the existing Firefox Privacy Notice?

Yes.

---
Result: datareview+

Comment 14

[Chris H-C :chutten]

Comment on attachment 9144547 [details]
data review request

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. This collection is Telemetry so is documented in its definitions file [Histograms.json](https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/telemetry/Histograms.json) and the [Probe Dictionary](https://telemetry.mozilla.org/probe-dictionary/).

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

No. This collection will expire in Firefox 83.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

    Is the data collection request for default-on or default-off?

Default on for all channels.

    Does the instrumentation include the addition of any new identifiers?

No.

    Is the data collection covered by the existing Firefox privacy notice?

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

Yes. :heycam is responsible for renewing or removing the collection before it expires in Firefox 83.

---
Result: datareview+

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6cb8c4385a39
Gracefully handle errors creating shared memory UA style sheets. r=emilio

Comment 16

[Cristian Brindusan [:cbrindusan]]

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

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:heycam, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 19

[Cameron McCormack (:heycam)]

I don't think I would bother uplifting this.