399863 - We frequently call InitialUpdate() more than once on SVG frames

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

We often call InitialUpdate more than once on some SVG frames. We call InitialUpdate under two different circumstances:

 1) When DidReflow is called for the first time on an SVG document fragment's
    nsSVGOuterSVGFrame we call InitialUpdate on all the SVG frames in that
    fragment.

 2) When an SVG frame is inserted into the frame tree and the parent SVG
    frame already has child frames, nsSVGDisplayContainerFrame::InsertFrames
    calls InitialUpdate on the new frames once they've been inserted.

Since #2 can happen during parsing before the document has loaded, we can get a parser initiated InitialUpdate on frames followed by the InitialUpdate triggered by nsSVGOuterSVGFrame::DidReflow. Really we want to ignore the InitialUpdate call from nsSVGDisplayContainerFrame::InsertFrames if the fragment's nsSVGOuterSVGFrame hasn't received it's initial reflow.

Comment 1

[Jonathan Watt [:jwatt]]

We can probably ignore the nsSVGDisplayContainerFrame::InsertFrames calls when we need to without using nsSVGUtils::GetOuterSVGFrame if we make InitialUpdate remove the NS_FRAME_FIRST_REFLOW bit from SVG frames.

Comment 2

[Jonathan Watt [:jwatt]]

This issue is getting in the way of fixing bug 389198, and it's responsible for bugs such as nsSVGForeignObjectFrame being reflowed during frame construction when the tree is in an inconsistent state. According to Boris that's likely to result in broken reflow and risks (possibly exploitable) crashes.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch

InitialUpdate already removes the NS_FRAME_FIRST_REFLOW bit from SVG frames, so we just need to change nsSVGDisplayContainerFrame::InsertFrames to check for it. I've thrown in some assertions for bonus points.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch with new assertions

Having the assertions call nsSVGUtils::GetOuterSVGFrame for every frame makes loading SVG painfully slow, and it doesn't provide protection against InitialUpdate being called more than once. I've copied the outerSVGFrame assertion to DoReflow so it more directly protects against reflowing at the wrong time, and changed the other assertions to check that InitialUpdate is not called twice.

Comment 5

[Jonathan Watt [:jwatt]]

Checked in.

Comment 6

[pal-moz]

regression from 20071015_1517_firefox-3.0a9pre.en-US.win32.zip

check-in for this or bug 296818 cause crash.
http://crash-stats.mozilla.com/report/index/6c1ddbca-7b80-11dc-901b-001a4bd43ef6
(URL is wrong, correct URL is http://blog.livedoor.jp/ayase_shiori/)

Comment 7

[Jonathan Watt [:jwatt]]

The problem was with bug 296818.

Comment 8

[Reed Loden [:reed]]

Backed out as part of the quest to find the Tp regression.

Comment 9

[Robert Longson [:longsonr]]

Given this has been backed out, receives is misspelled in the comment in nsISVGChildFrame.h

Comment 10

[Jonathan Watt [:jwatt]]

Heh. Thanks.

Comment 11

[Jonathan Watt [:jwatt]]

Checked in again.