If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Crash on SVG element nested in a xul:stack

RESOLVED FIXED

Status

()

Core
SVG
--
critical
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: zzxc, Assigned: Robert Longson)

Tracking

unspecified
x86
All
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 261908 [details]
Crash testcase
(Reporter)

Updated

11 years ago
Severity: major → critical

Comment 2

11 years ago
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.
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9?
(Reporter)

Updated

11 years ago
Summary: Nested SVG and XUL elements cause crash. → Crash on SVG element in a xul:stack (Nested SVG and XUL elements cause crash)
(Assignee)

Comment 4

11 years ago
This would be trunk only. SVG does not use state bits on the branch.
(Assignee)

Comment 5

11 years ago
Created attachment 261934 [details] [diff] [review]
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.
Attachment #261934 - Flags: superreview?(bzbarsky)
Attachment #261934 - Flags: review?(bzbarsky)

Updated

11 years ago
Summary: Crash on SVG element in a xul:stack (Nested SVG and XUL elements cause crash) → Crash on SVG element nested in a xul:stack
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.
Attachment #261934 - Flags: superreview?(bzbarsky)
Attachment #261934 - Flags: superreview-
Attachment #261934 - Flags: review?(bzbarsky)
Attachment #261934 - Flags: review-
(Assignee)

Comment 7

11 years ago
Created attachment 262102 [details] [diff] [review]
address review comments
Assignee: general → longsonr
Attachment #261934 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #262102 - Flags: superreview?(bzbarsky)
Attachment #262102 - Flags: review?(bzbarsky)
Comment on attachment 262102 [details] [diff] [review]
address review comments

Looks good!
Attachment #262102 - Flags: superreview?(bzbarsky)
Attachment #262102 - Flags: superreview+
Attachment #262102 - Flags: review?(bzbarsky)
Attachment #262102 - Flags: review+
(Assignee)

Comment 9

11 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.