Closed Bug 1297664 Opened 3 years ago Closed 3 years ago
Type font with iframe inside crashes Firefox
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.82 Safari/537.36 OPR/39.0.2256.48 Steps to reproduce: I created an SVG-in-OpenType test font. One of the glyphs contains an SVG with an iFrame inside. When this character is in the document, Firefox crashes. 1. Go to https://pixelambacht.nl/lapislegit/ 2. Go the the letter Q (it will show an exclamation mark) 3. Change the ! into a Q 4. Watch Firefox crash Actual results: Firefox crashes when document contains a character that's rendered in an SVG-in-OpenType font and the glyph contains an SVG with an iframe. Expected results: The glyph should've rendered, ignoring the iframe, or it should ignore the SVG glyph completely and show the normal fallback glyph.
Create a font with this SVG as a glyph to crash Firefox, or check out a test font containing this character from https://github.com/RoelN/LapisLegit
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Product: Firefox → Core
Here's regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ae5d04409cd9&tochange=0c2f7434c325
I am not able to reproduce this. Can someone who can reproduce this post a crash report from about:crashes?
Here's crash reports. Firefox 48.0.1: https://crash-stats.mozilla.com/report/index/55b97950-2e47-4610-a81b-0f6452160829 Nightly (2016-08-29): https://crash-stats.mozilla.com/report/index/502fc34d-c04d-4f9d-bedc-e1e742160829
Thanks arai. Seems related to bug 1295940, which you already hooked up. The stacks seem pretty similar. I'm going to dupe this over unless there are any objections.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1295940
Unless you're confident that the users who are crashing are crashing because of this bug, which is related to SVG-in-OpenType, this shouldn't be a duplicate.
Actually, I'm going to say: it's better to fix this bug here, and then if that fixes the crashes in the wild, great, but if it doesn't, we won't have mass confusion in bugzilla due to having put a bunch of work in the wrong bug and then needing to reopen it or file another version of it.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
That's fair - thanks for re-opening.
Presumably a regression from https://hg.mozilla.org/integration/mozilla-inbound/rev/6f69ad74baa5 which is both in the regression range in comment 2, and the last changeset to touch the line on which we crash.
Markus, could you have a look at this?
I can reproduce this. It because nsPresContext::GetDocShell can return nullptr so we should never use the return value directly. I wonder if we can change the return value to Maybe<nsIDocShell*>
This regression exists in Firefox 49 which goes to release candidate builds next week so our window is closing here...
Assignee: nobody → kchen
Comment on attachment 8787596 [details] Bug 1297664 - Null check docShell before use. https://reviewboard.mozilla.org/r/76316/#review74484 Thanks Kan-Ru! The "Get" in the name of the function already expresses the fact that it can be null; if it was guaranteed to always be non-null then it would be called ->DocShell(). So we don't need a Maybe<> here.
Attachment #8787596 - Flags: review?(mstange) → review+
Comment on attachment 8787596 [details] Bug 1297664 - Null check docShell before use. I think we should uplift this as soon as possible. It's basically a zero risk patch. Approval Request Comment [Feature/regressing bug #]: bug 1118169 (this bug has been shipping since 38) [User impact if declined]: crash on certain pages [Describe test coverage new/current, TreeHerder]: this particular case does not have a test, we should add a crashtest but this patch doesn't add one [Risks and why]: extremely low, just a null-check for the case where we would have crashed [String/UUID change made/needed]: none
Sounds good, we can approve this once it lands in m-c.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/34e0bd0f5df0 Null check docShell before use. r=mstange
Comment on attachment 8787596 [details] Bug 1297664 - Null check docShell before use. While it is great to fix a crash, it is not a new crash, the volume is very small, it is too late minute (we are building rc1), so, I prefer to let it ride the train from 50 to avoid any unexpected side effect
This is still reproducible in low volume on Fx50, based on last month's worth of crash data. SIGNATURE | nsDisplayListBuilder::EnterPresShell -------------------------------------------------- CRASH STATS | http://tinyurl.com/zp4lodz -------------------------------------------------- OVERVIEW | 0 crashes on nightly 52 | 0 crashes on nightly 51 | 0 crashes on aurora 51 | 0 crashes on nightly 50 | 1 crashes on aurora 50 | 2 crashes on beta 50 -------------------------------------------------- LAST CRASH | 2016-10-02 (on 50.0b3)
(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #21) > This is still reproducible in low volume on Fx50, based on last month's > worth of crash data. Same signature but different call stack. Looks likely a different issue.
I filed bug 1307666 for the new crash.
You need to log in before you can comment on or make changes to this bug.