[SVG Fonts] Fix <font-face> element leak document

[SVG Fonts] Fix <font-face> element leak document

Reported by hajimehoshi@: A svg document with <font-face> was never destructed from a reference cycle.

This patch breaks the reference cycle by making the reference from SVGFontFaceSource to SVGFontFaceElement weak.
SVGFontFaceSource only exist for in-document font face reference, so this change is safe.

This patch also validates SVGFontData's weak-ref to SVGFontFaceElement. SVGFontData should be only used while the document is alive. This CL changes the weak-ref to WeakPtr and ensure the svg tree is alive before reference.

TEST=LayoutTests/svg/svg-font-face-leak-document.html
BUG=270000
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171231

[Stephen Chennney, 2014-03-28 11:24:52]

We've had a lot of security issues in this area, which leaves me rather
hesitant. The code seems right, however, and Oilpan means it will be fixed
properly sometime reasonably soon.

Is there any way at all that the SVGSourceElement can continue to live after the
associated document has been destroyed? Can JS hold on to a reference somehow?

What's the history on the original RefPtr? Was it added to address a specific
problem?

[kouhei (in TOK), 2014-03-31 00:00:04]

+tasak

[kouhei (in TOK), 2014-04-01 01:20:34]

+ksakamoto

Hi, I updated the CL with more conservative fix. SVGFontData now carries a
WeakPtr to the SVGFontFaceElement, and RELEASE_ASSERT checks that
SVGFontFaceElement is still alive so that it won't cause UAF.

On 2014/03/28 11:24:52, Stephen Chenney wrote:
> We've had a lot of security issues in this area, which leaves me rather
> hesitant. The code seems right, however, and Oilpan means it will be fixed
> properly sometime reasonably soon.
Oilpan will fix the problem, but it is expected to take more than 3 months
before shipping Node/CSS, so I want to land this fix with RELEASE_ASSERT first,
then remove these RELEASE_ASSERT when oilpan is landed.

> Is there any way at all that the SVGSourceElement can continue to live after
the
> associated document has been destroyed? Can JS hold on to a reference somehow?
SVGFontFaceSource is held by FontFace, and its lifetime is same as the document.
I can't think of a way that JS can hold to the reference, but going conservative
way with WeakPtr/RELEASE_ASSERTs.

> What's the history on the original RefPtr? Was it added to address a specific
> problem?
http://webkit.org/b/53270 seems to be the bug, but I don't have a read access to
it. The discussion on https://bugs.webkit.org/show_bug.cgi?id=66438 mentions the
bug, but I think this bug shouldn't be a problem as long as
svg/custom/use-multiple-on-nested-disallowed-font.html pass.

[Kunihiko Sakamoto, 2014-04-01 04:44:13]

Can you update the change description?

https://codereview.chromium.org/216563002/diff/70001/Source/core/svg/SVGFontD...
File Source/core/svg/SVGFontData.h (right):

https://codereview.chromium.org/216563002/diff/70001/Source/core/svg/SVGFontD...
Source/core/svg/SVGFontData.h:50: SVGFontFaceElement* svgFontFaceElement() const
{ return m_svgFontFaceElement.get(); }
Maybe RELEASE_ASSERT in this method? A couple of classes other than SVGFontData
is calling this.

[kouhei (in TOK), 2014-04-03 01:57:17]

PTAL.

https://codereview.chromium.org/216563002/diff/70001/Source/core/svg/SVGFontD...
File Source/core/svg/SVGFontData.h (right):

https://codereview.chromium.org/216563002/diff/70001/Source/core/svg/SVGFontD...
Source/core/svg/SVGFontData.h:50: SVGFontFaceElement* svgFontFaceElement() const
{ return m_svgFontFaceElement.get(); }
On 2014/04/01 04:44:14, Kunihiko Sakamoto wrote:
> Maybe RELEASE_ASSERT in this method? A couple of classes other than
SVGFontData
> is calling this.

Done.

[Kunihiko Sakamoto, 2014-04-03 03:17:08]

non-owner lgtm

https://codereview.chromium.org/216563002/diff/110001/Source/core/svg/SVGFont...
File Source/core/svg/SVGFontData.cpp (right):

https://codereview.chromium.org/216563002/diff/110001/Source/core/svg/SVGFont...
Source/core/svg/SVGFontData.cpp:337: return !m_svgFontFaceElement ||
!m_svgFontFaceElement->inDocument();
This is workaround for crbug.com/359380, right?
If so, please add a FIXME comment here as well.

[kouhei (in TOK), 2014-04-03 09:17:58]

schenney: PTAL.

https://codereview.chromium.org/216563002/diff/110001/Source/core/svg/SVGFont...
File Source/core/svg/SVGFontData.cpp (right):

https://codereview.chromium.org/216563002/diff/110001/Source/core/svg/SVGFont...
Source/core/svg/SVGFontData.cpp:337: return !m_svgFontFaceElement ||
!m_svgFontFaceElement->inDocument();
On 2014/04/03 03:17:08, Kunihiko Sakamoto wrote:
> This is workaround for crbug.com/359380, right?
> If so, please add a FIXME comment here as well.

Done.

[kouhei (in TOK), 2014-04-09 08:07:56]

schenney: PTAL.

I think the patch is safe now.

[Stephen Chennney, 2014-04-09 14:27:19]

LGTM. Good work and I apologise for the delay.

[kouhei (in TOK), 2014-04-10 00:58:00]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-04-10 00:58:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/216563002/150001

[commit-bot: I haz the power, 2014-04-10 01:50:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-10 01:50:31]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[kouhei (in TOK), 2014-04-10 03:07:56]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-04-10 03:08:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/216563002/150001

[commit-bot: I haz the power, 2014-04-10 03:59:22]

Change committed as 171231