Closed Bug 1077746 Opened 5 years ago Closed 5 years ago

crash in nsCSSValue::nsCSSValue(nsCSSValue const&) on shutdown with certain sites if @font-face specified in userContent.css or MathML-fonts installed

Categories

(Core :: Layout, defect, critical)

35 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- unaffected
firefox35 + fixed

People

(Reporter: alice0775, Assigned: jtd)

References

Details

(Keywords: crash, reproducible)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-b363b29b-1d7e-45fe-b853-d43982141003.
=============================================================

Steps To Reproduce:
1. Update some of add-ons and restart

Actual Results:
Browser crashes w/ this crash sig.
[Tracking Requested - why for this release]: regression since Nightly35.0a1

There is no need to add-on to reproduce crash. 
Just need userContent.css and certain sites.

Steps To Reproduce:
1. Create New Profile
2. Create following userContent.css in the profile/chrome folder

@namespace url(http://www.w3.org/1999/xhtml);

@-moz-document url-prefix(http://), url-prefix(ftp://), url-prefix(file://), url-prefix(https://) {


@font-face {
  font-family: "Arial";
  src: local("Times New Roman");
}


}

3. Start Nightly with the profile
4. Open the following 2 url in new tabs
   https://bugzilla.mozilla.org/show_bug.cgi?id=1075194
   https://developer.mozilla.org/en-US/search?q=fr
5. Exit Browser

Actual Results:
Browser craches

Expected Results:
Not crash
Summary: crash in nsCSSValue::nsCSSValue(nsCSSValue const&) → crash in nsCSSValue::nsCSSValue(nsCSSValue const&) on shutdown with certain sites if @font-face specified in userContent.css
Component: General → Layout
Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9883d2467b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20141001203029
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/109ae9a694cc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20141001205424
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0e9883d2467b&tochange=109ae9a694cc

Regressed by: Bug 1028497
Blocks: 1028497
Keywords: reproducible
OS: Windows NT → Windows 7
Thanks for the STR.  I get this assertion in a debug build:

Assertion failure: HasRule(), at /z/moz2/a/layout/style/FontFace.cpp:827
...
(gdb) bt
#0  0x00007ffff076dd02 in mozilla::dom::FontFace::DisconnectFromRule (this=0x7fffb6f1ff70) at /z/moz2/a/layout/style/FontFace.cpp:827
#1  0x00007ffff073d169 in mozilla::dom::FontFaceSet::DestroyUserFontSet (this=0x7fffdc1d81a0) at /z/moz2/a/layout/style/FontFaceSet.cpp:364
#2  0x00007ffff09825b2 in nsPresContext::SetShell (this=0x7fffb54f3800, aShell=0x0) at /z/moz2/a/layout/base/nsPresContext.cpp:1098
#3  0x00007ffff08a244c in PresShell::Destroy (this=0x7fffb52bf800) at /z/moz2/a/layout/base/nsPresShell.cpp:1256
...
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86 → All
I think the issue is that we share the style sheet across documents, and the nsCSSFontFaceRule only has a single pointer for its FontFace.  But the FontFace has a pointer to the FontFaceSet that is managing it, and there is one of those per document.  So we end up inserting the same FontFace into more than one FontFaceSet, and we run into trouble when we tear down the second FontFaceSet that has a hold of the one FontFace.
Two solutions come to mind:

(1) Move the mapping of nsCSSFontFaceRule to FontFace into the FontFaceSet.
(2) Have the nsCSSFontFaceRule store a table of (FontFaceSet, FontFace) entries or (nsPresContext, FontFace) entries.

(2) doesn't quite feel right since I don't think we store any document/prescontext-specific information on rules at the moment.
Attached patch patchSplinter Review
Assignee: nobody → cam
Attachment #8499956 - Flags: review?(jdaggett)
This also crashes if you have the MathML-fonts extension installed.
Summary: crash in nsCSSValue::nsCSSValue(nsCSSValue const&) on shutdown with certain sites if @font-face specified in userContent.css → crash in nsCSSValue::nsCSSValue(nsCSSValue const&) on shutdown with certain sites if @font-face specified in userContent.css of MathML-fonts installed
Summary: crash in nsCSSValue::nsCSSValue(nsCSSValue const&) on shutdown with certain sites if @font-face specified in userContent.css of MathML-fonts installed → crash in nsCSSValue::nsCSSValue(nsCSSValue const&) on shutdown with certain sites if @font-face specified in userContent.css or MathML-fonts installed
(In reply to Cameron McCormack (:heycam) (away October 6 - November 5) from comment #7)
> Created attachment 8499956 [details] [diff] [review]
> patch

I have verified that this patch fixes my issue from comment 9 that a crash occurs if you have the MathML-fonts extension installed.
(In reply to Cameron McCormack (:heycam) (away October 6 - November 5) from comment #8)
> https://tbpl.mozilla.org/?tree=Try&rev=2cc860745274

The M1 oranges here were a problem with the revision of inbound I was on, btw; there doesn't look to be any failures from my patch in the try run.
I am not but think maybe I should change the severity to blocker because I suspect it is blocking work on MathML because people working on that probably have the MathML-fonts extension installed.
I'm on Arch and Manjaro Linux and I get this every time I exit or restart fx. For me the cause is an add-on called Bamboo Feed Reader.

> https://addons.mozilla.org/en-US/firefox/addon/bamboo-feed-reader/?src=api
> https://crash-stats.mozilla.com/report/index/61581719-b2c9-4d19-b36d-d826c2141005
This is top crasher #1 now (>2.4k crashes since this came up). topcrash keyword?
Anyone who wants to keep up to date with nightly changes but can't live with this crasher are welcome to use my builds located at https://www.wg9s.com/mozilla/firefox/ which contain this fix as well as some pending MathML fixes.
Duplicate of this bug: 1078095
Attachment #8499956 - Flags: review?(jdaggett) → review+
https://hg.mozilla.org/mozilla-central/rev/7fceb8bf84d2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1079422
(In reply to Stephen Donner [:stephend] from comment #19)
> Looks like I might have found another case -- should I reopen or file a new
> bug?

There's also bug 1079422, does it fit that one?
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #20)
> (In reply to Stephen Donner [:stephend] from comment #19)
> > Looks like I might have found another case -- should I reopen or file a new
> > bug?
> 
> There's also bug 1079422, does it fit that one?

Doesn't seem to (to my untrained eye, at least -- the top 5 frames aren't consistent, or sometimes only the first 2 are) - I also crash on my nightly build for my Fennec on HTC One (M8), here: https://crash-stats.mozilla.com/report/index/fcdfa9cc-7b2c-4965-a87d-fa40d2141007

I'll file a new bug and we can cross-link/dupe as we see fit.
(In reply to Stephen Donner [:stephend] from comment #21)

> Doesn't seem to (to my untrained eye, at least -- the top 5 frames aren't
> consistent, or sometimes only the first 2 are) - I also crash on my nightly
> build for my Fennec on HTC One (M8), here:
> https://crash-stats.mozilla.com/report/index/fcdfa9cc-7b2c-4965-a87d-
> fa40d2141007
> 
> I'll file a new bug and we can cross-link/dupe as we see fit.

Just filed bug 1079620.
Reopening this as it doesn't appear to be a good fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: cam → jdaggett
(In reply to John Daggett (:jtd) from comment #23)
> Reopening this as it doesn't appear to be a good fix.

Well, you are too **** yourself.  It is not a bad fix, just does not fix all the issues.  Without this patch I can not run nighties at all, not even good enough to get them to run good enough to update to the latest nightly without crashing.
On closer inspection, the patch here doesn't address the fundamental problem, namely that since it's possible for @font-face rules to be shared across multiple documents, an individual nsCSSFontFaceRule object should *not* point to a FontFace.  It looks like the methods that reference GetFontFace are only within FontFaceSet::UpdateRules, so it would be smarter I think to build a map on the fly within that routine and not try to maintain a link from nsCSSFontFaceRule ==> FontFace.

With a user stylesheet present, logging calls to SetFontFace shows that multiple fontsets are sharing the same underlying @font-face rule:

userfont FontFaceForRule rule: 0x1180930c0 fontset: 0x12350b660 fontface: 0x12350e500
userfont FontFaceForRule rule: 0x1180930c0 fontset: 0x12a6e5c20 fontface: 0x1294a7d30

So I think the better way is to eliminate the need for the FontFace ptr altogether and the map that's maintained across calls to UpdateRules. I think this should eliminate both the original crash and the crashes associated with the patch here.
Patches to back out this change and fix the underlying problem on bug 1079422.
See also bug 1080086 comment #5 for a similar crash on SeaMonkey (at closedown after updating add-ons, then at first startup when restarting the crashed session). Dupe?
Oh, and BTW I have a userChrome.css and a userContent.css; my userChrome.css includes several font rules (including a font-family for all chrome elements).
Fixed by changes made for bug 1079422.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.