789719 - Frame Poison Crash [@ nsLineBox::IndexOf ] | [@ nsIFrame::GetNextSibling() ] during reflow

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Bob Clary [:bc] (inactive)]

1. http://www.mathwarehouse.com/algebra/linear_equation/linear-equation-interactive-activity.php

2. Crash Aurora, Nightly all platforms

bp-3a4e3076-3d7e-4789-aae4-dc9a22120908
Firefox 18.0a1 Crash Report [@ nsLineBox::IndexOf ] 

Maybe dupe of bug 762494

ss since Ted's exploitable rates this as low. iiuc, 0xfffffffff0de801f is poisoned.

Operating system: Windows NT
                  5.1.2600 Service Pack 3
CPU: x86
     GenuineIntel family 6 model 37 stepping 1
     1 CPU

Crash reason:  EXCEPTION_ACCESS_VIOLATION_READ
Crash address: 0xfffffffff0de801f

Thread 0 (crashed)
 0  xul.dll!nsIFrame::GetNextSibling() [nsIFrame.h : 1094 + 0xa]
    eip = 0x0238667a   esp = 0x00128f38   ebp = 0x00128f3c   ebx = 0x0cf9b4c0
    esi = 0x00000000   edi = 0x00000000   eax = 0xf0de7fff   ecx = 0xf0de7fff
    edx = 0x00000001   efl = 0x00210286
    Found by: given as instruction pointer in context
 1  xul.dll!nsLineBox::IndexOf(nsIFrame *) [nsLineBox.cpp : 293 + 0x7]
    eip = 0x024c90ec   esp = 0x00128f44   ebp = 0x00128f54
    Found by: call frame info
 2  xul.dll!nsLineBox::Contains(nsIFrame *) [nsLineBox.h : 514 + 0x30]
    eip = 0x0246a69a   esp = 0x00128f5c   ebp = 0x00128f68
    Found by: call frame info
 3  xul.dll!nsBlockInFlowLineIterator::nsBlockInFlowLineIterator(nsBlockFrame *,nsIFrame *,bool *) [nsBlockFrame.cpp : 5258 + 0x12]
    eip = 0x0246efcc   esp = 0x00128f70   ebp = 0x00128fb8
    Found by: call frame info
 4  xul.dll!nsBlockFrame::ChildIsDirty(nsIFrame *) [nsBlockFrame.cpp : 6503 + 0x13]
    eip = 0x02472256   esp = 0x00128fc0   ebp = 0x00129018
    Found by: call frame info
 5  xul.dll!PresShell::FrameNeedsReflow(nsIFrame *,nsIPresShell::IntrinsicDirty,unsigned __int64) [nsPresShell.cpp : 2611 + 0x16]
    eip = 0x02419810   esp = 0x00129020   ebp = 0x0012918c
    Found by: call frame info

Found regression between 20120728145538-20120729152337
Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=29bff59d3bbe&tochange=36c30260e7fa
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/07/2012-07-29-mozilla-central-debug/firefox-17.0a1.en-US.debug-linux-i686.tar.bz2
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/07/2012-07-30-mozilla-central-debug/firefox-17.0a1.en-US.debug-linux-i686.tar.bz2

I have a local version that reproduces. I'll reduce and attach when its ready.

Comment 1

[Bob Clary [:bc] (inactive)]

Attempts to remove the remote resources have failed. If you are interested in the somewhat reduced testcase, let me know but I don't think it would be useful to attach what I have here. Note, during reduction I found that reloading the page helped improve the reproducibility.

Comment 2

[Mats Palmgren (inactive)]

Bob, please attach the semi-reduced test, thanks.

I can reproduce in a Linux64 debug asan build, but not Linux64 debug NON-asan
(using the same profile).  It appears the SVG image doesn't even load in the
non-asan build.  Weird.  Maybe your semi-reduced test works better.

> Maybe dupe of bug 762494

Seems likely - bug 762494 comment 1 mentions SVG, which we have on the
stack here.

I think the problem is that PresShell::FrameNeedsReflow is called during
frame tree destruction, and poking about line/frame lists using
nsBlockInFlowLineIterator on an arbitrary frame in the tree doesn't
seem very safe.  Looking at the DeleteLineList call from
nsBlockFrame::DestroyFrom it's clear that the line/frame lists are
in flux here:
http://hg.mozilla.org/mozilla-central/annotate/1d4fc0c60063/layout/generic/nsBlockFrame.cpp#l281
http://hg.mozilla.org/mozilla-central/annotate/1d4fc0c60063/layout/generic/nsLineBox.cpp#l357

While we can make that more robust, I think we should first try making
nsSVGUtils::ScheduleReflowSVG post an async event to request the reflow
of |outerSVGFrame| (using nsWeakFrame).  Would that work?

Comment 3

[Mats Palmgren (inactive)]

Attachment: wip

This seems to fix it for me.
https://tbpl.mozilla.org/?tree=Try&rev=a068c1d42817

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: somewhat reduced testcase

Comment 5

[Mats Palmgren (inactive)]

It seems the patch made one test almost perma-orange:

REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/svg/as-image/img-blobURI-2.html | load failed: timed out waiting for pending paint count to reach zero (after reftest-wait removed and switch to print mode) (waiting for MozAfterPaint)

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: reduced testcase

Comment 8

[Robert Longson [:longsonr]]

I'm working on a fix in bug 792857 that might fix this too.

Comment 9

[Jonathan Watt [:jwatt]]

Robert, does that mean you don't think that Mats patch is an appropriate fix for this? (I was just trying to figure out whether it is or isn't.)

I would be curious to know how do we make sure that non-SVG callers of PresShell::FrameNeedsReflow don't call it while frames are being destroyed, if anyone can tell me that.

Comment 10

[Robert Longson [:longsonr]]

Mat's patch is not landable due to comment 5. I've taken a different approach. I pass a flag about to say that frames are being destroyed and don't reflow in that case (although I do invalidate). Still fixing/building it, so not sure if it's going to pass all the reftests yet.

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #10)
> Mat's patch is not landable due to comment 5.

Turning that test permaorange may be a good thing. Turning it from intermittent orange to perma orange can often make it a lot easier to figure out and fix the bug that's causing the intermittent orange.

Comment 12

[Robert Longson [:longsonr]]

Patch in bug 792857 fixes this and at least locally, doesn't send comment 5 reftest orange.

Comment 13

[Robert Longson [:longsonr]]

Sent to try as: https://tbpl.mozilla.org/?tree=Try&rev=e1ef9a0b74fe

Comment 14

[Robert Longson [:longsonr]]

Need to check in the crashtest from bug 472782 too I'll add that when I land this.

Comment 15

[Robert Kaiser]

The signatures seem to be extremely low volume, just FYI.

Comment 16

[Alex Keybl [:akeybl]]

(In reply to Robert Kaiser (:[e-mail]) from comment #15)
> The signatures seem to be extremely low volume, just FYI.

Given that, and the security rating, no need to track for release.

Comment 17

[Robert Longson [:longsonr]]

Fixed by bug 807213 which has now landed for 19, 18 and 17.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Is Firefox 16 affected or is this 17 and higher only?

Comment 19

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #17)
> Fixed by bug 807213 which has now landed for 19, 18 and 17.

What makes you say that, Robert?

For me, the testcase appears to stop crashing on Mac between:

2012-09-28-03-05-44-mozilla-central
2012-09-29-03-06-24-mozilla-central

Which would suggest that DLBI (bug 539356) fixed this for 18 and 19. The pushlog is:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=895f66c4eada&tochange=c09a0c022b2e

Comment 20

[Jonathan Watt [:jwatt]]

(In reply to Al Billings [:abillings] from comment #18)
> Is Firefox 16 affected or is this 17 and higher only?

Firefox 16 does not crash for me.

Firefox 17 b1 crashes for me, but b2 and later do not. Offhand I'm not sure what fixed this for 17.

Comment 21

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #19)
> (In reply to Robert Longson from comment #17)
> > Fixed by bug 807213 which has now landed for 19, 18 and 17.
> 
> What makes you say that, Robert?
> 

This was crashing for me when 18 was trunk, it didn't crash when I tested it after your fix landed and I couldn't think of anything else in between that might have fixed it. On top of that the testcase and site both have markers being deleted.

Comment 22

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on 2012-09-08, nightly
Confirmed fixed on 2013-01-14, nightly
Confirmed fixed on 2013-01-14, Aurora
Confirmed fixed on 2013-01-14, beta
Confirmed fixed on 17.0.2esr