Hold SVGFontFaceElement in SVGDocumentExtensions until StyleRecalc is finished

Hold SVGFontFaceElement in SVGDocumentExtensions until StyleRecalc is finished.

- added HashSet holding SVGFontFaceElement and cleared after document's style recalc is finished, and

- made widthIterator return default value when SVGFontFaceElement is not in document.

BUG=352178
TEST=fast/dom/remove-svg-font-face-element-crash.xhtml

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169774

[tasak, 2014-03-17 06:30:45]

PTAL?

[kouhei (in TOK), 2014-03-17 09:39:26]

non-owner lgtm

https://codereview.chromium.org/198783006/diff/1/LayoutTests/fast/dom/remove-...
File LayoutTests/fast/dom/remove-svg-font-face-element-crash.xhtml (right):

https://codereview.chromium.org/198783006/diff/1/LayoutTests/fast/dom/remove-...
LayoutTests/fast/dom/remove-svg-font-face-element-crash.xhtml:5: <defs>
Nit: indent consistently?

[dglazkov, 2014-03-17 16:31:27]

+esprehn.

This fix is pretty gnarly. Elliott, WDYT?

[esprehn, 2014-03-17 17:12:01]

not lgtm, the RenderStyle should store a RefPtr instead. We could also use a
WeakPtr and clear it when the SVGFontFace is removed.

Doing a synchronous recalc isn't right though.

https://codereview.chromium.org/198783006/diff/1/Source/core/svg/SVGFontFaceE...
File Source/core/svg/SVGFontFaceElement.cpp (right):

https://codereview.chromium.org/198783006/diff/1/Source/core/svg/SVGFontFaceE...
Source/core/svg/SVGFontFaceElement.cpp:344: recalcTime = RecalcStyleImmediately;
This is bad, it means if you have a bunch of fonts and remove them we'll spam
the document with recalcs.

Why doesn't the font store a RefPtr to the SVGFontFaceElement?

[tasak, 2014-03-18 02:23:04]

Thank you for reviewing.

The reason why I didn't change SVGFontFaceElement* to RefPtr<SVGFontFaceElement>
in SVGFontData.h is  http://trac.webkit.org/changeset/37222

i.e.
fix SVGFontFaceElement leaks seen in Acid3

svg/SVGFontData.h: Changed the m_svgFontFaceElement member from a
    RefPtr to a plain pointer to break a ref cycle.
    (WebCore::SVGFontData::svgFontFaceElement):

I think, to do so, we need to refactor SVGFontData (and also platform font?).
However, it is too much task, because SVGFont will be deprecated in the near
future.

And another reason is that only few people use SVG font, i.e.
SVGFontInCSS (0.1991%)
http://www.chromestatus.com/metrics/feature/timeline/popularity/156

SVGFontElement (0.0112%)
http://www.chromestatus.com/metrics/feature/timeline/popularity/76

> Doing a synchronous recalc isn't right though.

So is it ok to traverse RenderTree to invalidate RenderStyle using SVGFontData?

https://codereview.chromium.org/198783006/diff/1/Source/core/svg/SVGFontFaceE...
File Source/core/svg/SVGFontFaceElement.cpp (right):

https://codereview.chromium.org/198783006/diff/1/Source/core/svg/SVGFontFaceE...
Source/core/svg/SVGFontFaceElement.cpp:344: recalcTime = RecalcStyleImmediately;
On 2014/03/17 17:12:01, esprehn wrote:
> This is bad, it means if you have a bunch of fonts and remove them we'll spam
> the document with recalcs.
> 
> Why doesn't the font store a RefPtr to the SVGFontFaceElement?

I agree that this is a bad solution.

[tasak, 2014-03-18 03:48:50]

On 2014/03/18 02:23:04, tasak wrote:
> 
> So is it ok to traverse RenderTree to invalidate RenderStyle using
SVGFontData?

I will try another approach, i.e. adding Vector<RefPtr<SVGFontFaceElement> > or
HashSet<RefPtr<SVGFontFaceElement> > to document and clear the vector (or the
hashset) after finishing document's style recalc.

[tasak, 2014-03-18 10:43:26]

Would you review this new approach?

https://codereview.chromium.org/198783006/diff/1/LayoutTests/fast/dom/remove-...
File LayoutTests/fast/dom/remove-svg-font-face-element-crash.xhtml (right):

https://codereview.chromium.org/198783006/diff/1/LayoutTests/fast/dom/remove-...
LayoutTests/fast/dom/remove-svg-font-face-element-crash.xhtml:5: <defs>
On 2014/03/17 09:39:27, kouhei wrote:
> Nit: indent consistently?

Done.

[esprehn, 2014-03-20 04:02:45]

lgtm

[esprehn, 2014-03-20 04:02:49]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-20 04:02:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/198783006/40001

[commit-bot: I haz the power, 2014-03-20 04:06:05]

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

[commit-bot: I haz the power, 2014-03-20 04:06:05]

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

[esprehn, 2014-03-21 21:08:30]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-21 21:08:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/198783006/40001

[commit-bot: I haz the power, 2014-03-22 04:07:53]

Change committed as 169774