1391787 - stylo: various crashes in gtk3 [@ _gtk_css_value_compute]

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase causes various gtk3 crashes when stylo is enabled by pref.

I'll attach logs for the various signatures I've been able to capture so far.
* pc = 0
* pc = wild
* segv reading wild ptr
* segv reading 0
* gtk abort
* double-free (infrequent)
* heap-use-after-free (once)

I also saw double-free infrequently and heap-use-after-free once during reduction. The segv on wild ptr and gtk abort were the most common.

This was hard to reduce, so it's not very concise and may still be intermittent.

Comment 1

[Jesse Schwartzentruber (:truber)]

Attachment: segv-wild.txt

in m-c rev 20170817-a6a1f5c1d971

Comment 2

[Jesse Schwartzentruber (:truber)]

Attachment: gtk-err.txt

in m-c rev 20170817-a6a1f5c1d971

Comment 3

[Jesse Schwartzentruber (:truber)]

Attachment: jmp-null.txt

in m-c rev 20170817-a6a1f5c1d971

Comment 4

[Jesse Schwartzentruber (:truber)]

Attachment: jmp-wild.txt

in m-c rev 20170817-a6a1f5c1d971

Comment 5

[Jesse Schwartzentruber (:truber)]

Attachment: double-free.txt

in m-c rev 20170817-a6a1f5c1d971

Comment 6

[Jesse Schwartzentruber (:truber)]

Attachment: segv-null.txt

in m-c rev 20170817-a6a1f5c1d971

Comment 7

[Jesse Schwartzentruber (:truber)]

Attachment: heap-uaf.txt

in m-i rev 20170812-113580d267ea

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Yeah, sounds like GTK may be doing something non-threadsafe there, and stylo is just triggering it... :(

Comment 9

[Jesse Schwartzentruber (:truber)]

Btw my GTK version is: 3.22.18

Comment 10

[Andrew McCreight [:mccr8]]

FWIW, bug 1386915 also involved nsLookAndFeel::NativeGetColor.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Just checked gtk's source, and there's lots of non thread-safe stuff going on, from non-atomic refcounts, to global caches. I think I'd just stash a mutex there, not much else we can do :(

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Patch.

This does the conservative thing, and fixes the crash for me as expected.

I doubt this will affect performance noticeably, since system colors are uncommon in content pages at least, and the chances on racing computing them are unlikely.

We can do more fine-grained stuff if we need to I suppose.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

remote: added 1 changesets with 1 changes to 1 files
remote: recorded push in pushlog
remote: 
remote: View your change here:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/de22ecdc7ec67f2e6e4186d9f390a73f93e28a53
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=de22ecdc7ec67f2e6e4186d9f390a73f93e28a53

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Cam, do you know what's the best way to add a test-case for this? It continually reloads the page, so it'll timeout in normal builds, do we have something like that?

Maybe stashing it in an iframe and reloading it a few times?

Comment 15

[Cameron McCormack (:heycam)]

If it would normally reproduce after a few reloads, then yeah, reloading an iframe a few times should be OK.  Although if it did start failing it would do so intermittently, which wouldn't be great I guess.  If it's not easy to reproduce the problem then feel free not to add a test.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de22ecdc7ec6