Closed Bug 1297664 Opened 3 years ago Closed 3 years ago

SVG-in-OpenType font with iframe inside crashes Firefox

Categories

(Core :: Layout, defect)

38 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox-esr45 --- wontfix
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: roel, Assigned: kanru)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files)

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.
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86
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
See Also: → 1295940
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Keywords: crash
Product: Firefox → Core
I am not able to reproduce this. Can someone who can reproduce this post a crash report from about:crashes?
Flags: needinfo?(roel)
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.
Blocks: 1118169
Markus, could you have a look at this?
Flags: needinfo?(mstange)
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
Flags: needinfo?(mstange)
Attachment #8787596 - Flags: approval-mozilla-beta?
Attachment #8787596 - Flags: approval-mozilla-aurora?
Sounds good, we can approve this once it lands in m-c.
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/34e0bd0f5df0
Null check docShell before use. r=mstange
https://hg.mozilla.org/mozilla-central/rev/34e0bd0f5df0
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Crash Signature: [@ nsDisplayListBuilder::EnterPresShell]
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
Attachment #8787596 - Flags: approval-mozilla-beta?
Attachment #8787596 - Flags: approval-mozilla-beta-
Attachment #8787596 - Flags: approval-mozilla-aurora?
Attachment #8787596 - Flags: approval-mozilla-aurora+
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.
Flags: needinfo?(roel)
Duplicate of this bug: 1295940
I filed bug 1307666 for the new crash.
Version: 48 Branch → 38 Branch
You need to log in before you can comment on or make changes to this bug.