310436 - Crash [@ IsContinuationPlaceholder] involving mixed SVG and HTML

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

The testcase crashes both Sept 28 trunk and Sept 28 branch.

Filing as security-sensitive because before I reduced the testcase, I saw
crashes [@ nsLineBox::DeleteLineList] with 0x0000011d on top (Gecko jumped to a
bogus address) and crashes [@ DoDeletingFrameSubtree] in addition to crash [@
IsContinuationPlaceholder] shown by the reduced testcase.

TB9857824Y

Comment 1

[Jesse Ruderman]

Attachment: testcase (crashes firefox)

Comment 2

[Jesse Ruderman]

See also bug 310638, another crash involving mixed SVG and HTML and DOM
manipulation.

Comment 3

[Asa Dotzler [:asa]]

->tor

Comment 4

[tor]

This isn't a SVG specific issue I don't think, but more about how we're tied to
the rest of gecko.

One way of fixing this might be to avoid constructing frames for non-svg
children of svg frames (except the case of foreignObject).  Would that be the
right thing to do?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yes, not constructing non-svg frames as children of svg frames is probably the
right way to go.

Comment 6

[tor]

Attachment: don't do frame creation for non-svg children of svg

Comment 7

[Boris Zbarsky [:bzbarsky]]

Er... Why do we care what the parent _element_ is.  We should be caring about the parent _frame_, no?

Comment 8

[Boris Zbarsky [:bzbarsky]]

To be more specific, with XBL there is no reason that an SVG element would have an SVG frame.

Comment 9

[tor]

Attachment: hopefully looking at the right information now

Comment 10

[Boris Zbarsky [:bzbarsky]]

That does look better, though I still really don't like doing this on every single frame construction.  :(  Especially given that ResolveTag is not exactly super-cheap.

Is there a reason not to just store information about whether the current parent is svg in the state or something?

Also, I suspect that using undisplayed content here is undesirable, but it's not that well-defined what undisplayed content means.

I have to ask -- are we targeting 1.8 branch here, or 1.9?  For the latter, I think we can come up with better solutions.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So the summary is:  For trunk, that patch gets r-.  For branch, I really don't like the perf hit, but if this is branch-critical I need to know ASAP so that I can start trying to figure out how to fix it on branch.  Please let me know.

Comment 12

[Jesse Ruderman]

I think it is not critical to have this fixed in Firefox 1.5.

Comment 13

[Mike Schroepfer]

Tor - can we pick this up for the 1.8.0.1 release in Jan?

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Alternative patch

This takes an alternative approach by implementing IsFrameOfType. Seems to work with the testcase in this bug and bug 310638. The mathml changes aren't neccesary, but I had them in my tree anyway so I left them in.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 208128 [details] [diff] [review]
Alternative patch

r=bzbarsky

Comment 16

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

Comment on attachment 208128 [details] [diff] [review]
Alternative patch

>Index: generic/nsIFrame.h
>+  virtual PRBool IsFrameOfType(PRUint32 aFlags) const
>+  {
>+    return PR_FALSE;

Seems like this should be |return !aFlags;|

Comment 17

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

Comment on attachment 208128 [details] [diff] [review]
Alternative patch

>+    aState.mFrameManager->SetUndisplayedContent(aContent, styleContext);

What was the rationale for this?  The primary purpose of the undisplayed content map is to cache style contexts for frames that are undisplayed as a result of 'display:none' style so that if those style contexts get a style change that makes them other than 'display:none', we pick it up and display them.

Is the undisplayed content map used for something else as well that makes this necessary?  If not, please remove it.  If so, why?

(I looked through LXR:  The two XBL callers of GetUndisplayedContent I don't quite understand; there's only one other inappropriate caller of SetUndisplayedContent, and that's in XTF code.)

Other than that and the comment above, sr=dbaron.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Noone should really be calling IsFrameOfType with aFlags=0, but there's no harm in supporting it so i'll make that change. Forgot about the SetUndisplayedContent thing, you're right, it's not needed.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in. We should land this on the branches too as it should be very safe.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: final version

This is what was checked in

Comment 21

[Jesse Ruderman]

*** Bug 323022 has been marked as a duplicate of this bug. ***

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This regressed text inside svg. Patch coming up.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Tim: so should we allow textframes inside all svg-frames? Or just inside <svg:text> frames?

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fix svg:text

This should fix svg:text

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 208128 [details] [diff] [review]
Alternative patch

This contains code that bug 317544 depends on. The patch is pretty safe and has not caused any regressions on trunk (apart from one that was fixed in this bug)

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 209663 [details] [diff] [review]
Fix svg:text

This one goes together with the other patch in this bug to avoid regressions.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 209663 [details] [diff] [review]
Fix svg:text

approved for 1.8.0 branch, a=dveditz for drivers

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 209641 [details] [diff] [review]
final version

This is the version that needs to land.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 209641 [details] [diff] [review]
final version

approved for 1.8.0 branch, a=dveditz for drivers

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in on 1.8.0 branch

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, it makes no sense to block any of the old releases for this since 1.5 was the first to ship with SVG.

Comment 33

[Dave Liebreich [:davel]]

Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)

Comment 34

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060301 Firefox/1.5.0.1, no more crashy.

Comment 35

[Mike Kaply [:mkaply]]

Jonas, are you planning to land this on 1.8.1?

Comment 36

[Mike Schroepfer]

Jonas - can you like on 1.8 branch :-)?

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

checked in on the 1.8 branch

Comment 38

[Boris Zbarsky [:bzbarsky]]

So... this changed nsIFrame on the branches. Not only that, but it inserted a method into the middle of a vtable and did NOT change the IID (something I should have caught in the trunk patch :().

And of course we've already shipped security releases with it... :(  Is there any way to make this sane on branch at this point, or do we just live with it and tell people who want to use nsIFrame to not target pre-1.8.0.2 builds?

Comment 39

[Benjamin Smedberg]

I really can't imagine nsIFrame mattering, really... it's not a refcounted object, and it's not really usable by extensions.

But we really should have changed the IID.

Comment 40

[Boris Zbarsky [:bzbarsky]]

I know various "extension" like things that use nsIFrame to get at layout geometry....  Luckily, all those methods are inline, so not affected by the vtable.

Comment 41

[Bob Clary [:bc] (inactive)]

ffb2 debug/nightly windows/linux no crash
verified fixed 1.8

Comment 42

[Jonathan Watt [:jwatt]]

bclary, you marked this as in‑testsuite+ - can you tell me which test suite and which test covers this bug?

Comment 43

[Bob Clary [:bc] (inactive)]

(In reply to comment #42)
> bclary, you marked this as in‑testsuite+ - can you tell me which test suite
> and which test covers this bug?
> 

it was in an internal only suite that will not be carried forward.

Comment 44

[Jesse Ruderman]

Crashtest checked in.