Closed Bug 1391787 Opened 6 years ago Closed 6 years ago

stylo: various crashes in gtk3 [@ _gtk_css_value_compute]


(Core :: CSS Parsing and Computation, defect)

Not set



Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed


(Reporter: truber, Assigned: emilio)


(Blocks 2 open bugs)


(Keywords: crash, csectype-undefined, testcase)


(9 files)

Attached file 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.
Attached file segv-wild.txt
in m-c rev 20170817-a6a1f5c1d971
Attached file gtk-err.txt
in m-c rev 20170817-a6a1f5c1d971
Attached file jmp-null.txt
in m-c rev 20170817-a6a1f5c1d971
Attached file jmp-wild.txt
in m-c rev 20170817-a6a1f5c1d971
Attached file double-free.txt
in m-c rev 20170817-a6a1f5c1d971
Attached file segv-null.txt
in m-c rev 20170817-a6a1f5c1d971
Attached file heap-uaf.txt
in m-i rev 20170812-113580d267ea
Yeah, sounds like GTK may be doing something non-threadsafe there, and stylo is just triggering it... :(
Depends on: 1354966
Btw my GTK version is: 3.22.18
FWIW, bug 1386915 also involved nsLookAndFeel::NativeGetColor.
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 :(
Flags: needinfo?(manishearth)
Attached patch Patch.Splinter Review
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.
Attachment #8899244 - Flags: review?(manishearth)
Attachment #8899244 - Flags: review?(manishearth) → review+
remote: added 1 changesets with 1 changes to 1 files
remote: recorded push in pushlog
remote: View your change here:
remote: Follow the progress of your build on Treeherder:
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?
Flags: needinfo?(cam)
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.
Flags: needinfo?(cam)
Assignee: nobody → emilio+bugs
Closed: 6 years ago
Resolution: --- → FIXED
Flags: needinfo?(manishearth)
Group: dom-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.