377833 - Crash on SVG element nested in a xul:stack

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Matthew Middleton (:zzxc)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070411 Minefield/3.0a4pre
Build Identifier: 

Putting HTML inside an xul element inside an svg:foreignObject inside an svg:svg element inside an xul:stack element inside an xul:window crashes Firefox in trunk builds.

(see attached testcase)


Reproducible: Always

Steps to Reproduce:
1.Load testcase
2.Firefox crashes
3.

Comment 1

[Matthew Middleton (:zzxc)]

Attachment: Crash testcase

Comment 2

[tor]

This is happening because nsStackLayout::AddOffset is adding NS_STATE_STACK_NOT_POSITIONED to its children, an outer <svg> in this case.  That state bit is treated as NS_STATE_SVG_CLIPPED_TRIVIAL by the SVG code, and things quickly go wrong.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So the simple fix is to not add that bit to kids that are not IsBoxFrame().

That said, this optimization seems to be broken in general -- it doesn't detect changes to the "left" and "top" style properties....  Maybe we can just remove it?  Especially once we start mapping XUL attrs into style?

Also, is this a problem on branch too?

Comment 4

[Robert Longson [:longsonr]]

This would be trunk only. SVG does not use state bits on the branch.

Comment 5

[Robert Longson [:longsonr]]

Attachment: patch

There seem to be various places GetChildBox is called which don't check whether the child is a box frame and then set/get state bits.

This fixes the crash. I'm not sure how to test the other changes.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 261934 [details] [diff] [review]
patch

>-    if ((box->GetStateBits() & (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN)) || childRect.width < clientRect.width) {
>+    if ((child->IsBoxFrame() &&
>+         (child->GetStateBits() & (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN))) ||
>+         childRect.width < clientRect.width) {

This change is wrong.  Those are generic nsIFrame bits, not box-specific bits.  So you want to check them for all frames.  Otherwise, e.g., modifying the width or height of an <svg> in a listbox won't work right.

In fact, none of the changes to nsListBoxLayout.cpp are needed that I can see.

Same for nsSprocketLayout.

Please parenthesize the first nsStackLayout change so people don't have to remember whether && or & binds stronger.

The third nsStackLayout change is wrong just like the listboxlayout and sprocket layout changes.

Comment 7

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 262102 [details] [diff] [review]
address review comments

Looks good!

Comment 9

[Robert Longson [:longsonr]]

checked in