Closed Bug 310436 Opened 19 years ago Closed 19 years ago

Crash [@ IsContinuationPlaceholder] involving mixed SVG and HTML


(Core :: SVG, defect)

Windows XP
Not set





(Reporter: jruderman, Assigned: sicking)



(4 keywords, Whiteboard: [sg:critical?] uses freed memory [rft-dl])

Crash Data


(3 files, 3 obsolete files)

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.

Whiteboard: [sg:fix]?
See also bug 310638, another crash involving mixed SVG and HTML and DOM
Assignee: general → 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?
Yes, not constructing non-svg frames as children of svg frames is probably the
right way to go.
Attachment #200614 - Flags: review?(bzbarsky)
Er... Why do we care what the parent _element_ is.  We should be caring about the parent _frame_, no?
To be more specific, with XBL there is no reason that an SVG element would have an SVG frame.
Attachment #200614 - Attachment is obsolete: true
Attachment #200659 - Flags: review?(bzbarsky)
Attachment #200614 - Flags: review?(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.
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.
I think it is not critical to have this fixed in Firefox 1.5.
Attachment #200659 - Flags: review?(bzbarsky) → review-
Tor - can we pick this up for the release in Jan?
Flags: blocking1.8.0.1?
Whiteboard: [sg:fix]? → [sg:critical?] uses freed memory
Flags: blocking1.8.1?
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1-
Attached patch Alternative patch (obsolete) — Splinter Review
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.
Attachment #208128 - Flags: superreview?(dbaron)
Attachment #208128 - Flags: review?(bzbarsky)
Comment on attachment 208128 [details] [diff] [review]
Alternative patch

Attachment #208128 - Flags: review?(bzbarsky) → review+
Blocks: 323022
Assignee: tor → bugmail
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 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.
Attachment #208128 - Flags: superreview?(dbaron) → superreview+
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.
Checked in. We should land this on the branches too as it should be very safe.
Closed: 19 years ago
Resolution: --- → FIXED
Attached patch final versionSplinter Review
This is what was checked in
Attachment #200659 - Attachment is obsolete: true
Attachment #208128 - Attachment is obsolete: true
*** Bug 323022 has been marked as a duplicate of this bug. ***
No longer blocks: 323022
This regressed text inside svg. Patch coming up.
Resolution: FIXED → ---
Tim: so should we allow textframes inside all svg-frames? Or just inside <svg:text> frames?
Attached patch Fix svg:textSplinter Review
This should fix svg:text
Attachment #209663 - Flags: review?
Attachment #209663 - Flags: review? → review?(tor)
Attachment #209663 - Flags: review?(tor) → review+
Checked in
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2+
Blocks: 317544
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)
Attachment #208128 - Flags: approval1.8.0.2?
Comment on attachment 209663 [details] [diff] [review]
Fix svg:text

This one goes together with the other patch in this bug to avoid regressions.
Attachment #209663 - Flags: approval1.8.0.2?
Comment on attachment 209663 [details] [diff] [review]
Fix svg:text

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #209663 - Flags: approval1.8.0.2? → approval1.8.0.2+
Comment on attachment 209641 [details] [diff] [review]
final version

This is the version that needs to land.
Attachment #209641 - Flags: approval1.8.0.2?
Comment on attachment 209641 [details] [diff] [review]
final version

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #209641 - Flags: approval1.8.0.2? → approval1.8.0.2+
Checked in on 1.8.0 branch
Keywords: fixed1.8.0.2
Flags: blocking1.7.13-
Flags: blocking-aviary1.0.8-
Actually, it makes no sense to block any of the old releases for this since 1.5 was the first to ship with SVG.
Marking [rft-dl] (ready for testing in Firefox release candidates)
Whiteboard: [sg:critical?] uses freed memory → [sg:critical?] uses freed memory [rft-dl]
v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20060301 Firefox/, no more crashy.
Flags: in-testsuite+
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Jonas, are you planning to land this on 1.8.1?
Group: security
Attachment #209641 - Flags: approval1.8.1+
Attachment #209663 - Flags: approval1.8.1+
Jonas - can you like on 1.8 branch :-)?
checked in on the 1.8 branch
Keywords: fixed1.8.1
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- builds?
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.
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.
ffb2 debug/nightly windows/linux no crash
verified fixed 1.8
bclary, you marked this as in‑testsuite+ - can you tell me which test suite and which test covers this bug?
(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.
Flags: in-testsuite+ → in-testsuite-
Flags: in-testsuite- → in-testsuite?
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ IsContinuationPlaceholder]
You need to log in before you can comment on or make changes to this bug.