1297664 - SVG-in-OpenType font with iframe inside crashes Firefox

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Roel Nieskens]

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.

Comment 1

[Roel Nieskens]

Attachment: SVG that, when used as OpenType SVG glyph, crashes Firefox

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

Comment 2

[Tooru Fujisawa [:arai]]

Here's regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ae5d04409cd9&tochange=0c2f7434c325

Comment 3

[Mike Conley (:mconley) (:⚙️) (PTO Dec 22 - Jan 2)]

I am not able to reproduce this. Can someone who can reproduce this post a crash report from about:crashes?

Comment 4

[Tooru Fujisawa [:arai]]

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

Comment 5

[Mike Conley (:mconley) (:⚙️) (PTO Dec 22 - Jan 2)]

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.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 8

[Mike Conley (:mconley) (:⚙️) (PTO Dec 22 - Jan 2)]

That's fair - thanks for re-opening.

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Markus, could you have a look at this?

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

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*>

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Bug 1297664 - Null check docShell before use.

Review commit: https://reviewboard.mozilla.org/r/76316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76316/

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

This regression exists in Firefox 49 which goes to release candidate builds next week so our window is closing here...

Comment 14

[Markus Stange [:mstange]]

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.

Comment 15

[Markus Stange [:mstange]]

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

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Sounds good, we can approve this once it lands in m-c.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34e0bd0f5df0
Null check docShell before use. r=mstange

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34e0bd0f5df0

Comment 19

[Sylvestre Ledru [:Sylvestre]]

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

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/03212a10d20d

Comment 21

[Andrei Vaida [:avaida]]

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)

Comment 22

[Kan-Ru Chen [:kanru] (UTC+9)]

(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.

Comment 24

[Kan-Ru Chen [:kanru] (UTC+9)]

I filed bug 1307666 for the new crash.